Date: Tue, 27 Feb 2024 10:09:51 -0800
From: Stanislav Fomichev <sdf@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, amritha.nambiar@...el.com, danielj@...dia.com, 
	mst@...hat.com, michael.chan@...adcom.com, vadim.fedorenko@...ux.dev
Subject: Re: [PATCH net-next 1/3] netdev: add per-queue statistics

On 02/27, Jakub Kicinski wrote:
> On Mon, 26 Feb 2024 19:37:04 -0800 Stanislav Fomichev wrote:
> > On 02/26, Jakub Kicinski wrote:
> > > On Mon, 26 Feb 2024 13:35:34 -0800 Stanislav Fomichev wrote:  
> > > > IIUC, in order to get netdev-scoped stats in v1 (vs rfc) is to not set
> > > > stats-scope, right? Any reason we dropped the explicit netdev entry?
> > > > It seems more robust with a separate entry and removes the ambiguity about
> > > > which stats we're querying.  
> > > 
> > > The change is because I switched from enum to flags.
> > > 
> > > I'm not 100% sure which one is going to cause fewer issues down
> > > the line. It's a question of whether the next scope we add will 
> > > be disjoint with or subdividing previous scopes.
> > > 
> > > I think only subdividing previous scopes makes sense. If we were 
> > > to add "stats per NAPI" (bad example) or "per buffer pool" or IDK what
> > > other thing -- we should expose that as a new netlink command, not mix 
> > > it with the queues.
> > > 
> > > The expectation is that scopes will be extended with hw vs sw, or
> > > per-CPU (e.g. page pool recycling). In which case we'll want flags,
> > > so that we can combine them -- ask for HW stats for a queue or hw
> > > stats for the entire netdev.
> > > 
> > > Perhaps I should rename stats -> queue-stats to make this more explicit?
> > > 
> > > The initial version I wrote could iterate both over NAPIs and
> > > queues. This could be helpful to some drivers - but I realized that it
> > > would lead to rather painful user experience (does the driver maintain
> > > stats per NAPI or per queue?) and tricky implementation of the device
> > > level sum (device stats = Sum(queue) or Sum(queue) + Sum(NAPI)??)  
> > 
> > Yeah, same, not sure. The flags may be more flexible but a bit harder
> > wrt discoverability. Assuming a somewhat ignorant spec reader/user,
> > it might be hard to say which flags makes sense to combine and which isn't.
> > Or, I guess, we can try to document it?
> 
> We're talking about driver API here, so document and enforce in code
> review :) But fundamentally, I don't think we should be turning this op
> into a mux for all sort of stats. We can have 64k ops in the family.
> 
> > For HW vs SW, do you think it makes sense to expose it as a scope?
> > Why not have something like 'rx-packets' and 'hw-rx-packets'?
> 
> I had that in one of the WIP versions but (a) a lot of the stats can 
> be maintained by either device or the driver, so we'd end up with a hw-
> flavor for most of the entries, and (b) 90% of the time the user will
> not care whether it's the HW or SW that counted the bytes, or GSO
> segments. Similarly to how most of the users will not care about
> per-queue breakdown, TBH, which made me think that from user
> perspective both queue and hw vs sw are just a form of detailed
> breakdown. Majority will dump the combined sw|hw stats for the device.
> 
> I could be wrong.
> 
> > Maybe, as you're suggesting, we should rename stats to queue-states
> > and drop the score for now? When the time comes to add hw counters,
> > we can revisit. For total netdev stats, we can ask the user to aggregate
> > the per-queue ones?
> 
> I'd keep the scope, and ability to show the device level aggregation.
> There are drivers (bnxt, off the top of my head, but I feel like there's
> more) which stash the counters when queues get freed. Without the device
> level aggregation we'd need to expose that as "no queue" or "history"
> or "delta" etc stats. I think that's uglier that showing the sum, which
> is what user will care about 99% of the time.
> 
> It'd be a pure rename.

Ack, sounds fair!

Powered by blists - more mailing lists