[PATCH] BUG-FIX - conversion errors

[PATCH] BUG-FIX - conversion errors

[Gerrit Renker]

Sorry if this comes late. By checking on values, I noted that values were
not shifted properly since the typecast from u32 to __u64 was not performed
properly. Fixed by explicitly casting each type. 
(uploaded as number 17_CCID3_fix-scaling-type-cast.diff)

----------------------------> Patch <--------------------------------------------

[CCID3]: BUG-FIX - conversion errors

This fixes conversion errors which arose by not properly type-casting
from u32 to __u64. Fixed by explicitly casting each type which is not
__u64, or by performing operation after assignment.

The patch further adds missing debug information to track the current
value of X_recv.

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

--- a/net/dccp/ccids/ccid3.c
+++ b/net/dccp/ccids/ccid3.c
@@ -123,18 +123,19 @@ static void ccid3_hc_tx_update_x(struct 
 
 	if (hctx->ccid3hctx_p > 0) {
 
-		hctx->ccid3hctx_x = min_t(u64, hctx->ccid3hctx_x_calc << 6,
-					       hctx->ccid3hctx_x_recv * 2  );
-		hctx->ccid3hctx_x = max_t(u64, hctx->ccid3hctx_x,
-					  (hctx->ccid3hctx_s << 6)/TFRC_T_MBI);
+		hctx->ccid3hctx_x = min(((__u64)hctx->ccid3hctx_x_calc) << 6,
+					hctx->ccid3hctx_x_recv * 2           );
+		hctx->ccid3hctx_x = max(hctx->ccid3hctx_x,
+					(((__u64)hctx->ccid3hctx_s) << 6) /
+					 			   TFRC_T_MBI);
 
 	} else if (timeval_delta(now, &hctx->ccid3hctx_t_ld) -
 			(suseconds_t)hctx->ccid3hctx_rtt >= 0 ) {
 
-		hctx->ccid3hctx_x = max(2 * min(hctx->ccid3hctx_x,
-						hctx->ccid3hctx_x_recv),
-					scaled_div(hctx->ccid3hctx_s << 6,
-						   hctx->ccid3hctx_rtt    ));
+		hctx->ccid3hctx_x +			max(2 * min(hctx->ccid3hctx_x, hctx->ccid3hctx_x_recv),
+			    scaled_div(((__u64)hctx->ccid3hctx_s) << 6,
+				       hctx->ccid3hctx_rtt             )      );
 		hctx->ccid3hctx_t_ld = *now;
 	}
 
@@ -207,8 +208,9 @@ static void ccid3_hc_tx_no_feedback_time
 	switch (hctx->ccid3hctx_state) {
 	case TFRC_SSTATE_NO_FBACK:
 		/* RFC 3448, 4.4: Halve send rate directly */
-		hctx->ccid3hctx_x = max_t(u32, hctx->ccid3hctx_x / 2,
-					  (hctx->ccid3hctx_s << 6)/TFRC_T_MBI);
+		hctx->ccid3hctx_x = max(hctx->ccid3hctx_x / 2,
+					(((__u64)hctx->ccid3hctx_s) << 6) /
+								    TFRC_T_MBI);
 
 		ccid3_pr_debug("%s(%p, state=%s), updated tx rate to %u "
 			       "bytes/s\n", dccp_role(sk), sk,
@@ -226,7 +228,7 @@ static void ccid3_hc_tx_no_feedback_time
 		 */
 		if (!hctx->ccid3hctx_idle ||
 		    (hctx->ccid3hctx_x_recv >= 4 *
-		     scaled_div(hctx->ccid3hctx_s << 6, hctx->ccid3hctx_rtt))) {
+		     scaled_div(((__u64)hctx->ccid3hctx_s) << 6, hctx->ccid3hctx_rtt))) {
 			struct timeval now;
 
 			ccid3_pr_debug("%s(%p, state=%s), not idle\n",
@@ -249,15 +251,16 @@ static void ccid3_hc_tx_no_feedback_time
 			    hctx->ccid3hctx_x_calc > (hctx->ccid3hctx_x_recv >> 5))  {
 
 				hctx->ccid3hctx_x_recv -					max_t(u64, hctx->ccid3hctx_x_recv / 2,
-					      	  (hctx->ccid3hctx_s << 6) /
+					max(hctx->ccid3hctx_x_recv / 2,
+					    (((__u64)hctx->ccid3hctx_s) << 6) /
 					      			(2*TFRC_T_MBI));
 
 				if (hctx->ccid3hctx_p = 0)
 					dccp_timestamp(sk, &now);
-			} else
-				hctx->ccid3hctx_x_recv = hctx->ccid3hctx_x_calc << 4;
-
+			} else {
+				hctx->ccid3hctx_x_recv = hctx->ccid3hctx_x_calc;
+				hctx->ccid3hctx_x_recv <<= 4;
+			}
 			/* Now recalculate X [RFC 3448, 4.3, step (4)] */
 			ccid3_hc_tx_update_x(sk, &now);
 		}
@@ -320,7 +323,8 @@ static int ccid3_hc_tx_send_packet(struc
 
 		/* Set initial sending rate X/s to 1pps (X is scaled by 2^6) */
 		ccid3_hc_tx_update_s(hctx, skb->len);
-		hctx->ccid3hctx_x = hctx->ccid3hctx_s << 6;
+		hctx->ccid3hctx_x = hctx->ccid3hctx_s;
+		hctx->ccid3hctx_x <<= 6;
 
 		/* First timeout, according to [RFC 3448, 4.2], is 1 second */
 		hctx->ccid3hctx_t_ipi = USEC_PER_SEC;
@@ -421,7 +425,8 @@ static void ccid3_hc_tx_packet_recv(stru
 		}
 
 		/* Update receive rate in units of 64 * bytes/second */
-		hctx->ccid3hctx_x_recv = opt_recv->ccid3or_receive_rate << 6;
+		hctx->ccid3hctx_x_recv = opt_recv->ccid3or_receive_rate;
+		hctx->ccid3hctx_x_recv <<= 6;
 
 		/* Update loss event rate */
 		pinv = opt_recv->ccid3or_loss_event_rate;
@@ -460,10 +465,10 @@ static void ccid3_hc_tx_packet_recv(stru
 			 * Larger Initial Windows [RFC 4342, sec. 5]
 			 * We deviate in that we use `s' instead of `MSS'.
 			 */
-			u16 w_init = min(    4 * hctx->ccid3hctx_s,
-					 max(2 * hctx->ccid3hctx_s, 4380));
+			__u64 w_init = min(    4 * hctx->ccid3hctx_s,
+					   max(2 * hctx->ccid3hctx_s, 4380));
 			hctx->ccid3hctx_rtt  = r_sample;
-			hctx->ccid3hctx_x    = scaled_div(w_init<< 6, r_sample);
+			hctx->ccid3hctx_x    = scaled_div(w_init << 6, r_sample);
 			hctx->ccid3hctx_t_ld = now;
 
 			ccid3_update_send_time(hctx);
@@ -487,11 +492,12 @@ static void ccid3_hc_tx_packet_recv(stru
 			ccid3_hc_tx_update_x(sk, &now);
 
 			ccid3_pr_debug("%s(%p), RTT=%uus (sample=%ldus), s=%u, "
-				       "p=%u, X_calc=%u, X=%u\n", dccp_role(sk),
+				       "p=%u, X_calc=%u, X_recv=%u, X=%u\n", dccp_role(sk),
 				       sk, hctx->ccid3hctx_rtt, r_sample,
 				       hctx->ccid3hctx_s, hctx->ccid3hctx_p,
 				       hctx->ccid3hctx_x_calc,
-				       (unsigned)(hctx->ccid3hctx_x >> 6));
+				       (unsigned)(hctx->ccid3hctx_x_recv >> 6),
+				       (unsigned)(hctx->ccid3hctx_x >> 6)     );
 		}
 
 		/* unschedule no feedback timer */

Re: [PATCH] BUG-FIX - conversion errors

[Ian McDonald]

On 12/9/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID3]: BUG-FIX - conversion errors
Acked-by: Ian McDonald <ian.mcdonald@jandi.co.nz>
-- 
Web: http://wand.net.nz/~iam4
Blog: http://imcdnzl.blogspot.com
WAND Network Research Group

Re: [PATCH] BUG-FIX - conversion errors

[Ian McDonald]

On 12/9/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> [CCID3]: BUG-FIX - conversion errors
> -                       u16 w_init = min(    4 * hctx->ccid3hctx_s,
> -                                        max(2 * hctx->ccid3hctx_s, 4380));
> +                       __u64 w_init = min(    4 * hctx->ccid3hctx_s,
> +                                          max(2 * hctx->ccid3hctx_s, 4380));

This has broken this code on line 478 of ccid3.c:
			ccid3_pr_debug("%s(%p), s=%u, w_init=%u, "
				       "R_sample=%ldus, X=%u\n", dccp_role(sk),
				       sk, hctx->ccid3hctx_s, w_init, r_sample,
				       (unsigned)(hctx->ccid3hctx_x >> 6));


If I change the line to this it works for me:
			ccid3_pr_debug("%s(%p), s=%u, w_init=%llu, "

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

Re: [PATCH] BUG-FIX - conversion errors

[Arnaldo Carvalho de Melo]

On Sat, Dec 09, 2006 at 04:57:12PM +1300, Ian McDonald wrote:
> On 12/9/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> >[CCID3]: BUG-FIX - conversion errors
> >-                       u16 w_init = min(    4 * hctx->ccid3hctx_s,
> >-                                        max(2 * hctx->ccid3hctx_s, 4380));
> >+                       __u64 w_init = min(    4 * hctx->ccid3hctx_s,
> >+                                          max(2 * hctx->ccid3hctx_s, 
> >4380));
> 
> This has broken this code on line 478 of ccid3.c:
> 			ccid3_pr_debug("%s(%p), s=%u, w_init=%u, "
> 				       "R_sample=%ldus, X=%u\n", 
> 				       dccp_role(sk),
> 				       sk, hctx->ccid3hctx_s, w_init, 
> 				       r_sample,
> 				       (unsigned)(hctx->ccid3hctx_x >> 6));
> 
> 
> If I change the line to this it works for me:
> 			ccid3_pr_debug("%s(%p), s=%u, w_init=%llu, "

Thanks Ian, applied this fix.

- Arnaldo
- 

Re: [PATCH] BUG-FIX - conversion errors

[Gerrit Renker]

Quoting Ian McDonald:
|  On 12/9/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
|  > [CCID3]: BUG-FIX - conversion errors
|  > -                       u16 w_init = min(    4 * hctx->ccid3hctx_s,
|  > -                                        max(2 * hctx->ccid3hctx_s, 4380));
|  > +                       __u64 w_init = min(    4 * hctx->ccid3hctx_s,
|  > +                                          max(2 * hctx->ccid3hctx_s, 4380));
|  
|  This has broken this code on line 478 of ccid3.c:
|  			ccid3_pr_debug("%s(%p), s=%u, w_init=%u, "
|  				       "R_sample=%ldus, X=%u\n", dccp_role(sk),
|  				       sk, hctx->ccid3hctx_s, w_init, r_sample,
|  				       (unsigned)(hctx->ccid3hctx_x >> 6));
|  
|  
|  If I change the line to this it works for me:
|  			ccid3_pr_debug("%s(%p), s=%u, w_init=%llu, "
|  
Hi there - sorry about the whitespace errors and the above one. I thought about this one
when I went home on Friday. I will re-work the above, also to use MSS instead of `s', which
does make sense.

Thanks for all the good work, Arnaldo.

Gerrit

Re: [PATCH] BUG-FIX - conversion errors

[Ian McDonald]

On 12/11/06, Gerrit Renker <gerrit@erg.abdn.ac.uk> wrote:
> Hi there - sorry about the whitespace errors and the above one. I thought about this one
> when I went home on Friday. I will re-work the above, also to use MSS instead of `s', which
> does make sense.
>
> Thanks for all the good work, Arnaldo.
>
> Gerrit
>
Do we calculate mss anywhere?

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

Re: [PATCH] BUG-FIX - conversion errors

[Gerrit Renker]

Quoting Ian McDonald:
|  Do we calculate mss anywhere?
Yes there is support in dccps_mss_cache .
At the begin of dccp_send_msg e.g. it is used. 

I have thought about `s' vs. MSS and it is really stupid not to use MSS
 -- if `s' is e.g. 20 bytes, one can only send 40 ... 80 bytes per RTT
 -- with jumbo ethernet cards the MTU can be as high as 10k -- a multiple of 20 bytes :)

Gerrit

Re: [PATCH] BUG-FIX - conversion errors

[Arnaldo Carvalho de Melo]

On Mon, Dec 11, 2006 at 09:03:09AM +0000, Gerrit Renker wrote:
> Quoting Ian McDonald:
> |  Do we calculate mss anywhere?
> Yes there is support in dccps_mss_cache .
> At the begin of dccp_send_msg e.g. it is used. 

Yup, look at dccp_sync_mss() used in dccp_connect_init():

dccp_sync_mss(sk, dst_mtu(dst));

dccp_init_sock also sets it to a default value (same as for TCP in
tcp_v4_init_sock) and sets the struct inet_connection_sock (TCP and DCCP
have this same common ancestor class) -icsk_sync_mss() method (function
pointer 8) ) to dccp_sync_mss(), this allows the IPv4 and V6 transport
protocol independent setsockopt code to sync the mss when IP_OPTIONS is
set by apps, making sure ->icsk_ext_hdr_len is correctly taken into
account when the MSS is calculated (see net/ipv4/ip_sockglue.c &
net/ipv6/ipv6_sockglue.c).

Having said that look at dccp_sync_mss, there is a big FIXME, this one:

----------------- 8< -------------

        int mss_now = (pmtu - icsk->icsk_af_ops->net_header_len -
                       sizeof(struct dccp_hdr) -
		       sizeof(struct dccp_hdr_ext));

        /* Now subtract optional transport overhead */
        mss_now -= icsk->icsk_ext_hdr_len;

        /*
	 * FIXME: this should come from the CCID infrastructure, where,
	 * say, TFRC will say it wants TIMESTAMPS, ELAPSED time, etc,
	 * for now lets put a rough estimate for NDP + TIMESTAMP +
	 * TIMESTAMP_ECHO + ELAPSED TIME + TFRC_OPT_LOSS_EVENT_RATE +
	 * TFRC_OPT_RECEIVE_RATE + padding to
	 * make it a multiple of 4
         */

        mss_now -= ((5 + 6 + 10 + 6 + 6 + 6 + 3) / 4) * 4;

        /* And store cached results */
        icsk->icsk_pmtu_cookie = pmtu;
        dp->dccps_mss_cache = mss_now;

----------------- 8< -------------

Infrastructure wise I guess struct ccid_operations should have a...
lemme read the relevant part of 4340... there CCMPS:

----------------- 8< -------------
14.  Maximum Packet Size

   A DCCP implementation MUST maintain the maximum packet size (MPS)
   allowed for each active DCCP session.  The MPS is influenced by the
   maximum packet size allowed by the current congestion control
   mechanism (CCMPS), the maximum packet size supported by the path's
   links (PMTU, the Path Maximum Transmission Unit) [RFC1191], and the
   lengths of the IP and DCCP headers.
----------------- 8< -------------

something like adding a 'ccid_mps' member to struct ccid_operations,
that would be set at suitable places in the CCID code and would be used
by icsk->icsk_sync_mss(), that could be called from ccid code when and
if the ccid code thinks it should change this value, which I guess its
just at connection setup, but could be at any time.

> I have thought about `s' vs. MSS and it is really stupid not to use MSS
>  -- if `s' is e.g. 20 bytes, one can only send 40 ... 80 bytes per RTT
>  -- with jumbo ethernet cards the MTU can be as high as 10k -- a multiple of 20 bytes :)

Yup :-)

- Arnaldo

Re: [PATCH] BUG-FIX - conversion errors

[Gerrit Renker]

|  > |  Do we calculate mss anywhere?
|  > Yes there is support in dccps_mss_cache .
|  > At the begin of dccp_send_msg e.g. it is used. 
|  
|  Yup, look at dccp_sync_mss() used in dccp_connect_init():

<snip>

Thank you for the helpful `pointers' :)  - I was amazed a couple of months ago
about how much of that code is active and quietly doing its job with accuracy. 


|          /*
|           * FIXME: this should come from the CCID infrastructure, where,
|           * say, TFRC will say it wants TIMESTAMPS, ELAPSED time, etc,
|           * for now lets put a rough estimate for NDP + TIMESTAMP +
|           * TIMESTAMP_ECHO + ELAPSED TIME + TFRC_OPT_LOSS_EVENT_RATE +
|           * TFRC_OPT_RECEIVE_RATE + padding to
|           * make it a multiple of 4
|           */
|  
|          mss_now -= ((5 + 6 + 10 + 6 + 6 + 6 + 3) / 4) * 4;
|  
I thought that when we are actively using the provided MSS facilities, then there
is more incentive to tackle this one above too.



|  Infrastructure wise I guess struct ccid_operations should have a...
|  lemme read the relevant part of 4340... there CCMPS:
|  
|  ----------------- 8< -------------
|  14.  Maximum Packet Size
|  
|     A DCCP implementation MUST maintain the maximum packet size (MPS)
|     allowed for each active DCCP session.  The MPS is influenced by the
|     maximum packet size allowed by the current congestion control
|     mechanism (CCMPS), the maximum packet size supported by the path's
|     links (PMTU, the Path Maximum Transmission Unit) [RFC1191], and the
|     lengths of the IP and DCCP headers.
|  ----------------- 8< -------------
I don't know how often I read through 4340, 4341 and 4342 trying to find out what `CCMPS'
is for - it seems that in CCID2 and CCID3 it is both set to `infinity', at least I can
find no explicit mention in 4341/4342 of CCMPS.

But for the MSS caching a `ccid_mps' member would be useful anyway for the above required purpose.

This in turn re-opens other questions - in CCID3 e.g. feedback packets should always contain
also a Loss Intervals option - which is still missing in CCID 3. 

It is good that you raise this; I put a copy into my ToDo folder and will ask around in some time
if anyone is working on it. If no one else does it I will give it a try at some time, but am bogged
down by other work. I am re-working the CCID `larger initial windows' patch at the moment, plus
sequence number issues. 

Re: [PATCH] BUG-FIX - conversion errors

[Eddie Kohler]

FYI, you're right, Gerrit: CCIDs 2 and 3 don't mention CCMPS because for 
those CCIDs, CCMPS=\infty.  CCID 4, when defined, will impose a CCMPS.

Eddie


Gerrit Renker wrote:
> |  > |  Do we calculate mss anywhere?
> |  > Yes there is support in dccps_mss_cache .
> |  > At the begin of dccp_send_msg e.g. it is used. 
> |  
> |  Yup, look at dccp_sync_mss() used in dccp_connect_init():
> 
> <snip>
> 
> Thank you for the helpful `pointers' :)  - I was amazed a couple of months ago
> about how much of that code is active and quietly doing its job with accuracy. 
> 
> 
> |          /*
> |           * FIXME: this should come from the CCID infrastructure, where,
> |           * say, TFRC will say it wants TIMESTAMPS, ELAPSED time, etc,
> |           * for now lets put a rough estimate for NDP + TIMESTAMP +
> |           * TIMESTAMP_ECHO + ELAPSED TIME + TFRC_OPT_LOSS_EVENT_RATE +
> |           * TFRC_OPT_RECEIVE_RATE + padding to
> |           * make it a multiple of 4
> |           */
> |  
> |          mss_now -= ((5 + 6 + 10 + 6 + 6 + 6 + 3) / 4) * 4;
> |  
> I thought that when we are actively using the provided MSS facilities, then there
> is more incentive to tackle this one above too.
> 
> 
> 
> |  Infrastructure wise I guess struct ccid_operations should have a...
> |  lemme read the relevant part of 4340... there CCMPS:
> |  
> |  ----------------- 8< -------------
> |  14.  Maximum Packet Size
> |  
> |     A DCCP implementation MUST maintain the maximum packet size (MPS)
> |     allowed for each active DCCP session.  The MPS is influenced by the
> |     maximum packet size allowed by the current congestion control
> |     mechanism (CCMPS), the maximum packet size supported by the path's
> |     links (PMTU, the Path Maximum Transmission Unit) [RFC1191], and the
> |     lengths of the IP and DCCP headers.
> |  ----------------- 8< -------------
> I don't know how often I read through 4340, 4341 and 4342 trying to find out what `CCMPS'
> is for - it seems that in CCID2 and CCID3 it is both set to `infinity', at least I can
> find no explicit mention in 4341/4342 of CCMPS.
> 
> But for the MSS caching a `ccid_mps' member would be useful anyway for the above required purpose.
> 
> This in turn re-opens other questions - in CCID3 e.g. feedback packets should always contain
> also a Loss Intervals option - which is still missing in CCID 3. 
> 
> It is good that you raise this; I put a copy into my ToDo folder and will ask around in some time
> if anyone is working on it. If no one else does it I will give it a try at some time, but am bogged
> down by other work. I am re-working the CCID `larger initial windows' patch at the moment, plus
> sequence number issues. 
> -
> To unsubscribe from this list: send the line "unsubscribe dccp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html