[PATCH 5/5]: Revert use of MSS instead of s

[PATCH 5/5]: Revert use of MSS instead of s

[Gerrit Renker]

[CCID3]: Revert use of MSS instead of s

This updates the CCID3 code with regard to two instances of using `MSS' in place of `s':

 1. The RFC3390-based initial rate. Both rfc3448bis as well as the Faster Restart
    draft now consistently use `s' instead of MSS. Swapping MSS for s had been done
    shortly after revision 01 of rfc3448bis, but that change subsequently disappeared.

 2. According to section 4.2 of rfc3448bis: "If the sender is ready to send data when
    it does not yet have a round trip sample, the value of X is set to s bytes per 
    second, for segment size s [...]"

Apart from tracking documents, this change makes sense: in particular, it avoids gross
over-estimation if the packet size `s' is small (imagine a VoIP application with sd
bytes and MSS according to Ethernet jumbo frames).

Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>
---
 net/dccp/ccids/ccid3.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -77,18 +77,20 @@ static void ccid3_hc_tx_set_state(struct
 }
 
 /*
- * Compute the initial sending rate X_init according to RFC 3390:
- *	w_init   =    min(4 * MSS, max(2 * MSS, 4380 bytes))
- *	X_init   =    w_init / RTT
+ * Compute the initial sending rate X_init in the manner of RFC 3390:
+ *
+ *	X_init  =  min(4*s, max(2*s, 4380 bytes)) / RTT
+ *
+ * Note that RFC 3390 uses MSS, while TFRC uses the nominal packet size s.
  * For consistency with other parts of the code, X_init is scaled by 2^6.
  */
 static inline u64 rfc3390_initial_rate(struct sock *sk)
 {
-	const struct dccp_sock *dp = dccp_sk(sk);
-	const __u32 w_init = min(4 * dp->dccps_mss_cache,
-				 max(2 * dp->dccps_mss_cache, 4380U));
+	const struct ccid3_hc_tx_sock *hctx = ccid3_hc_tx_sk(sk);
+	const __u32 w_init = min(4 * hctx->ccid3hctx_s,
+				 max(2 * hctx->ccid3hctx_s, 4380U));
 
-	return scaled_div(w_init << 6, ccid3_hc_tx_sk(sk)->ccid3hctx_rtt);
+	return scaled_div(w_init << 6, hctx->ccid3hctx_rtt);
 }
 
 /*
@@ -333,7 +335,7 @@ static int ccid3_hc_tx_send_packet(struc
 			hctx->ccid3hctx_t_ld = now;
 		} else {
 			/* Sender does not have RTT sample: X = MSS/second */
-			hctx->ccid3hctx_x = dp->dccps_mss_cache;
+			hctx->ccid3hctx_x = hctx->ccid3hctx_s;
 			hctx->ccid3hctx_x <<= 6;
 		}
 		ccid3_update_send_interval(hctx);

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Ian McDonald]

On 7/27/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID3]: Revert use of MSS instead of s
>
> This updates the CCID3 code with regard to two instances of using `MSS' in place of `s':
>
>  1. The RFC3390-based initial rate. Both rfc3448bis as well as the Faster Restart
>     draft now consistently use `s' instead of MSS. Swapping MSS for s had been done
>     shortly after revision 01 of rfc3448bis, but that change subsequently disappeared.
>
>  2. According to section 4.2 of rfc3448bis: "If the sender is ready to send data when
>     it does not yet have a round trip sample, the value of X is set to s bytes per
>     second, for segment size s [...]"
>
> Apart from tracking documents, this change makes sense: in particular, it avoids gross
> over-estimation if the packet size `s' is small (imagine a VoIP application with sd
> bytes and MSS according to Ethernet jumbo frames).
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

Hmm,

I want to think about this. In particular the reference to VoIP - go
look at CCID4 and that's EXACTLY what they do in effect (but standard
frames - not jumbo).

I'm heading down the patch that we should set 3448bis via a socket
option as well as faster restart but maybe that's a question we should
ask on the IETF list. It really depends on whether existing TFRC is
meant to be phased out and replaced or whether the application can
choose - like a lot of TCP enhancements e.g. SACK. I think 3448bis
should be the default but whether the only option is another
question...

Ian

-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Gerrit Renker]

|  I want to think about this. In particular the reference to VoIP - go
|  look at CCID4 and that's EXACTLY what they do in effect (but standard
|  frames - not jumbo).
 The reference to VoIP/jumbo frames is not relevant in this context, what is
relevant is thatthe code did not correspond to the current state of what 
several documents specify to use as default for `s'. That's enough justification
for me.

I don't understand the above comment about VoIP.

|  I'm heading down the patch that we should set 3448bis via a socket
|  option as well as faster restart but maybe that's a question we should
|  ask on the IETF list. 
If you want to do this and come back with some usable results to this list, feel free
to do this. I'd like to hear the results, but don't want to participate in IETF discussions.
I have lots of other work to do and am doing the DCCP/CCID3 work on top of it / aside
of it. That means for me `technical support yes' but `research discussions on the IETF
list (which is a specification list): no'. 

|  It really depends on whether existing TFRC is meant to be phased out and 
|  replaced or whether the application can choose - like a lot of TCP 
|  enhancements e.g. SACK. I think 3448bis should be the default but whether 
|  the only option is another question...
I disagree with the idea. It is already the case that the many documents (3448, 4340,
4342, rfc3448bis, Faster Restart draft) all make related, but in a subtle way different
interpretations of the same thing. I recall that when I did the patches, I would spend
reading up to two or three hours the different drafts, because they are all not really
consistent. What you are proposing is a parallel implementation of both the orthodox
RFC 4342/3448 if I understand you correctly. With that you only increase the number of
nightmares, since now you need not only align drafts, but also track sub-changes of 
different drafts in the code.

My suggestion is to keep the most common-sense collection of features as default and
to do everything else on a case-by-case or experimental basis. Already the code is not
really orthodox RFC 4342, we have for instance the Request/Response RTT sample which
is not part of the document, and there are several other features (can list them if
needed) that make the code not a true RFC 3448/4342 implementation.

We could fork subtrees for such features, how about this? That would keep defaults
and new features separate, at least for a while, and they can be integrated when the
specification gains maturity.


So what is your stance regarding the `s' vs `MSS' issue? This patch is according
to the book, I sent it to make your Faster Restart work a bit easier, and you
are saying you don't want it?

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Ian McDonald]

Hi there Gerrit,

Sorry for slow response. A bit behind on my emails at present.

On 7/28/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> |  I want to think about this. In particular the reference to VoIP - go
> |  look at CCID4 and that's EXACTLY what they do in effect (but standard
> |  frames - not jumbo).
>  The reference to VoIP/jumbo frames is not relevant in this context, what is
> relevant is thatthe code did not correspond to the current state of what
> several documents specify to use as default for `s'. That's enough justification
> for me.

Now I've read more my comment was wrong. I've gone back and found that
MSS changed to s in bis/FR arising out of Arjuna's comments. It was
MSS though in 4342. I never picked this up earlier.
>
> I don't understand the above comment about VoIP.

Ignore it.
>
> |  I'm heading down the patch that we should set 3448bis via a socket
> |  option as well as faster restart but maybe that's a question we should
> |  ask on the IETF list.
> If you want to do this and come back with some usable results to this list, feel free
> to do this. I'd like to hear the results, but don't want to participate in IETF discussions.
> I have lots of other work to do and am doing the DCCP/CCID3 work on top of it / aside
> of it. That means for me `technical support yes' but `research discussions on the IETF
> list (which is a specification list): no'.
>
Understand what you are saying.

> |  It really depends on whether existing TFRC is meant to be phased out and
> |  replaced or whether the application can choose - like a lot of TCP
> |  enhancements e.g. SACK. I think 3448bis should be the default but whether
> |  the only option is another question...
> I disagree with the idea. It is already the case that the many documents (3448, 4340,
> 4342, rfc3448bis, Faster Restart draft) all make related, but in a subtle way different
> interpretations of the same thing. I recall that when I did the patches, I would spend
> reading up to two or three hours the different drafts, because they are all not really
> consistent. What you are proposing is a parallel implementation of both the orthodox
> RFC 4342/3448 if I understand you correctly. With that you only increase the number of
> nightmares, since now you need not only align drafts, but also track sub-changes of
> different drafts in the code.
>
OK. Herein lies my problem. For my research I want to see what
improvements Faster Restart make to performance of my tests. However
as you point out it is quite complicated. I absolutely don't think we
should be tracking differences between 3448 (original) and 4342 - it
is clear 4342 takes precedence. If we implement drafts we need to
update with any changes but we don't need to keep past history there -
only latest version of draft. For me personally I need to track Faster
Restart. Where it's becoming an issue for me though is that there is
overlap between FR and 3448bis. It's causing me a bit of grief to be
honest.

Now as what is best for the Linux kernel - as opposed to what is best
for me, which is not the same thing :-(. I believe 3448bis changes
should be brought into code without having option to enable/disable -
much like Reno in the kernel is really NewReno. I think Faster Restart
should be an option though much like SACK, ABC, FRTO are in the
kernel.

> My suggestion is to keep the most common-sense collection of features as default and
> to do everything else on a case-by-case or experimental basis. Already the code is not
> really orthodox RFC 4342, we have for instance the Request/Response RTT sample which
> is not part of the document, and there are several other features (can list them if
> needed) that make the code not a true RFC 3448/4342 implementation.
>
If you do have a list of them it would be great. I see quite a few
comments about bis in the code.

> We could fork subtrees for such features, how about this? That would keep defaults
> and new features separate, at least for a while, and they can be integrated when the
> specification gains maturity.
>
I'm against forking trees if at all possible as it makes tracking
changes a nightmare. However if my patches are purely experimental,
and won't ever make it into mainline (e.g. my send best packet next)
I'll keep them as a separate patch series which can be a separate
branch if needed/wanted.
>
> So what is your stance regarding the `s' vs `MSS' issue? This patch is according
> to the book, I sent it to make your Faster Restart work a bit easier, and you
> are saying you don't want it?
>
I do want the s and I want the MSS as outlined earlier. However your
patch is valid and it's up to me to patch to allow both for my
research!

Thanks,

Ian
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Ian McDonald]

On 7/27/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID3]: Revert use of MSS instead of s
>
> Signed-off-by: Gerrit Renker <gerrit@erg.abdn.ac.uk>

Signed-off-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
>  /*
> - * Compute the initial sending rate X_init according to RFC 3390:
> - *     w_init   =    min(4 * MSS, max(2 * MSS, 4380 bytes))
> - *     X_init   =    w_init / RTT
> + * Compute the initial sending rate X_init in the manner of RFC 3390:
> + *
> + *     X_init  =  min(4*s, max(2*s, 4380 bytes)) / RTT
> + *
> + * Note that RFC 3390 uses MSS, while TFRC uses the nominal packet size s.

Can you perhaps change this to say something like....
* Note that RFC 3390 uses MSS, while TFRC uses the nominal packet size
s and TRFCbis clarifies this to the above formula.



-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Gerrit Renker]

Quoting Ian McDonald:
|  > |  It really depends on whether existing TFRC is meant to be phased out and
|  > |  replaced or whether the application can choose - like a lot of TCP
|  > |  enhancements e.g. SACK. I think 3448bis should be the default but whether
|  > |  the only option is another question...
|  > I disagree with the idea. It is already the case that the many documents (3448, 4340,
|  > 4342, rfc3448bis, Faster Restart draft) all make related, but in a subtle way different
|  > interpretations of the same thing. I recall that when I did the patches, I would spend
|  > reading up to two or three hours the different drafts, because they are all not really
|  > consistent. What you are proposing is a parallel implementation of both the orthodox
|  > RFC 4342/3448 if I understand you correctly. With that you only increase the number of
|  > nightmares, since now you need not only align drafts, but also track sub-changes of
|  > different drafts in the code.
|  >
|  OK. Herein lies my problem. For my research I want to see what
|  improvements Faster Restart make to performance of my tests. However
|  as you point out it is quite complicated. I absolutely don't think we
|  should be tracking differences between 3448 (original) and 4342 - it
|  is clear 4342 takes precedence. If we implement drafts we need to
|  update with any changes but we don't need to keep past history there -
|  only latest version of draft. For me personally I need to track Faster
|  Restart. Where it's becoming an issue for me though is that there is
|  overlap between FR and 3448bis. It's causing me a bit of grief to be
|  honest.
Can understand that - that is partly why I suggested to wait a little, and see if and the
two drafts stabilise. If there were updates to 4342 then I would also be all for tracking
them. However, 4342 is written in such a clever way that updates of 3448 do not apply.
I am in support of the 3448 draft changes that are in the kernel so far, since to my
understanding they improve the performance over an `orthodox' 4342; the point being that
no one cares how orthodox the implementation is when the performance is awful.
  
|  I believe 3448bis changes should be brought into code without having option to 
|  enable/disable -  much like Reno in the kernel is really NewReno. 
Yes, agree fully with that point. Am not fully sure about the status of 3448bis, during
the past 5 months there have been heavy, frequent and also retracted changes.

|  I think Faster Restart should be an option though much like SACK, ABC, FRTO are in the
|  kernel.
Makes sense. I thought your concept of using a socket option was a good one, since it
could be reused later for CCID4 (it is not a single-CCID socket option).

|  > Already the code is not really orthodox RFC 4342, we have for instance the 
|  > Request/Response RTT sample which is not part of the document, and there are several 
|  > other features (can list them if > needed) 
|  If you do have a list of them it would be great. I see quite a few
|  comments about bis in the code.
I don't have a list at present, need to go through the works. Will post at a later point.

 
|  > We could fork subtrees for such features, how about this? That would keep defaults
|  > and new features separate, at least for a while, and they can be integrated when the
|  > specification gains maturity.
|  >
|  I'm against forking trees if at all possible as it makes tracking
|  changes a nightmare. However if my patches are purely experimental,
|  and won't ever make it into mainline (e.g. my send best packet next)
|  I'll keep them as a separate patch series which can be a separate
|  branch if needed/wanted.
Forking trees is actually not so bad with regard to tracking the changes. I am doing that
frequently for the test tree when the netdev tree changes. If there are not too many subtrees,
(more than a handful), I'd say that this is feasible. It also makes it easier to track what
is going on, and can be used as a temporary workspace while patch sets are under discussion.

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Ian McDonald]

On 8/2/07, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Can understand that - that is partly why I suggested to wait a little, and see if and the
> two drafts stabilise. If there were updates to 4342 then I would also be all for tracking
> them. However, 4342 is written in such a clever way that updates of 3448 do not apply.
> I am in support of the 3448 draft changes that are in the kernel so far, since to my
> understanding they improve the performance over an `orthodox' 4342; the point being that
> no one cares how orthodox the implementation is when the performance is awful.
>
Unfortunately I can't really wait too long as it is for my thesis.
That's OK as I can reference which draft I use. It has been suggested
for CCID3.bis (and agreed) that CCID3 should track TFRC.

> |  If you do have a list of them it would be great. I see quite a few
> |  comments about bis in the code.
> I don't have a list at present, need to go through the works. Will post at a later point.
>
Don't worry too much about this, as this is my research so don't want
to waste your time :-)

> Forking trees is actually not so bad with regard to tracking the changes. I am doing that
> frequently for the test tree when the netdev tree changes. If there are not too many subtrees,
> (more than a handful), I'd say that this is feasible. It also makes it easier to track what
> is going on, and can be used as a temporary workspace while patch sets are under discussion.
>
Yes, that's what I do in effect - I work off your tree and occasionally resync.

Ian
-- 
Web1: http://wand.net.nz/~iam4/
Web2: http://www.jandi.co.nz
Blog: http://iansblog.jandi.co.nz

Re: [PATCH 5/5]: Revert use of MSS instead of s

[Gerrit Renker]

Quoting Ian McDonald:
|  > Can understand that - that is partly why I suggested to wait a little, and see if and the
|  > two drafts stabilise. If there were updates to 4342 then I would also be all for tracking
|  > them. However, 4342 is written in such a clever way that updates of 3448 do not apply.
|  > I am in support of the 3448 draft changes that are in the kernel so far, since to my
|  > understanding they improve the performance over an `orthodox' 4342; the point being that
|  > no one cares how orthodox the implementation is when the performance is awful.
|  >
|  Unfortunately I can't really wait too long as it is for my thesis.
|  That's OK as I can reference which draft I use. It has been suggested
|  for CCID3.bis (and agreed) that CCID3 should track TFRC.

I have enquired about the status of rfc3448bis - the situation at the moment seems that there
are a lot of changes in rfc3448bis which have not gone through community review yet; there are
further edits/changes which need to be addressed (probably in the next revision for the December
IETF meeting); and only after this has been done can some stabilisation be expected. Until
then there are probably going to be many changes. It seems that Faster Restart is being rewritten
into a new algorithm, using something like TCP cwnd validation.

For me this all looks very experimental, but it can be useful to experiment. Therefore, if you
wanted to, the offer is there to put all this stuff as a fork of the test tree.
 
|  > |  If you do have a list of them it would be great. I see quite a few
|  > |  comments about bis in the code.
|  > I don't have a list at present, need to go through the works. Will post at a later point.
|  >
|  Don't worry too much about this, as this is my research so don't want
|  to waste your time :-)
Thanks - much appreciated, this can take a little time.