[PATCH net-next 0/4] tcp_metrics: four fixes

[PATCH net-next 0/4] tcp_metrics: four fixes

[Eric Dumazet]

Looking at an inconclusive syzbot report, I was surprised
to see that tcp_metrics cache on my host was full of
useless entries, even though I have
/proc/sys/net/ipv4/tcp_no_metrics_save set to 1.

While looking more closely I found a total of four issues.

Eric Dumazet (4):
  tcp_metrics: add missing barriers on delete
  tcp_metrics: properly set tp->snd_ssthresh in tcp_init_metrics()
  tcp_metrics: do not create an entry from tcp_init_metrics()
  tcp_metrics: optimize tcp_metrics_flush_all()

 net/ipv4/tcp_metrics.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH net-next 1/4] tcp_metrics: add missing barriers on delete

[Eric Dumazet]

When removing an item from RCU protected list, we must prevent
store-tearing, using rcu_assign_pointer() or WRITE_ONCE().

Fixes: 04f721c671656 ("tcp_metrics: Rewrite tcp_metrics_flush_all")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index c196759f1d3bd8cb16479522e936bd95091f89b2..4bfa2fb27de5481ca3d1300d7e7b2c80d1577a31 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -908,7 +908,7 @@ static void tcp_metrics_flush_all(struct net *net)
 			match = net ? net_eq(tm_net(tm), net) :
 				!refcount_read(&tm_net(tm)->ns.count);
 			if (match) {
-				*pp = tm->tcpm_next;
+				rcu_assign_pointer(*pp, tm->tcpm_next);
 				kfree_rcu(tm, rcu_head);
 			} else {
 				pp = &tm->tcpm_next;
@@ -949,7 +949,7 @@ static int tcp_metrics_nl_cmd_del(struct sk_buff *skb, struct genl_info *info)
 		if (addr_same(&tm->tcpm_daddr, &daddr) &&
 		    (!src || addr_same(&tm->tcpm_saddr, &saddr)) &&
 		    net_eq(tm_net(tm), net)) {
-			*pp = tm->tcpm_next;
+			rcu_assign_pointer(*pp, tm->tcpm_next);
 			kfree_rcu(tm, rcu_head);
 			found = true;
 		} else {
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH net-next 2/4] tcp_metrics: properly set tp->snd_ssthresh in tcp_init_metrics()

[Eric Dumazet]

We need to set tp->snd_ssthresh to TCP_INFINITE_SSTHRESH
in the case tcp_get_metrics() fails for some reason.

Fixes: 9ad7c049f0f7 ("tcp: RFC2988bis + taking RTT sample from 3WHS for the passive open side")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 4bfa2fb27de5481ca3d1300d7e7b2c80d1577a31..0c03f564878ff0a0dbefd9b631f54697346c8fa9 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -470,6 +470,10 @@ void tcp_init_metrics(struct sock *sk)
 	u32 val, crtt = 0; /* cached RTT scaled by 8 */
 
 	sk_dst_confirm(sk);
+	/* ssthresh may have been reduced unnecessarily during.
+	 * 3WHS. Restore it back to its initial default.
+	 */
+	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
 	if (!dst)
 		goto reset;
 
@@ -489,11 +493,6 @@ void tcp_init_metrics(struct sock *sk)
 		tp->snd_ssthresh = val;
 		if (tp->snd_ssthresh > tp->snd_cwnd_clamp)
 			tp->snd_ssthresh = tp->snd_cwnd_clamp;
-	} else {
-		/* ssthresh may have been reduced unnecessarily during.
-		 * 3WHS. Restore it back to its initial default.
-		 */
-		tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
 	}
 	val = tcp_metric_get(tm, TCP_METRIC_REORDERING);
 	if (val && tp->reordering != val)
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH net-next 3/4] tcp_metrics: do not create an entry from tcp_init_metrics()

[Eric Dumazet]

tcp_init_metrics() only wants to get metrics if they were
previously stored in the cache. Creating an entry is adding
useless costs, especially when tcp_no_metrics_save is set.

Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 0c03f564878ff0a0dbefd9b631f54697346c8fa9..7aca12c59c18483f42276d01252ed0fac326e5d8 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -478,7 +478,7 @@ void tcp_init_metrics(struct sock *sk)
 		goto reset;
 
 	rcu_read_lock();
-	tm = tcp_get_metrics(sk, dst, true);
+	tm = tcp_get_metrics(sk, dst, false);
 	if (!tm) {
 		rcu_read_unlock();
 		goto reset;
-- 
2.42.0.515.g380fc7ccd1-goog

[PATCH net-next 4/4] tcp_metrics: optimize tcp_metrics_flush_all()

[Eric Dumazet]

This is inspired by several syzbot reports where
tcp_metrics_flush_all() was seen in the traces.

We can avoid acquiring tcp_metrics_lock for empty buckets,
and we should add one cond_resched() to break potential long loops.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/tcp_metrics.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
index 7aca12c59c18483f42276d01252ed0fac326e5d8..c2a925538542b5d787596b7d76705dda86cf48d8 100644
--- a/net/ipv4/tcp_metrics.c
+++ b/net/ipv4/tcp_metrics.c
@@ -898,11 +898,13 @@ static void tcp_metrics_flush_all(struct net *net)
 	unsigned int row;
 
 	for (row = 0; row < max_rows; row++, hb++) {
-		struct tcp_metrics_block __rcu **pp;
+		struct tcp_metrics_block __rcu **pp = &hb->chain;
 		bool match;
 
+		if (!rcu_access_pointer(*pp))
+			continue;
+
 		spin_lock_bh(&tcp_metrics_lock);
-		pp = &hb->chain;
 		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
 			match = net ? net_eq(tm_net(tm), net) :
 				!refcount_read(&tm_net(tm)->ns.count);
@@ -914,6 +916,7 @@ static void tcp_metrics_flush_all(struct net *net)
 			}
 		}
 		spin_unlock_bh(&tcp_metrics_lock);
+		cond_resched();
 	}
 }
 
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [PATCH net-next 4/4] tcp_metrics: optimize tcp_metrics_flush_all()

[David Ahern]

On 9/22/23 4:03 PM, Eric Dumazet wrote:
> This is inspired by several syzbot reports where
> tcp_metrics_flush_all() was seen in the traces.
> 
> We can avoid acquiring tcp_metrics_lock for empty buckets,
> and we should add one cond_resched() to break potential long loops.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 7aca12c59c18483f42276d01252ed0fac326e5d8..c2a925538542b5d787596b7d76705dda86cf48d8 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -898,11 +898,13 @@ static void tcp_metrics_flush_all(struct net *net)
>  	unsigned int row;
>  
>  	for (row = 0; row < max_rows; row++, hb++) {
> -		struct tcp_metrics_block __rcu **pp;
> +		struct tcp_metrics_block __rcu **pp = &hb->chain;
>  		bool match;
>  
> +		if (!rcu_access_pointer(*pp))
> +			continue;
> +
>  		spin_lock_bh(&tcp_metrics_lock);
> -		pp = &hb->chain;
>  		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
>  			match = net ? net_eq(tm_net(tm), net) :
>  				!refcount_read(&tm_net(tm)->ns.count);
> @@ -914,6 +916,7 @@ static void tcp_metrics_flush_all(struct net *net)
>  			}
>  		}
>  		spin_unlock_bh(&tcp_metrics_lock);
> +		cond_resched();

I have found cond_resched() can occur some unnecessary overhead if
called too often. Wrap in `if (need_resched)`?


Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 2/4] tcp_metrics: properly set tp->snd_ssthresh in tcp_init_metrics()

[David Ahern]

On 9/22/23 4:03 PM, Eric Dumazet wrote:
> We need to set tp->snd_ssthresh to TCP_INFINITE_SSTHRESH
> in the case tcp_get_metrics() fails for some reason.
> 
> Fixes: 9ad7c049f0f7 ("tcp: RFC2988bis + taking RTT sample from 3WHS for the passive open side")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> index 4bfa2fb27de5481ca3d1300d7e7b2c80d1577a31..0c03f564878ff0a0dbefd9b631f54697346c8fa9 100644
> --- a/net/ipv4/tcp_metrics.c
> +++ b/net/ipv4/tcp_metrics.c
> @@ -470,6 +470,10 @@ void tcp_init_metrics(struct sock *sk)
>  	u32 val, crtt = 0; /* cached RTT scaled by 8 */
>  
>  	sk_dst_confirm(sk);
> +	/* ssthresh may have been reduced unnecessarily during.
> +	 * 3WHS. Restore it back to its initial default.
> +	 */
> +	tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;

This would read easier with newlines after sk_dst_confirm and again here.
>  	if (!dst)
>  		goto reset;
>  
> @@ -489,11 +493,6 @@ void tcp_init_metrics(struct sock *sk)
>  		tp->snd_ssthresh = val;
>  		if (tp->snd_ssthresh > tp->snd_cwnd_clamp)
>  			tp->snd_ssthresh = tp->snd_cwnd_clamp;
> -	} else {
> -		/* ssthresh may have been reduced unnecessarily during.
> -		 * 3WHS. Restore it back to its initial default.
> -		 */
> -		tp->snd_ssthresh = TCP_INFINITE_SSTHRESH;
>  	}
>  	val = tcp_metric_get(tm, TCP_METRIC_REORDERING);
>  	if (val && tp->reordering != val)

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 1/4] tcp_metrics: add missing barriers on delete

[David Ahern]

On 9/22/23 4:03 PM, Eric Dumazet wrote:
> When removing an item from RCU protected list, we must prevent
> store-tearing, using rcu_assign_pointer() or WRITE_ONCE().
> 
> Fixes: 04f721c671656 ("tcp_metrics: Rewrite tcp_metrics_flush_all")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 3/4] tcp_metrics: do not create an entry from tcp_init_metrics()

[David Ahern]

On 9/22/23 4:03 PM, Eric Dumazet wrote:
> tcp_init_metrics() only wants to get metrics if they were
> previously stored in the cache. Creating an entry is adding
> useless costs, especially when tcp_no_metrics_save is set.
> 
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/tcp_metrics.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: David Ahern <dsahern@kernel.org>

Re: [PATCH net-next 4/4] tcp_metrics: optimize tcp_metrics_flush_all()

[Neal Cardwell]

On Fri, Sep 22, 2023 at 6:04 PM Eric Dumazet <edumazet@google.com> wrote:
>
> This is inspired by several syzbot reports where
> tcp_metrics_flush_all() was seen in the traces.
>
> We can avoid acquiring tcp_metrics_lock for empty buckets,
> and we should add one cond_resched() to break potential long loops.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH net-next 2/4] tcp_metrics: properly set tp->snd_ssthresh in tcp_init_metrics()

[Neal Cardwell]

On Fri, Sep 22, 2023 at 6:04 PM Eric Dumazet <edumazet@google.com> wrote:
>
> We need to set tp->snd_ssthresh to TCP_INFINITE_SSTHRESH
> in the case tcp_get_metrics() fails for some reason.
>
> Fixes: 9ad7c049f0f7 ("tcp: RFC2988bis + taking RTT sample from 3WHS for the passive open side")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---


Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH net-next 1/4] tcp_metrics: add missing barriers on delete

[Neal Cardwell]

On Fri, Sep 22, 2023 at 6:04 PM Eric Dumazet <edumazet@google.com> wrote:
>
> When removing an item from RCU protected list, we must prevent
> store-tearing, using rcu_assign_pointer() or WRITE_ONCE().
>
> Fixes: 04f721c671656 ("tcp_metrics: Rewrite tcp_metrics_flush_all")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH net-next 3/4] tcp_metrics: do not create an entry from tcp_init_metrics()

[Neal Cardwell]

On Fri, Sep 22, 2023 at 6:04 PM Eric Dumazet <edumazet@google.com> wrote:
>
> tcp_init_metrics() only wants to get metrics if they were
> previously stored in the cache. Creating an entry is adding
> useless costs, especially when tcp_no_metrics_save is set.
>
> Fixes: 51c5d0c4b169 ("tcp: Maintain dynamic metrics in local cache.")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---

Acked-by: Neal Cardwell <ncardwell@google.com>

neal

Re: [PATCH net-next 4/4] tcp_metrics: optimize tcp_metrics_flush_all()

[Paolo Abeni]

On Sat, 2023-09-23 at 13:07 +0200, David Ahern wrote:
> On 9/22/23 4:03 PM, Eric Dumazet wrote:
> > This is inspired by several syzbot reports where
> > tcp_metrics_flush_all() was seen in the traces.
> > 
> > We can avoid acquiring tcp_metrics_lock for empty buckets,
> > and we should add one cond_resched() to break potential long loops.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/ipv4/tcp_metrics.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_metrics.c b/net/ipv4/tcp_metrics.c
> > index 7aca12c59c18483f42276d01252ed0fac326e5d8..c2a925538542b5d787596b7d76705dda86cf48d8 100644
> > --- a/net/ipv4/tcp_metrics.c
> > +++ b/net/ipv4/tcp_metrics.c
> > @@ -898,11 +898,13 @@ static void tcp_metrics_flush_all(struct net *net)
> >  	unsigned int row;
> >  
> >  	for (row = 0; row < max_rows; row++, hb++) {
> > -		struct tcp_metrics_block __rcu **pp;
> > +		struct tcp_metrics_block __rcu **pp = &hb->chain;
> >  		bool match;
> >  
> > +		if (!rcu_access_pointer(*pp))
> > +			continue;
> > +
> >  		spin_lock_bh(&tcp_metrics_lock);
> > -		pp = &hb->chain;
> >  		for (tm = deref_locked(*pp); tm; tm = deref_locked(*pp)) {
> >  			match = net ? net_eq(tm_net(tm), net) :
> >  				!refcount_read(&tm_net(tm)->ns.count);
> > @@ -914,6 +916,7 @@ static void tcp_metrics_flush_all(struct net *net)
> >  			}
> >  		}
> >  		spin_unlock_bh(&tcp_metrics_lock);
> > +		cond_resched();
> 
> I have found cond_resched() can occur some unnecessary overhead if
> called too often. Wrap in `if (need_resched)`?

Interesting. I could not find any significant overhead with code
inspection - it should be a matter of 2 conditionals instead of one -
Any idea why?

In any case I think we can follow-up with that if needed - e.g. no
changes required here.

Cheers,

Paolo

Re: [PATCH net-next 0/4] tcp_metrics: four fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 22 Sep 2023 22:03:52 +0000 you wrote:
> Looking at an inconclusive syzbot report, I was surprised
> to see that tcp_metrics cache on my host was full of
> useless entries, even though I have
> /proc/sys/net/ipv4/tcp_no_metrics_save set to 1.
> 
> While looking more closely I found a total of four issues.
> 
> [...]

Here is the summary with links:
  - [net-next,1/4] tcp_metrics: add missing barriers on delete
    https://git.kernel.org/netdev/net-next/c/cbc3a1532228
  - [net-next,2/4] tcp_metrics: properly set tp->snd_ssthresh in tcp_init_metrics()
    https://git.kernel.org/netdev/net-next/c/081480014a64
  - [net-next,3/4] tcp_metrics: do not create an entry from tcp_init_metrics()
    https://git.kernel.org/netdev/net-next/c/a135798e6e20
  - [net-next,4/4] tcp_metrics: optimize tcp_metrics_flush_all()
    https://git.kernel.org/netdev/net-next/c/6532e257aa73

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 4/4] tcp_metrics: optimize tcp_metrics_flush_all()

[David Ahern]

On 10/3/23 2:04 AM, Paolo Abeni wrote:
>> I have found cond_resched() can occur some unnecessary overhead if
>> called too often. Wrap in `if (need_resched)`?
> 
> Interesting. I could not find any significant overhead with code
> inspection - it should be a matter of 2 conditionals instead of one -
> Any idea why?

I would need to go through the ftrace output. I think it is really use
case dependent and for this one and the way tcp_metrics_nl_cmd_del is
called just cond_resched is ok.