netdev - Re: [PATCH net] bareudp: Fix device stats updates.

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 for Android: free password hash cracker in your pocket

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

Message-ID: <Ztie4AoXc9PhLi5w@debian>
Date: Wed, 4 Sep 2024 19:54:40 +0200
From: Guillaume Nault <gnault@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: David Miller <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>,
	Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
	Martin Varghese <martin.varghese@...ia.com>,
	Willem de Bruijn <willemb@...gle.com>,
	David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH net] bareudp: Fix device stats updates.

[Adding David Ahern for the vrf/dstats discussion]

On Wed, Sep 04, 2024 at 07:57:32AM -0700, Jakub Kicinski wrote:
> On Wed, 4 Sep 2024 14:29:44 +0200 Guillaume Nault wrote:
> > > The driver already uses struct pcpu_sw_netstats, would it make sense to
> > > bump it up to struct pcpu_dstats and have per CPU rx drops as well?
> > 
> > Long term, I was considering moving bareudp to use dev->tstats for
> > packets/bytes and dev->core_stats for drops. It looks like dev->dstats
> > is only used for VRF, so I didn't consider it.
> 
> Right, d stands for dummy so I guess they also were used by dummy
> at some stage? Mostly I think it's a matter of the other stats being
> less recent.

Looks like dummy had its own dstats, yes. But those dstats were really
like the current lstats (packets and bytes counters, nothing for
drops). Dummy was later converted to lstats by commit 4a43b1f96b1d
("net: dummy: use standard dev_lstats_add() and dev_lstats_read()").

The dstats we have now really come from vrf (different counters for tx
and rx and counters for packet drops), which had its own implementation
at that time.

My understanding is that vrf implemented its own dstats in order to
have per-cpu counters for regular bytes/packets counters and also for
packet drops.

But when vrf's dstats got moved to the core (commits
79e0c5be8c73 ("net, vrf: Move dstats structure to core") and
34d21de99cea ("net: Move {l,t,d}stats allocation to core and convert
veth & vrf")), the networking core had caught up and had also gained
support for pcpu drop counters (commit 625788b58445 ("net: add per-cpu
storage and net->core_stats")).

In this context, I feel that dstats is now just a mix of tstats and
core_stats.

> > Should we favour dev->dstats for tunnels instead of combining ->tstats
> > and ->core_stats? (vxlan uses the later for example).
> 
> Seems reasonable to me. Not important enough to convert existing
> drivers, maybe, unless someone sees contention. But in new code,
> or if we're touching the relevant lines I reckon we should consider it?

Given that we now have pcpu stats for packet drops anyway, what does
dstats bring compared to tstats?

Shouldn't we go the other way around and convert vrf to tstats and
core_stats? Then we could drop dstats entirely.

Back to bareudp, for the moment, I'd prefer to convert it to tstats
rather than dstats. The reason is that vxlan (and geneve to a lesser
extent) use tstats and I'd like to ease potential future code
consolidation between those three modules.

> No strong feelings tho, LMK if you want to send v2 or keep this patch
> as is.

I'd prefer to have this patch merged as is in -net. I have other
patches pending that have to update stats and I'd like to do that
correctly (that is, in a non-racy way) and consistently with existing
code. I feel that converting bareudp to either tstats or dstats is
something for net-next.

After -net will merge into net-next, I'll can convert bareudp to either
dstats or tstats, depending on the outcome of this conversation.