[RFCv2] static builtin CCIDs was Re: [PATCH 2/5] dccp: Auto-load (when supported) CCID plugins for negotiation

Sorry if it gets duplicated, I'm having some mail problems :-\

Em Wed, Dec 17, 2008 at 04:42:28PM -0200, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Dec 17, 2008 at 04:30:41PM -0200, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Dec 17, 2008 at 04:20:38PM -0200, Arnaldo Carvalho de Melo escreveu:
> > > > IOW we're back to my suggestion on looking at
> > > > tcp_set_congestion_control(). :-)
> > > 
> > > I tried to test this using ttcp over loopback but the tree seems broken
> > > somehow, with or without this patch I'm getting:
> > > 
> > > Could not activate 0 at /home/acme/git/net-next-2.6/net/dccp/feat.c:1176
> > > 
> > > I tried doing a quick chase on this one but failed miserably, Gerrit,
> > > any ideas?
> > 
> > Well, without the patch the problem was that dccp_ccid2 was not being
> > autoloaded, as soon as I manually loaded it, ttcp worked. Now to see
> > why...
> 
> Autoloading mess indeed... what probably is happening is that I start
> the server, that will not try to load the modules it advertises right
> away, but instead wait to do that when the connection completes, but
> then...
> 
> OK, back to my patch to check why it finds ccids[2] as NULL when it
> shouldn't...

Ok, now it survives basic testing, i.e.:

[root@mica ~]# ~acme/ttcp -l256 -cacme -t localhost
ttcp-t: buflen=256, nbuf=2048, align=16384/+0, port=5001  dccp(inet)  ->
localhost
ttcp-t: socket
ttcp-r: accept from mica.ghostprotocols.net
ttcp-t: connect
ttcp-t: 524288 bytes in 0.02 real seconds = 23517.52 KB/sec +++
ttcp-t: 2048 I/O calls, msec/call = 0.01, calls/sec = 94070.09
ttcp-t: 0.0user 0.0sys 0:00real 50% 0i+0d 0maxrss 0+1pf 0+2048csw
ttcp-r: 524288 bytes in 0.02 real seconds = 23164.28 KB/sec +++
ttcp-r: 2049 I/O calls, msec/call = 0.01, calls/sec = 92702.35
ttcp-r: 0.0user 0.0sys 0:00real 0% 0i+0d 0maxrss 0+1pf 2049+0csw
[1]+  Done                    ~acme/ttcp -l256 -cacme -r
[root@mica ~]# 

The problem was that I was not selecting IP_DCCP_ACKVEC, that is needed
by CCID2, with that CCID2 always loads, this option makes no sense and
is thus removed.

So, if nobody sees any problem, please apply.

Build CCID2, because the RFC requires it to be always available, and
CCID3 too as it is the most interesting one for VoIP, etc, together with
the main, layer 3 agnostic, DCCP core code, so that we have a faster
connection path by eliminating the need to always go thru the CCID
registration lock. But keep it there, so that we can experiment with
newer CCIDs without having to rebuild/reboot the whole kernel.

Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>


----- End forwarded message -----
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Date: Wed, 17 Dec 2008 19:46:08 -0200

> So, if nobody sees any problem, please apply.
> 
> Build CCID2, because the RFC requires it to be always available, and
> CCID3 too as it is the most interesting one for VoIP, etc, together with
> the main, layer 3 agnostic, DCCP core code, so that we have a faster
> connection path by eliminating the need to always go thru the CCID
> registration lock. But keep it there, so that we can experiment with
> newer CCIDs without having to rebuild/reboot the whole kernel.
> 
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Gerrit, are you OK with this?
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

| > Build CCID2, because the RFC requires it to be always available, and
| > CCID3 too as it is the most interesting one for VoIP, etc, together with
| > the main, layer 3 agnostic, DCCP core code, so that we have a faster
| > connection path by eliminating the need to always go thru the CCID
| > registration lock. But keep it there, so that we can experiment with
| > newer CCIDs without having to rebuild/reboot the whole kernel.
| > 
| > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
| 
| Gerrit, are you OK with this?
| 
Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

Yes absolutely and thanks a lot to Arnaldo for the quick reply.

I apologize being late in replying, I actually sat down after work
yesterday and formulated a reply which is in agreement with this code.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Thu, 18 Dec 2008 06:33:49 +0100

> | > Build CCID2, because the RFC requires it to be always available, and
> | > CCID3 too as it is the most interesting one for VoIP, etc, together with
> | > the main, layer 3 agnostic, DCCP core code, so that we have a faster
> | > connection path by eliminating the need to always go thru the CCID
> | > registration lock. But keep it there, so that we can experiment with
> | > newer CCIDs without having to rebuild/reboot the whole kernel.
> | > 
> | > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> | 
> | Gerrit, are you OK with this?
> | 
> Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

Applied, thanks everyone.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

| > | Gerrit, are you OK with this?
| > | 
| > Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
| 
| Applied, thanks everyone.
| 
I am sorry but I have to revert the Acked-by. As per
yesterday's email I was ok with the basic idea, but
already said that this did not include testing.

Please do not apply this patch yet, it introduces several
new problems and needs more work.

1. Module unloading does no longer work - it seems that
   there is an unmatched module_put somewhere.

2. There is an unresolved cyclic dependency with the
   dccp_tfrc_lib module: as soon as CCID-3 is enabled,
   - dccp.ko needs dccp_tfrc_lib.ko to resolve the
     dependencies of CCID-3
   - dccp_tfrc_lib.ko needs dccp.ko since it imports
     symbols of the main DCCP module.
   That is this also prohibits module unloading.

3. A third point why module unloading does not work is
   that there is no ccid_builtins_unregister() function
   called from dccp_fini() in net/dccp/proto.c.

4. This may be Arnaldo's intention, but I disagree wholly
   with it: why do we still have the module loading stuff
   in net/dccp/ccid.c? We currently have no other CCID 
   modules, and this just keeps the flaws of the old
   interface. When I (have to) fix up CCID-4 to work with
   the new interface then I will simply add a new entry 
   into the ccid_builtin_ops. Also -- is the r/w lock that
   was the cause for making this patch really still
   necessary in ccid.c?

5. (May be the cause for point (4)): we now have 3 arrays
   in ccid.c where a single one would fully do:
    
	struct ccid_operations *ccids[CCID_MAX];
	struct ccid_operations *builtin_ccids_ops[]
	u8 builtin_ccids[]

    Why do we keep the duplication between 'ccids' and
    'builtin_ccids_ops' instead of simply saying

      struct ccid_operations *ccids[CCID_MAX] = {
	  /* CCID-2 is supported by default */
	  [DCCPC_CCID2] = &ccid2_ops,	
      #ifdef CONFIG_IP_DCCP_CCID3
	  [DCCPC_CCID3] = &ccid3_ops,	
      #endif
      };

   In this manner we can do away with all the locking
   and loading overhead for non-builtin modules that
   do not even exist. Furthermore, the new routine
   is_builtin_ccid() is then also redundant.

   The second array redundancy is between builtin_ccids[]
   and builtin_ccids_ops[] - one can get the former via
   builtin_ccids_ops[index]->ccid_id.

   That is, I am asking to 
    * use 1 array instead of 3 that each do similar things
    * not make the complicated distinction between builtin
      and non-builtin (which at the moment is the same as
      non-existing)
    * as a result, several routines automatically fall
      under the table, the code becomes much simpler.
    

6. Suggestion: use '__init' annotation for 
   ccids_register_builtins()? (Since almost all routines in
   ccid.c start with ccid_xxx, would it also be more
   consistent to name the routine 'ccid_register_builtins')

7. When fixing the Kconfig dependency for IP_DCCP_TFRC_LIB 
   we need a bool instead of a tristate, e.g.
   
	config IP_DCCP_TFRC_LIB
		bool
		def_bool y if (IP_DCCP_CCID3 = y)

   And then the module_init/exit routines become unnecessary
   in net/dccp/ccids/lib/tfrc.c. When calling their replacement
   from ccid_builtins_register()/unregister() in net/dccp/ccid.c,
   we would need #ifdefs to avoid fusing the code to ccid3.c, e.g.

	int __init ccid_register_builtins(void)
	{
		int i, err;

	#ifdef CONFIG_IP_DCCP_TFRC_LIB
		err = tfrc_lib_init();
		if (err)
			return err;
	#endif
		for ($i = 0; i < ARRAY_SIZE(builtin_ccids_ops); i++) {
			// ...
		}
		return 0;

	unwind_registrations:
	#ifdef CONFIG_IP_DCCP_TFRC_LIB
		tfrc_lib_exit();
	#endif
		// ...
	}

   Linking the tfrc_lib code into ccid3.o seems not to solve the 
   problem since the init()/exit() routines of tfrc_lib are called
   only once instead of for each new socket, as in ccid3.c.

8. The 'extern struct ccid_operations ccid?_ops;' should be in
   ccid.h instead of ccid.c (found via sparse).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: Gerrit Renker <gerrit@erg.abdn.ac.uk>
Date: Fri, 19 Dec 2008 06:24:46 +0100

> | > | Gerrit, are you OK with this?
> | > | 
> | > Acked-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
> | 
> | Applied, thanks everyone.
> | 
> I am sorry but I have to revert the Acked-by. As per
> yesterday's email I was ok with the basic idea, but
> already said that this did not include testing.
> 
> Please do not apply this patch yet, it introduces several
> new problems and needs more work.

You are so incredibly lucky I didn't push this out to kernel.org yet,
it's been sitting in my tree all day long.

Don't say ACK unless you mean it.

It's reverted but you should do real reviews before you ACK changes in
the future.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

> You are so incredibly lucky I didn't push this out to kernel.org yet,
> it's been sitting in my tree all day long.
>
> Don't say ACK unless you mean it.
>
Yes for me that's a lesson learned.

I am waiting for Arnaldo's answer before taking any further steps;
the changes are a bit bigger than expected so that dividing into a
few smaller patches may be useful.

Thanks a lot
Gerrit

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

This set addresses the issues in making CCIDs an available part of DCCP.

I am sending this for discussion, it differs in one point from the previous
patch - this set does not keep the module loading and locking code.

The rationale for this is that
 * at the moment there are only 2 fully standardised CCIDs
 * this situation is not likely to change in the foreseeable
   future (RFC progress is very slow, the update of RFC 3448
   for instance took over 1 year and went through 6 revisions)
 * so it would only be research prototypes as candidates for
   the non-builtin case
 * and for this I think we are better off temporarily modifying
   the build process
 * this includes CCID-4, since its Internet draft has expired
   (http://www.ietf.org/internet-drafts/draft-ietf-dccp-ccid4-03.txt)
 * (but for heretics, we still have the "UDP-like" no-op module)

All patches apply on top of Arnaldo's (your) patch to simplify discussion.
Awaiting feedback, please do not apply immediately.

Patch #1: Removes CCID module references that were still around.
Patch #2: Removes the r/w locks around module loading.
Patch #3: Adds a missing ccid_unregister_builtins() function.
Patch #4: Integrates the TFRC library (which is a dependency of
          CCID-3/4) with DCCP, including necessary initialisation.

I have performed testing with this set yesterday, it worked very
well, including module unloading, use of dccp_probe, different links
(WiFi/Ethernet/loopback).

The set is also available via the test tree (git/patch/tarball):

	git://eden-feed.erg.abdn.ac.uk/dccp_exp	[subtree `dccp']
	http://  (same host)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Em Fri, Dec 19, 2008 at 07:56:25AM -0000, gerrit@erg.abdn.ac.uk escreveu:
> > You are so incredibly lucky I didn't push this out to kernel.org yet,
> > it's been sitting in my tree all day long.
> >
> > Don't say ACK unless you mean it.
> >
> Yes for me that's a lesson learned.
> 
> I am waiting for Arnaldo's answer before taking any further steps;
> the changes are a bit bigger than expected so that dividing into a
> few smaller patches may be useful.
> 
> Thanks a lot
> Gerrit
> 

I was away, travelling, will look at the points you raised tomorrow.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html