| Message ID | 4E82FAD6.1010708@intel.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 28 Sep 2011, Yan, Zheng wrote: > > But is the non-SACKed case really handled right when hint == skb by the > > sacktag_one. We move the seqno in between and then before(x->newseq, > > x->newseq) check returns false? > > > you are right, thank you. > > really hope my patch is correct this time :) > --- > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 21fab3e..a04622e 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -1390,8 +1390,7 @@ static int tcp_shifted_skb(struct sock *sk, struct sk_buff *skb, > BUG_ON(!pcount); > > /* Tweak before seqno plays */ > - if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint && > - !before(TCP_SKB_CB(tp->lost_skb_hint)->seq, TCP_SKB_CB(skb)->seq)) > + if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint == skb) > tp->lost_cnt_hint += pcount; > > TCP_SKB_CB(prev)->end_seq += shifted; It also looks a lot nicer now and more obvious. According to my current understanding, feel free to add this once doing the proper submission with Signed-off etc. (please also remove the comment too as seqnos have no longer any significance here): Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> ...but it certainly wouldn't hurt if also somebody else has pair of eyes to spare to confirm that we (both) are now in agreement what the code really says.
Could you please clarify this case for me- skb == tp->lost_skb_hint If skb is sacked, doesn't tcp_mark_head_lost() already increment lost_cnt_hint, in which case you won't need to do it here? Nandita On Wed, Sep 28, 2011 at 4:29 AM, Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> wrote: > On Wed, 28 Sep 2011, Yan, Zheng wrote: > >> > But is the non-SACKed case really handled right when hint == skb by the >> > sacktag_one. We move the seqno in between and then before(x->newseq, >> > x->newseq) check returns false? >> > >> you are right, thank you. >> >> really hope my patch is correct this time :) >> --- >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >> index 21fab3e..a04622e 100644 >> --- a/net/ipv4/tcp_input.c >> +++ b/net/ipv4/tcp_input.c >> @@ -1390,8 +1390,7 @@ static int tcp_shifted_skb(struct sock *sk, struct sk_buff *skb, >> BUG_ON(!pcount); >> >> /* Tweak before seqno plays */ >> - if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint && >> - !before(TCP_SKB_CB(tp->lost_skb_hint)->seq, TCP_SKB_CB(skb)->seq)) >> + if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint == skb) >> tp->lost_cnt_hint += pcount; >> >> TCP_SKB_CB(prev)->end_seq += shifted; > > It also looks a lot nicer now and more obvious. According to my current > understanding, feel free to add this once doing the proper submission with > Signed-off etc. (please also remove the comment too as seqnos have no > longer any significance here): > > Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> > > ...but it certainly wouldn't hurt if also somebody else has pair of eyes > to spare to confirm that we (both) are now in agreement what the code > really says. > > > -- > i. -- 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
Actually don't bother about my question. Your patch is correct. I convinced myself that it's taking care of diff. cases correctly. Nandita On Wed, Sep 28, 2011 at 5:06 PM, Nandita Dukkipati <nanditad@google.com> wrote: > Could you please clarify this case for me- > > skb == tp->lost_skb_hint > If skb is sacked, doesn't tcp_mark_head_lost() already increment > lost_cnt_hint, in which case you won't need to do it here? > > Nandita > > On Wed, Sep 28, 2011 at 4:29 AM, Ilpo Järvinen > <ilpo.jarvinen@helsinki.fi> wrote: >> On Wed, 28 Sep 2011, Yan, Zheng wrote: >> >>> > But is the non-SACKed case really handled right when hint == skb by the >>> > sacktag_one. We move the seqno in between and then before(x->newseq, >>> > x->newseq) check returns false? >>> > >>> you are right, thank you. >>> >>> really hope my patch is correct this time :) >>> --- >>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c >>> index 21fab3e..a04622e 100644 >>> --- a/net/ipv4/tcp_input.c >>> +++ b/net/ipv4/tcp_input.c >>> @@ -1390,8 +1390,7 @@ static int tcp_shifted_skb(struct sock *sk, struct sk_buff *skb, >>> BUG_ON(!pcount); >>> >>> /* Tweak before seqno plays */ >>> - if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint && >>> - !before(TCP_SKB_CB(tp->lost_skb_hint)->seq, TCP_SKB_CB(skb)->seq)) >>> + if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint == skb) >>> tp->lost_cnt_hint += pcount; >>> >>> TCP_SKB_CB(prev)->end_seq += shifted; >> >> It also looks a lot nicer now and more obvious. According to my current >> understanding, feel free to add this once doing the proper submission with >> Signed-off etc. (please also remove the comment too as seqnos have no >> longer any significance here): >> >> Acked-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi> >> >> ...but it certainly wouldn't hurt if also somebody else has pair of eyes >> to spare to confirm that we (both) are now in agreement what the code >> really says. >> >> >> -- >> i. > -- 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
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 21fab3e..a04622e 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -1390,8 +1390,7 @@ static int tcp_shifted_skb(struct sock *sk, struct sk_buff *skb, BUG_ON(!pcount); /* Tweak before seqno plays */ - if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint && - !before(TCP_SKB_CB(tp->lost_skb_hint)->seq, TCP_SKB_CB(skb)->seq)) + if (!tcp_is_fack(tp) && tcp_is_sack(tp) && tp->lost_skb_hint == skb) tp->lost_cnt_hint += pcount; TCP_SKB_CB(prev)->end_seq += shifted;