netdev - Re: [PATCH net-next] geneve: Use pcpu stats to update rx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a6bd2ee8-c732-4922-9e7c-ae89a1ad8056@redhat.com>
Date: Tue, 12 Nov 2024 17:53:36 +0100
From: Paolo Abeni <pabeni@...hat.com>
To: Guillaume Nault <gnault@...hat.com>, David Miller <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, Eric Dumazet <edumazet@...gle.com>
Cc: netdev@...r.kernel.org, Simon Horman <horms@...nel.org>,
 Andrew Lunn <andrew+netdev@...n.ch>
Subject: Re: [PATCH net-next] geneve: Use pcpu stats to update rx_dropped
 counter.

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@...hat.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