: Patch Preview

I have put quite a lot of work into
* bringing CCID 3 up to spec
* implement loss detection according to RFC 3448/4342
* add loss intervals handling according to rfc3448bis
* many cool new features
* ample documentation, applications, and test modules for most features

However, since patches are small for review-ability, this amounts to a total of 75 patches, including the patch backlog; please see suggestion (2).

1. PREVIEW: Since mostly the following files are modified:

      net/dccp/ccids/lib/ccid3.*
      net/dccp/ccids/lib/loss_interval.*
      net/dccp/ccids/lib/packet_history.*

I have posted combined patches onto

http://www.erg.abdn.ac.uk/users/gerrit/dccp/patches_working-set/

These apply in-order (see series file) onto today's tree. They have been extensively tested, can offer a test module for most essential features.

2. SUGGESTION: Since the patch backlog is really quite old now (over 2 months), I would like to suggest to re-submit it as one, combined patch, in the format of
http://www.erg.abdn.ac.uk/users/gerrit/dccp/patches_working-set/01_The_Patch_Backlog.diff
(maybe a bit more verbose)
=> But this depends on whether people are ok with this (see (3))

3. MISSED? I am not sure if I have missed Sign-offs or Acked-byes. Ian (and others) can you please check that http://www.erg.abdn.ac.uk/users/gerrit/dccp/patch-backlog/
is correct? In particular, only the following two out of 25 are not acked/signed-off:
* http://www.erg.abdn.ac.uk/users/gerrit/dccp/patch-backlog/3e_CCID3_send_time_race_condition.diff
(but I believe you said you were ok with it, since the patch was changed to update t_ipi when `s' changes)
* http://www.erg.abdn.ac.uk/users/gerrit/dccp/patch-backlog/4a_debug_always_no_module.diff

4. SUMMARY of patch sets 2 and 3:

* avoiding accumulation of sending credits
* faster & more efficient time-stamping (timestamps from NIC)
* honour initial (SYN handshake) RTT estimate
* implement rfc3448bis nofeedback timer handling
- ignore feedback after nofeedback timer expiry when p=0
- do not reduce X after nofeedback timer expiry
* implement larger initial windows as described in rfc3448bis
* resolve locking issues for SMP
- atomic context between sending packet & updating CCID structures
- atomic context when updating t_nom
- protect transmitter data structures against asynchronous changes
- keep garbage collection separate from access

* clean up and minimize socket data structures
* allow retrieving current Maximum Packet Size via socket option
* compilation fixes for 64-bit architectures
* function for distinguishing data' and non-data' (NDP) packet types
* routines to perform modulo-16 (window counter CCval) arithmetic
* simplify the kernel API (split functionality into modules)
* completely revised RX history structure
- circular list using only minimum of entries
- object-oriented programming interface (like abstract data type)
- reusing the same structure for both RTT sampling and loss detection
- generic service, available to other CCIDs
* loss detection
- cope with delayed, late, and re-ordered packets
- detect when a sequence hole is later filled
- take NDP information into account
- assign losses of the same RTT to the same loss event
- recycle history structures after loss detection (cope with loss bursts)

* loss interval management
- also ring list since simplifies interface
- object-oriented programming interface (like abstract data type)
- on-demand allocation: less memory consumption on loss-free connections
- handing over from loss detection to loss interval history completely automated by module (reusable for future CCIDs)

Enjoy,
Gerrit

No, what I think is the best to do is to consider your patch backlog as a separate git tree that now will be pulled, preserving the changesets history.

- Arnaldo

Gerrit,

This sounds great. Well done!

I'm not so confident when it comes to the debugging code which is why I didn't comment. One thing is I notice this code which looks like it could be useful if ported to DCCP:
http://master.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.22.git;a=commit;hŽ2101a9faab43dcf307d46784a1ede7997fe729

This allows turning debug code on/off by sysctl.

The new patches sound great but I find them hard to review all combined. Arnaldo has asked for a git tree I see. Can that be provided, or I prefer your directory structure of patches so I can read though them? You implied that you had done this for new patches but I can't actually find it.

Again - this looks like a great step forward.

Ian

Web: http://wand.net.nz/~iam4
Blog: http://iansblog.jandi.co.nz
WAND Network Research Group

No need for a git tree, I can apply it one by one here,what I said is that what gerryt has is equivalent to a git tree, i.e. a series of patches with comments and I don't want to lose that comments neither the separation.

Will submit some more csets I have here for the skb layer header pointers series and merge this after a quick review (famous last words, lets see if its true today :-) ).

- Arnaldo

I can see a set of patches for the first lot, but not the second two lots...

Ian

Web: http://wand.net.nz/~iam4
Blog: http://iansblog.jandi.co.nz
WAND Network Research Group

I started merging some, to speed up the process I'll accept some even with reservations on some aspects of it, such as:

1a_simplify_seqno_arithmetic.diff:

- use of all upercase (ADD48, etc)
- DCCP_MAX_SEQNO should really be something like whats in include/linux/kernel.h:

#define INT_MAX         ((int)(~0U>>1))
#define INT_MIN         (-INT_MAX - 1)
#define UINT_MAX        (~0U)
#define LONG_MAX        ((long)(~0UL>>1))
#define LONG_MIN        (-LONG_MAX - 1)
#define ULONG_MAX       (~0UL)
#define LLONG_MAX       ((long long)(~0ULL>>1))
#define LLONG_MIN       (-LLONG_MAX - 1)
#define ULLONG_MAX      (~0ULL)

But named UINT48_MAX or something in this direction, In fact I don't see why these things are in kernel.h, they probably would be better of in limits.h, like in userland.

1e_dccp_inc_seqno-no-pointer-variant.diff:

What is the point? INC48() is introduced but not used anywhere besides in dccp_inc_seqno, where there is no changes with this patch, does the other patches after this series use it?

The others in the 1e series looks OK apart from the issues mentioned above, will try and continue tomorrow

Thanks a lot!

- Arnaldo

|  I started merging some, to speed up the process I'll accept some even
|  with reservations on some aspects of it, such as:

Please do let me know those reservations. I'd rather revise a patch several times than submitting something which is only half as good as could be.

|  1a_simplify_seqno_arithmetic.diff:
|  
|  - use of all upercase (ADD48, etc)
|  - DCCP_MAX_SEQNO should really be something like whats in
|  include/linux/kernel.h:
|  
|  #define INT_MAX         ((int)(~0U>>1))
|  #define INT_MIN         (-INT_MAX - 1)
|  #define UINT_MAX        (~0U)
|  #define LONG_MAX        ((long)(~0UL>>1))
|  #define LONG_MIN        (-LONG_MAX - 1)
|  #define ULONG_MAX       (~0UL)
|  #define LLONG_MAX       ((long long)(~0ULL>>1))
|  #define LLONG_MIN       (-LLONG_MAX - 1)
|  #define ULLONG_MAX      (~0ULL)
|  
|  But named UINT48_MAX or something in this direction, In fact I don't
|  see why these things are in kernel.h, they probably would be better of
|  in limits.h, like in userland.

I fully agree, will re-submit a v2 later on. If you don't mind, I'd prefer to keep the long (LL) hex constants in the definition of INT48_MIN/UINT48_MAX.

|  1e_dccp_inc_seqno-no-pointer-variant.diff:
|  
|  What is the point? INC48() is introduced but not used anywhere besides
|  in dccp_inc_seqno, where there is no changes with this patch, does the
|  other patches after this series use it?

The purpose of the INC48() macro is to increment bitfield u64 counters (those which were declared as u64 ccidvariable:48, ... ). The macro is not currently used by the code, it had initially been used for a piece of code Ian wrote. Since no other patches are currently depend on it, and since there are already the ADD48/SUB48 macros, I have dropped this patch - hope you are happy with this.

(Btw, follows48 (of patch 1d) is also no longer used and could be dropped,
but this can only be done when all other patches are applied).

Many thanks for looking into this and for feedback Gerrit

|  I'm not so confident when it comes to the debugging code which is why
|  I didn't comment. One thing is I notice this code which looks like it
|  could be useful if ported to DCCP:
|  http://master.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.22.git;a=commit;h�2101a9faab43dcf307d46784a1ede7997fe729
|  
|  This allows turning debug code on/off by sysctl.

Good idea, but I think that makes things unnecessarily complex. We would need
* a sysctl subtree of net.dccp for each module wishing to debug something and
* a sysctl variable for each such module
That makes the code much more complex. What this patch does is merely to switch

"debugging is always turned on when DCCP and its modules
are built statically into the kernel"

off, for the reasons that
(a) someone who builds DCCP statically into the kernel is most likely not interested in developing and/or debugging the DCCP module and
(b) will not much appreciate his syslogs being flooded by debugging messages turned on by default.

But as said, I have no strong opinions about this patch.

|  The new patches sound great but I find them hard to review all
|  combined. Arnaldo has asked for a git tree I see. Can that be
|  provided, or I prefer your directory structure of patches so I can
|  read though them? You implied that you had done this for new patches
|  but I can't actually find it.

No they are not meant for review. The sole purpose is, since the number of individual patches is quite high, to get an overall picture. Patches will be submitted as soon as there is time. I also have a lot of documentation that goes with it, will upload that very soon - this allows also to understand the details better. And documentation is the most important thing here - I have spent a lot of time just with a sheet of
paper: CCID3 is so complicated that without a diagram or a flowchart one gets very very easily lost.
-
To unsubscribe from this list: send the line "unsubscribe dccp" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html