| Message ID | 1541264071-9905-1-git-send-email-laoar.shao@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | tcp: do not update snd_una if it is same with ack_seq | expand |
On Sun, 2018-11-04 at 00:54 +0800, 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. [] > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c [] > @@ -3610,7 +3611,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) > if (flag & FLAG_UPDATE_TS_RECENT) > tcp_replace_ts_recent(tp, TCP_SKB_CB(skb)->seq); > > - if (!(flag & FLAG_SLOWPATH) && after(ack, prior_snd_una)) { > + if (!(flag & FLAG_SLOWPATH) && flag & FLAG_SND_UNA_ADVANCED) { stylistic nit: While the precedence is correct in any case, perhaps adding parentheses around flag & FLAG_SND_UNA_ADVANCED would make it more obvious.
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. 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 ?
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
On Sun, Nov 4, 2018 at 1:04 AM Joe Perches <joe@perches.com> wrote: > > On Sun, 2018-11-04 at 00:54 +0800, 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. > [] > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > [] > > @@ -3610,7 +3611,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) > > if (flag & FLAG_UPDATE_TS_RECENT) > > tcp_replace_ts_recent(tp, TCP_SKB_CB(skb)->seq); > > > > - if (!(flag & FLAG_SLOWPATH) && after(ack, prior_snd_una)) { > > + if (!(flag & FLAG_SLOWPATH) && flag & FLAG_SND_UNA_ADVANCED) { > > stylistic nit: > > While the precedence is correct in any case, > perhaps adding parentheses around > flag & FLAG_SND_UNA_ADVANCED > would make it more obvious. > Sure. will change it. Thanks Yafang
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); return flag; } @@ -3610,7 +3611,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag) if (flag & FLAG_UPDATE_TS_RECENT) tcp_replace_ts_recent(tp, TCP_SKB_CB(skb)->seq); - if (!(flag & FLAG_SLOWPATH) && after(ack, prior_snd_una)) { + if (!(flag & FLAG_SLOWPATH) && flag & FLAG_SND_UNA_ADVANCED) { /* Window is constant, pure forward advance. * No more checks are required. * Note, we use the fact that SND.UNA>=SND.WL2.
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(-)