Skip to contentSkip to conversation

: Changes after retracting timestamping patches

PATCH-UPDATE Gerrit Renker 2 participants 4 messages Apr 13, 2007 Networking

lore
Share & tools
lore
#0 lore raw

Following the earlier discussions of patches, here is the update after removing the skb_get_timestamp() patches.

I. Changes
=====

The following patches have been withdrawn:

* 6b_Timestamp_Without-Epoch.diff

        http://www.mail-archive.com/[email protected]/msg01643.html

* 6d_CCID3-TX-Use-net_timestamp.diff

        http://www.mail-archive.com/[email protected]/msg01644.html

* 6e_CCID3-RX-Use-net_timestamp.diff

        http://www.mail-archive.com/[email protected]/msg01645.html

All subsequent patches which depended on these patches have been modified using the following vim-expressions:

s#skb_get_timestamp(skb,#dccp_timestamp(sk,#gc s#do_gettimeofday(#dccp_timestamp(sk,#gc

There are no other changes, which is why the patches are not resubmitted. The number of patches in the first patch has reduced from 25 to 19 (3 were committed, 3 withdrawn).

The following patch has changed in addition to the above:

9f_CCID3-Macro-for-Moving-Average.diff

This now uses the code sent by Arnaldo on
http://www.mail-archive.com/[email protected]/msg01662.html
and has been renamed into 9f_CCID3-Inline-for-Moving-Average.diff

II. Updates
=====All 66 updated patches have been uploaded to
http://www.erg.abdn.ac.uk/users/gerrit/dccp/patch-backlog

The entire updated changeset is again in
http://www.erg.abdn.ac.uk/users/gerrit/dccp/patch-backlog/the_whole_lot.tar.gz

I have further updated the combined patch sets on
http://www.erg.abdn.ac.uk/users/gerrit/dccp/patches_working-set/

#1 lore raw
Quoted 2 lines · Gerrit Renker

Following the earlier discussions of patches, here is the update after removing the skb_get_timestamp() patches.

OK, starting with the first, how can it be that it honours the initial exchange if the timestamp is taken now not from the skb timestamp but at that point in time? I.e. from a quick reading the comment doesn't make sense anymore, as we're not getting the timestamp taken at packet arrival, as dccp_timestamp just does a do_gettimeofday.

I think we should take the timestamp at dccp_v[46]_rcv time, into the skb, that won't be as precise as skb_get_timestamp+net_enable_timestamp, but at least will eliminate the time the packet sits in the backlog (if it sits at all), no?

- Arnaldo

#2 lore raw
In reply to Gerrit Renker

You are right, this is an oversight. I wanted to resubmit this patch to the list anyway, since Dave had some reservations which I think originated from the timestamping issue.

The fix is in using dccp_timestamp, as in all other patches I have been updating over the day. Will send this update shortly.

|  I think we should take the timestamp at dccp_v[46]_rcv time, into the
|  skb, that won't be  as precise as
|  skb_get_timestamp+net_enable_timestamp, but at least will eliminate
|  the time the packet sits in the backlog (if it sits at all), no?
|  

I think that this makes not a huge difference. I am preparing an email to Eddie which says why. Essentially I think that Linux is the wrong platform for CCID3 anyway, so if this slows down the sender, it may even be better to keep speed within a controllable range. Many thanks for looking into this.

#3 lore raw
In reply to Gerrit Renker
|  The fix is in using dccp_timestamp, as in all other patches I have been updating
|  over the day. Will send this update shortly.

I have verified this - the patch set is correct. The initial RTT estimate taken from the Request/Response exchange in net/dccp/input.c

The reason is that the patch "Cheaper & smaller timestamping" (6b) has been retracted, i.e. the following change no longer applies

net/dccp/input.c +1 -1
@@ -304,7 +304,7 @@ static int dccp_rcv_request_sent_state_p
                if (dp->dccps_options_received.dccpor_timestamp_echo) {
                        struct timeval now;
 
-                       dccp_timestamp(sk, &now);
+                       do_gettimeofday(&now);
                        dp->dccps_syn_rtt = dccp_sample_rtt(sk, &now, NULL);
                }
 

=> Which means that all timestamps are relative to dccps_epoch.

To be extra safe, I just did a test run and got

Apr 13 19:11:41 kernel: [ 3171.585188] ccid3_hc_tx_send_packet: SYN RTT = 321us

So this does work.

However, the RTTs now grow to big fat values such as several seconds. Can't help it, that is the price of not using a realtime operating system.