[PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Gerrit Renker]

This is a small set of patches, likely the last for this year.
I have run my whitespace test script over them and gave them a
test run (sorry, the sparc64 is not yet up).

Patch 1: Adds the add48/sub48 functions to dccp.h. This was
         inspired by Ian McDonald

Patch 2: Changes dccp_delta_seqno(a, b) to return signed numbers,
         depending on whether a sequence number a is `before' b or not

Patch 3: Makes `before48' unambiguous, following the previous discussions.
         I have kept Arnaldo's basic concept, only the subtraction and
         comparison changes. Thanks to Eddie for discussions. 

Patch 4: Fixes a problem with follows48, solved as a byproduct of dccp_delta_seqno

Patch 5: Moves ccid3_pr_debug into header file so that the macro can be used by
         other files. Thanks to Ian McDonald who raised this.

Patch 6: Removes a now redundant `len' test in ccid3_hc_tx_update_s

Patch 7: Re-orders ccid3.c in a TX/RX section, following the ordering in packet_history.{c,h}

Patch 8: Now uses MSS instead of `s' for larger windows -- and now finally also larger throughput :)

Re: [PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Ian McDonald]

On 12/15/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> This is a small set of patches, likely the last for this year.
> I have run my whitespace test script over them and gave them a
> test run (sorry, the sparc64 is not yet up).
>
These look good. I presume that they are unfortunately 2.6.21 material
now :-( At least Gerrit has them as a patch set up. I'll go rework my
other patches now when I get a chance.

The only change (at present) I know that hasn't made it up to Dave M
for 2.6.20 would be this trivial one of mine:
http://www.mail-archive.com/dccp@vger.kernel.org/msg01150.html

I presume this will get pushed to him at some stage.

Ian
-- 
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Re: [PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Arnaldo Carvalho de Melo]

On 12/15/06, Ian McDonald <ian.mcdonald@jandi.co.nz> wrote:
> On 12/15/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> > This is a small set of patches, likely the last for this year.
> > I have run my whitespace test script over them and gave them a
> > test run (sorry, the sparc64 is not yet up).
> >
> These look good. I presume that they are unfortunately 2.6.21 material
> now :-( At least Gerrit has them as a patch set up. I'll go rework my
> other patches now when I get a chance.
>
> The only change (at present) I know that hasn't made it up to Dave M
> for 2.6.20 would be this trivial one of mine:
> http://www.mail-archive.com/dccp@vger.kernel.org/msg01150.html
>
> I presume this will get pushed to him at some stage.

Well, I guess it doesn't matter at what stage things are submitted for
inclusion as long as the proposed changes are pure bugfixes, and as
DCCP is in such a experimental stage it may well be the case that
Gerrit's changes can be classified as such and submitted for 2.6.20.

Tomorrow I'll try to review this patch series and take into account
your opinion when deciding which ones are pure bug fixes worthy for
pushing to DaveM and then to Linus.

Thanks a lot for your work.

- Arnaldo

Re: [PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Ian McDonald]

On 12/16/06, Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote:
> Well, I guess it doesn't matter at what stage things are submitted for
> inclusion as long as the proposed changes are pure bugfixes, and as
> DCCP is in such a experimental stage it may well be the case that
> Gerrit's changes can be classified as such and submitted for 2.6.20.
>
> Tomorrow I'll try to review this patch series and take into account
> your opinion when deciding which ones are pure bug fixes worthy for
> pushing to DaveM and then to Linus.
>
> Thanks a lot for your work.
>
> - Arnaldo
>
That's good news then. On that basis I'd say everything except 5 and 7
are 2.6.20 material.

Ian
-- 
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Re: [PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Gerrit Renker]

Hi Arnaldo / Ian
|  Tomorrow I'll try to review this patch series and take into account
|  your opinion when deciding which ones are pure bug fixes worthy for
|  pushing to DaveM and then to Linus.
I am not fussy about getting the patches into 2.6.20 at all cost. I am reconsidering
the add48/sub48 and the dccp_inc_seqno functions/macros.

The reason is something which I initially didn't consider: Ian reminded me that the
address of a bitfield (e.g. ccid3hcrx_seqno_nonloss). This means that dccp_inc_seqno
is of little use for this purpose - and so are, likely, the add48/sub48 functions.
Please see below for a suggestion of an _inc48() macro which would work in this case.

Since I expect that the sub48/add48 functions will be needed mostly for bitfields and
not for 64-bit values of DCCP internal structures, it may be good to leave those patches
`on ice' and give Ian's patch the preference.

Ian I would appreciate input on how and where such subtraction/addition functions shall
be used. Maybe it is better to just just a set of macros, ditch the sub48/add48 functions.

Gerrit

+++ b/net/dccp/dccp.h
@@ -102,6 +102,7 @@ extern int  sysctl_dccp_tx_qlen;
 #define TO_UNSIGNED48(x) (((x) >= 0)?	       (x) :  COMPLEMENT48(-(x)))
 #define _add48(a, b)	 (((a) + (b)) & DCCP_MAX_SEQNO)
 #define _sub48(a, b)	 _add48((a), COMPLEMENT48((b)))
+#define _inc48(seqno)	 (seqno) = _add48((seqno), 1)
 
 static inline void dccp_set_seqno(u64 *seqno, u64 value)
 {
--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -981,10 +981,8 @@ static int ccid3_hc_rx_detect_loss(struc
 		loss = 1;
 		ccid3_hc_rx_update_li(sk, hcrx->ccid3hcrx_seqno_nonloss,
 		   hcrx->ccid3hcrx_ccval_nonloss);
+		_inc48(hcrx->ccid3hcrx_seqno_nonloss);
 		tmp_seqno = hcrx->ccid3hcrx_seqno_nonloss;
-		dccp_inc_seqno(&tmp_seqno);
-		hcrx->ccid3hcrx_seqno_nonloss = tmp_seqno;
-		dccp_inc_seqno(&tmp_seqno);
 		while (dccp_rx_hist_find_entry(&hcrx->ccid3hcrx_hist,
 		   tmp_seqno, &ccval)) {
 			hcrx->ccid3hcrx_seqno_nonloss = tmp_seqno;

Re: [PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Ian McDonald]

On 12/19/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> I am not fussy about getting the patches into 2.6.20 at all cost. I am reconsidering
> the add48/sub48 and the dccp_inc_seqno functions/macros.
>
Gerrit,

I think leave your code in as it covered more cases than mine and it
works - I've been doing quite a bit of testing of CCID3 and I'm happy
with your code as per your patch series.

Ian
-- 
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Re: [PATCH 0/8]: Sequence numbers, larger windows revisited, minor cleanups

[Gerrit Renker]

Thank you - but what I actually wanted/needed to know is:

 * OK to ditch the add48/sub48 functions (which take a pointer to u64 and thus are useless on 48:-bit fields)?
 * OK to instead use the following macros instead?
#define add48(seqno, b)     	seqno = (seqno + (b)) & DCCP_MAX_SEQNO
#define sub48(seqno, b) 	add48(seqno, COMPLEMENT48((b)))
#define inc48(seqno)		add48(seqno, 1)

Reasons: 
	1) this kind of sequence arithmetic seems unlikely to be used for main DCCP
           data structures which all use u64 to store sequence numbers (e.g. all places 
           where dccp_inc_seqno is used)
	2) ccid3 has very long names, it makes things simpler to put assignent into the macro
	3) with a function this kind of assignment is not possible, and the operation is actually simple

Gerrit

Quoting Ian McDonald:
|  On 12/19/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
|  > I am not fussy about getting the patches into 2.6.20 at all cost. I am reconsidering
|  > the add48/sub48 and the dccp_inc_seqno functions/macros.
|  >
|  Gerrit,
|  
|  I think leave your code in as it covered more cases than mine and it
|  works - I've been doing quite a bit of testing of CCID3 and I'm happy
|  with your code as per your patch series.
|  
|  Ian