Messages in this thread | | | From | Yafang Shao <> | Date | Sun, 4 Nov 2018 09:26:53 +0800 | Subject | Re: [PATCH] tcp: do not update snd_una if it is same with ack_seq |
| |
On Sun, Nov 4, 2018 at 8:40 AM Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > > On 11/03/2018 09:54 AM, Yafang Shao wrote: > > In the slow path, TCP_SKB_SB(skb)->ack_seq may be same with tp->snd_una, > > and under this condition we don't need to update the snd_una. > > > > Furthermore, tcp_ack_update_window() is only called in slow path, > > so introducing this check won't affect the fast path processing. > > > > By the way, '&' is a little faster than '-', so I replaced after() with > > "flag & FLAG_SND_UNA_ADVANCED". > > > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > > --- > > net/ipv4/tcp_input.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > > index 2868ef2..db5a6b7 100644 > > --- a/net/ipv4/tcp_input.c > > +++ b/net/ipv4/tcp_input.c > > @@ -3376,7 +3376,8 @@ static int tcp_ack_update_window(struct sock *sk, const struct sk_buff *skb, u32 > > } > > } > > > > - tcp_snd_una_update(tp, ack); > > + if (after(ack, tp->snd_una)) > > + tcp_snd_una_update(tp, ack); > > > > Adding this after() here is confusing, how ack could be before snd_una ? > That would be a serious bug. >
ack can't be before snd_una, but it can be equal with snd_una. Seems bellow change would be more specific, if (ack != tp->snd_una) tcp_snd_una_update(tp, ack);
> I do not see why another conditional has any gain here. > > You are trying to avoid very cheap operations : > > u32 delta = ack - tp->snd_una; > > tp->bytes_acked += delta; > tp->snd_una = ack; > > Maybe the real reason for this patch is not explained in the changelog ?
No additional reason. I just want to avoid these uneccessary operations. Because I find that this uncessary operations always happen, especially when head prediction fails and then the packet can't go to fast path processing.
Thanks Yafang
| |