netdev - Re: [PATCH RFC net-next] tcp: improve SACK RTT for CC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAK6E8=chmgkJf1-4m+SjMLQm2na0AaP6-bdJebb1yMQ=wx01nQ@mail.gmail.com>
Date:	Sun, 12 Apr 2015 21:24:20 -0700
From:	Yuchung Cheng <ycheng@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	Kenneth Klette Jonassen <kennetkl@....uio.no>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC net-next] tcp: improve SACK RTT for CC

On Sun, Apr 12, 2015 at 11:37 AM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Sun, 2015-04-12 at 19:51 +0200, Kenneth Klette Jonassen wrote:
>> tcp_sacktag_one() always picks the earliest sequence SACKed for RTT.
>> This might not make sense for congestion control in cases where:
>>
>>   1. ACKs are lost, i.e. a SACK subsequent to a lost SACK covers both
>>      a new and an old segment at the receiver.
>>   2. The receiver disregards the RFC 5681 recommendation to immediately
>>      ACK out-of-order segments.
>>
>> Give congestion control a RTT for the latest segment SACKed, which is the
>> most accurate RTT estimate, but preserve the conservative RTT for RTO.
>>
>> Also remove the dependency on skb_mstamp_get() in tcp_sacktag_one() which
>> potentially contributes to variance between RTT signals.
>>
>> This is a request for comments. For full effect, a later patch would move
>> the call to pkts_acked() outside the scope of if (flag & FLAG_ACKED). As is,
>> this hook only gets called when sequentially acknowledging new data.
>> ---
>>  net/ipv4/tcp_input.c | 90 ++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 52 insertions(+), 38 deletions(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> index a7ef679..8f0bd3c 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -1130,7 +1130,16 @@ static bool tcp_check_dsack(struct sock *sk, const struct sk_buff *ack_skb,
>>  struct tcp_sacktag_state {
>>       int     reord;
>>       int     fack_count;
>> -     long    rtt_us; /* RTT measured by SACKing never-retransmitted data */
>> +     /* Timestamp for earliest never-retransmitted segment that was
>> +      * SACKed and its usec delta to the corresponding latest segment.
>> +      * RTO needs the earliest RTT to be conservative against receivers
>> +      * that might delay SACKing (RFC 5681 does not require ACKing
>> +      * out-of-order segments immediately), but congestion control should
>> +      * still get an accurate delay signal. Even without delayed SACKing,
>> +      * this helps mitigate the effect of ACK loss on delay-based CC.
>> +      */
>> +     struct skb_mstamp first_ackt;
>> +     u32     last_ack_us_delta;
>>       int     flag;
>>  };
>>
>> @@ -1233,14 +1242,13 @@ static u8 tcp_sacktag_one(struct sock *sk,
>>                                                          state->reord);
>>                               if (!after(end_seq, tp->high_seq))
>>                                       state->flag |= FLAG_ORIG_SACK_ACKED;
>> -                             /* Pick the earliest sequence sacked for RTT */
>> -                             if (state->rtt_us < 0) {
>> -                                     struct skb_mstamp now;
>>
>> -                                     skb_mstamp_get(&now);
>> -                                     state->rtt_us = skb_mstamp_us_delta(&now,
>> -                                                             xmit_time);
>> -                             }
>> +                             if (state->first_ackt.v64 == 0)
>> +                                     state->first_ackt.v64 = xmit_time->v64;
state->first_ackt = *xmit_time?

>> +                             else
>> +                                     state->last_ack_us_delta =
>> +                                              skb_mstamp_us_delta(xmit_time,
>> +                                                                  &state->first_ackt);
>
> I do not think you need to perform this skb_mstamp_us_delta() at this
> point.
>
> Instead of storing a state->last_ack_us_delta, I would store
> state->last_ackt
>
> And later doing a single skb_mstamp_us_delta()
yes I agree with Eric that having last_ackt and first_act are better.
perhaps s/ackt/sackt too?
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html