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

[Gerrit Renker <gerrit@erg.abdn.ac.uk>]

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).

Please can you let me know whether:

 * the outlined "struct dccp_request_sock" below is still the preferred format;
 * 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.