netdev - Re: [PATCH net] tcp: set retransmit

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: <CADVnQy=s6mVpVzNKhSQw9ZULsomz=Qj9Xu-iOfREH3Z9gyMfbg@mail.gmail.com>
Date:	Wed, 14 May 2014 14:46:52 -0400
From:	Neal Cardwell <ncardwell@...gle.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
Cc:	David Miller <davem@...emloft.net>, panweiping3@...il.com,
	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] tcp: set retransmit_high on loop exit

On Wed, May 14, 2014 at 2:27 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
> On Wed, 2014-05-14 at 14:08 -0400, David Miller wrote:
>> From: Weiping Pan <panweiping3@...il.com>
>> Date: Sun, 11 May 2014 20:57:30 +0800
>>
>> > To save some CPU cycles slightly.
>> >
>> > Signed-off-by: Weiping Pan <panweiping3@...il.com>
>>
>> Can a TCP expert please review this?
>>
>> > ---
>> >  net/ipv4/tcp_input.c | 8 ++++++--
>> >  1 file changed, 6 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
>> > index d6b46eb..e431037 100644
>> > --- a/net/ipv4/tcp_input.c
>> > +++ b/net/ipv4/tcp_input.c
>> > @@ -1941,8 +1941,13 @@ void tcp_enter_loss(struct sock *sk, int how)
>> >     tcp_clear_all_retrans_hints(tp);
>> >
>> >     tcp_for_write_queue(skb, sk) {
>> > -           if (skb == tcp_send_head(sk))
>> > +           if (skb == tcp_send_head(sk)) {
>> > +                   if (skb != tcp_write_queue_head(sk)) {
>> > +                           skb = tcp_write_queue_prev(sk, skb);
>> > +                           tp->retransmit_high = TCP_SKB_CB(skb)->end_seq;
>> > +                   }
>> >                     break;
>> > +           }
>> >
>> >             if (TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_RETRANS)
>> >                     tp->undo_marker = 0;
>> > @@ -1952,7 +1957,6 @@ void tcp_enter_loss(struct sock *sk, int how)
>> >                     TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_ACKED;
>> >                     TCP_SKB_CB(skb)->sacked |= TCPCB_LOST;
>> >                     tp->lost_out += tcp_skb_pcount(skb);
>> > -                   tp->retransmit_high = TCP_SKB_CB(skb)->end_seq;
>> >             }
>> >     }
>> >     tcp_verify_left_out(tp);
>> > --
>
> Frankly I do not see the point here.
>
> This adds code and conditional jumps, while original code is 2
> instructions with no cache line misses, and is cleaner IMHO.

I very much agree with Eric that the original code is simpler,
cleaner, and almost certainly just as fast as the code in the patch.

In addition, the patch has at least one significant bug: it sets
tp->retransmit_high to the end of the last skb even when the last skb
has been SACKed. We don't want to do that. :-)

neal
--
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