[net] tcp: set retransmit_high on loop exit

To save some CPU cycles slightly.

Signed-off-by: Weiping Pan <panweiping3@gmail.com>
---
 net/ipv4/tcp_input.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

From: Weiping Pan <panweiping3@gmail.com>
Date: Sun, 11 May 2014 20:57:30 +0800

> To save some CPU cycles slightly.
> 
> Signed-off-by: Weiping Pan <panweiping3@gmail.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);
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2014-05-14 at 14:08 -0400, David Miller wrote:
> From: Weiping Pan <panweiping3@gmail.com>
> Date: Sun, 11 May 2014 20:57:30 +0800
> 
> > To save some CPU cycles slightly.
> > 
> > Signed-off-by: Weiping Pan <panweiping3@gmail.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.





--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, May 14, 2014 at 2:27 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Wed, 2014-05-14 at 14:08 -0400, David Miller wrote:
>> From: Weiping Pan <panweiping3@gmail.com>
>> Date: Sun, 11 May 2014 20:57:30 +0800
>>
>> > To save some CPU cycles slightly.
>> >
>> > Signed-off-by: Weiping Pan <panweiping3@gmail.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html