* [PATCH net] tcp: set retransmit_high on loop exit
@ 2014-05-11 12:57 Weiping Pan
2014-05-14 18:08 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Weiping Pan @ 2014-05-11 12:57 UTC (permalink / raw)
To: netdev; +Cc: Weiping Pan
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] tcp: set retransmit_high on loop exit
2014-05-11 12:57 [PATCH net] tcp: set retransmit_high on loop exit Weiping Pan
@ 2014-05-14 18:08 ` David Miller
2014-05-14 18:27 ` Eric Dumazet
0 siblings, 1 reply; 4+ messages in thread
From: David Miller @ 2014-05-14 18:08 UTC (permalink / raw)
To: panweiping3; +Cc: netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] tcp: set retransmit_high on loop exit
2014-05-14 18:08 ` David Miller
@ 2014-05-14 18:27 ` Eric Dumazet
2014-05-14 18:46 ` Neal Cardwell
0 siblings, 1 reply; 4+ messages in thread
From: Eric Dumazet @ 2014-05-14 18:27 UTC (permalink / raw)
To: David Miller; +Cc: panweiping3, netdev
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] tcp: set retransmit_high on loop exit
2014-05-14 18:27 ` Eric Dumazet
@ 2014-05-14 18:46 ` Neal Cardwell
0 siblings, 0 replies; 4+ messages in thread
From: Neal Cardwell @ 2014-05-14 18:46 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, panweiping3, Netdev
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-05-14 18:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-05-11 12:57 [PATCH net] tcp: set retransmit_high on loop exit Weiping Pan
2014-05-14 18:08 ` David Miller
2014-05-14 18:27 ` Eric Dumazet
2014-05-14 18:46 ` Neal Cardwell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).