Re: [PATCH 7/8]: Handle timestamps on Request/Response exchange

[Arnaldo Carvalho de Melo <acme@ghostprotocols.net>]

Em Fri, Oct 05, 2007 at 11:25:24AM +0100, Gerrit Renker escreveu:
> Arnaldo,
> 
> please disregard the earlier suggestion from below regarding ts_recent and feel free
> to do with the structure as you see fit. 
> 
> To me it seems that the main problems using a RFC1323-like algorithm are
>  * the ts_recent field is not enough, the algorithm requires other information (e.g. whether
>    an Ack advances the send window) to deal robustly with delays, holes, 
>  * it is hard to get right (e.g. omments above tcp_ack_saw_tstamp() in tcp_input.c)
>  * the current solution of timing both send time and Ack arrival is the simplest
>    and has the advantage of being responsive to receiver behaviour (as in CCID3).
>    An additional advantage is that the current code already provides Elapsed Time information
>    on each Ack Vector, so that dccp_sample_rtt() can be used.
>    Maybe CCID2 could benefit by upgrading from jiffies to ktime_t, as this enables to
>    better determine whether multiple losses belong to the same RTT (with 1ms resolution
>    and Gbps speed this does not work so well).

CCID2 needs a lot of love and care, yes :-\

> Please can you let me know whether:
> 
>  * the outlined "struct dccp_request_sock" below is still the preferred format;


Please use the outlined one. I haven't checked, but if we use a struct
like in your second option (below) we can end up with struct holes on
64-bit arches.

>  * whether as an alternative the dreq_tstamp_{echo,time} fields can be combined, i.e.
>    use a fixed member of type
> 	struct dccp_ts_echo {
> 	       	ktime_t         ts_time;
> 	       	__u32           ts_echo;
> 	};
>    or similar - but without the mallocing, and with overriding each time a new timestamp arrives;
>  * or whether a different solution is planned.
> 
> I'd need to know so that I can rework the patches and resubmit them accordingly.
> 
> 	
> Quoting Gerrit Renker:
> |  Quoting Arnaldo Carvalho de Melo:
> |  |  I suggest it to become:
> |  |  
> |  |  [acme@mica net-2.6.24]$ pahole -C dccp_request_sock net/dccp/minisocks.o
> |  |  
> |  |  struct dccp_request_sock {
> |  |          struct inet_request_sock dreq_inet_rsk;    /*  0 56 */
> |  |          __u64                    dreq_iss;         /* 56  8 */
> |  |          /* --- cacheline 1 boundary (64 bytes) --- */
> |  |          __u64                    dreq_isr;         /* 64  8 */
> |  |          __be32                   dreq_service;     /* 72  4 */
> |  |          __u32                    dreq_tstamp_echo; /* 76  4 */
> |  |          ktime_t                  dreq_tstamp_time; /* 80  8 */
> |  |  
> |  |          /* size: 88, cachelines: 2 */
> |  |          /* last cacheline: 24 bytes */
> |  |  };
> |  |  
> |  |  Humm, these minisocks are getting fat... another thing for my TODO list,
> |  |  request_sock::ts_recent seems to be used only by the TCP machinery, ripe
> |  |  for the picking....
> |  
> |  I have thought about this: do you think the following solution is better -
> |  the difference between kmallocing and fixed is now between pointer to struct
> |  and u64 (ktime_t).
> |  
> |  
> |  struct dccp_request_sock {
> |          struct inet_request_sock	dreq_inet_rsk;
> |  	__u64                    	dreq_iss,
> |  				 	dreq_isr; 
> |  	__be32                   	dreq_service;
> |  #define dreq_tstamp_echo		dreq_inet_rsk.req.ts_recent 
> |  	ktime_t                  	dreq_tstamp_time;
> |  };
> |  
> |  
> |  The only other thing that is required is then to change the insertion routine to
> |  
> |  dccp_insert_option_timestamp_echo(struct sock *sk, struct dccp_request_sock *dreq,
> |                                    struct sk_buff *skb);
> |  				  /* when @dreq is NULL, @sk is used */
> |  
> |  
> |  
> |  On another note I think that the CCID2 code could benefit from using such timestamps also, in particular
> |  for high-speed networks.