[PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[Guillaume Nault]

Use the core_stats rx_dropped counter to avoid the cost of atomic
increments.

Signed-off-by: Guillaume Nault <gnault@redhat.com>
---
 drivers/net/geneve.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
index 2f29b1386b1c..671ca5260e92 100644
--- a/drivers/net/geneve.c
+++ b/drivers/net/geneve.c
@@ -235,7 +235,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
 					 vni_to_tunnel_id(gnvh->vni),
 					 gnvh->opt_len * 4);
 		if (!tun_dst) {
-			DEV_STATS_INC(geneve->dev, rx_dropped);
+			dev_core_stats_rx_dropped_inc(geneve->dev);
 			goto drop;
 		}
 		/* Update tunnel dst according to Geneve options. */
@@ -387,14 +387,14 @@ static int geneve_udp_encap_recv(struct sock *sk, struct sk_buff *skb)
 
 	if (unlikely((!geneve->cfg.inner_proto_inherit &&
 		      inner_proto != htons(ETH_P_TEB)))) {
-		DEV_STATS_INC(geneve->dev, rx_dropped);
+		dev_core_stats_rx_dropped_inc(geneve->dev);
 		goto drop;
 	}
 
 	opts_len = geneveh->opt_len * 4;
 	if (iptunnel_pull_header(skb, GENEVE_BASE_HLEN + opts_len, inner_proto,
 				 !net_eq(geneve->net, dev_net(geneve->dev)))) {
-		DEV_STATS_INC(geneve->dev, rx_dropped);
+		dev_core_stats_rx_dropped_inc(geneve->dev);
 		goto drop;
 	}
 
@@ -1023,7 +1023,7 @@ static netdev_tx_t geneve_xmit(struct sk_buff *skb, struct net_device *dev)
 		if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX))) {
 			netdev_dbg(dev, "no tunnel metadata\n");
 			dev_kfree_skb(skb);
-			DEV_STATS_INC(dev, tx_dropped);
+			dev_core_stats_tx_dropped_inc(dev);
 			return NETDEV_TX_OK;
 		}
 	} else {
-- 
2.39.2

Re: [PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[Simon Horman]

On Thu, Nov 07, 2024 at 12:41:44PM +0100, Guillaume Nault wrote:
> Use the core_stats rx_dropped counter to avoid the cost of atomic
> increments.
> 
> Signed-off-by: Guillaume Nault <gnault@redhat.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[Paolo Abeni]

On 11/7/24 12:41, Guillaume Nault wrote:
> Use the core_stats rx_dropped counter to avoid the cost of atomic
> increments.
> 
> Signed-off-by: Guillaume Nault <gnault@redhat.com>

It looks like other UDP tunnels devices could benefit from a similar
change (vxlan, bareudp). Would you mind to also touch them, to keep such
implementations aligned?

> ---
>  drivers/net/geneve.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
> index 2f29b1386b1c..671ca5260e92 100644
> --- a/drivers/net/geneve.c
> +++ b/drivers/net/geneve.c
> @@ -235,7 +235,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
>  					 vni_to_tunnel_id(gnvh->vni),
>  					 gnvh->opt_len * 4);
>  		if (!tun_dst) {
> -			DEV_STATS_INC(geneve->dev, rx_dropped);
> +			dev_core_stats_rx_dropped_inc(geneve->dev);

How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a
single percpu struct allocated x device (geneve already uses
NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly
memory usage lower.

/P

Re: [PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[Paolo Abeni]

On 11/12/24 11:53, Paolo Abeni wrote:
> On 11/7/24 12:41, Guillaume Nault wrote:
>> Use the core_stats rx_dropped counter to avoid the cost of atomic
>> increments.
>>
>> Signed-off-by: Guillaume Nault <gnault@redhat.com>
> 
> It looks like other UDP tunnels devices could benefit from a similar
> change (vxlan, bareudp). Would you mind to also touch them, to keep such
> implementations aligned?
> 
>> ---
>>  drivers/net/geneve.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c
>> index 2f29b1386b1c..671ca5260e92 100644
>> --- a/drivers/net/geneve.c
>> +++ b/drivers/net/geneve.c
>> @@ -235,7 +235,7 @@ static void geneve_rx(struct geneve_dev *geneve, struct geneve_sock *gs,
>>  					 vni_to_tunnel_id(gnvh->vni),
>>  					 gnvh->opt_len * 4);
>>  		if (!tun_dst) {
>> -			DEV_STATS_INC(geneve->dev, rx_dropped);
>> +			dev_core_stats_rx_dropped_inc(geneve->dev);
> 
> How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a
> single percpu struct allocated x device (geneve already uses
> NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly
> memory usage lower.

I was not aware of the previous discussion on this same topic:

https://lore.kernel.org/netdev/20240903113402.41d19129@kernel.org/

and I missed the previous change on bareudp.c

I still think that avoiding the double per-cpu traversal when fetching
the stats could be useful, especially on large multi-numa nodes systems.

I guess it's better to be consistent and keep geneve and bareudp
aligned. We can eventually consolidate the stats later.

/P

Re: [PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[Jakub Kicinski]

On Tue, 12 Nov 2024 17:53:36 +0100 Paolo Abeni wrote:
> > How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a
> > single percpu struct allocated x device (geneve already uses
> > NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly
> > memory usage lower.  
> 
> I was not aware of the previous discussion on this same topic:
> 
> https://lore.kernel.org/netdev/20240903113402.41d19129@kernel.org/
> 
> and I missed the previous change on bareudp.c
> 
> I still think that avoiding the double per-cpu traversal when fetching
> the stats could be useful, especially on large multi-numa nodes systems.
> 
> I guess it's better to be consistent and keep geneve and bareudp
> aligned. We can eventually consolidate the stats later.

We merged the bareudp changes... begrudgingly. You're the third
maintainer in a row to make a similar suggestion, which is pretty
strong signal that it's a better direction.

Re: [PATCH net-next] geneve: Use pcpu stats to update rx_dropped counter.

[Guillaume Nault]

On Tue, Nov 12, 2024 at 06:15:18PM -0800, Jakub Kicinski wrote:
> On Tue, 12 Nov 2024 17:53:36 +0100 Paolo Abeni wrote:
> > > How about switching to NETDEV_PCPU_STAT_DSTATS instead, so there is a
> > > single percpu struct allocated x device (geneve already uses
> > > NETDEV_PCPU_STAT_TSTATS): stats fetching will be faster, and possibly
> > > memory usage lower.  
> > 
> > I was not aware of the previous discussion on this same topic:
> > 
> > https://lore.kernel.org/netdev/20240903113402.41d19129@kernel.org/
> > 
> > and I missed the previous change on bareudp.c
> > 
> > I still think that avoiding the double per-cpu traversal when fetching
> > the stats could be useful, especially on large multi-numa nodes systems.
> > 
> > I guess it's better to be consistent and keep geneve and bareudp
> > aligned. We can eventually consolidate the stats later.
> 
> We merged the bareudp changes... begrudgingly. You're the third
> maintainer in a row to make a similar suggestion, which is pretty
> strong signal that it's a better direction.

No problem, I can work on DSTAT conversion.