Message-ID: <20240826180921.560e112d@kernel.org>
Date: Mon, 26 Aug 2024 18:09:21 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>, <davem@...emloft.net>,
 <pabeni@...hat.com>, <edumazet@...gle.com>, <netdev@...r.kernel.org>,
 <przemyslaw.kitszel@...el.com>, <joshua.a.hay@...el.com>,
 <michal.kubiak@...el.com>, <nex.sw.ncis.osdt.itp.upstreaming@...el.com>
Subject: Re: [PATCH net-next v2 2/9] libeth: add common queue stats

On Fri, 23 Aug 2024 14:59:17 +0200 Alexander Lobakin wrote:
> >> For now it's done as a lib inside Intel folder, BUT if any other vendor
> >> would like to use this, that would be great and then we could move it
> >> level up or some of these APIs can go into the core.
> >> IOW depends on users.
> >>
> >> libie in contrary contains HW-specific code and will always be
> >> Intel-specific.  
> > 
> > Seems like an odd middle ground. If you believe it's generic finding
> > another driver shouldn't be hard.  
> 
> But it's up to the vendors right, I can't force them to use this code or
> just switch their driver to use it :D

It shouldn't be up to interpretation whether the library makes code
better. If it doesn't -- what's the point of the library. If it does
-- the other vendors better have a solid reason to push back.

> >> So you mean just open-code reads/writes per each field than to compress
> >> it that way?  
> > 
> > Yes. <rant> I don't understand why people try to be clever and
> > complicate stats reading for minor LoC saving (almost everyone,
> > including those working on fbnic). Just type the code in -- it 
> > makes maintaining it, grepping and adding a new stat without
> > remembering all the details soo much easier. </rant>  
> 
> In some cases, not this one, iterating over an array means way less
> object code than open-coded per-field assignment. Just try do that for
> 50 fields and you'll see.

Do you have numbers? How much binary code is 50 simple moves on x86?

> >> You mean to leave 0xffs for unsupported fields?  
> > 
> > Kinda of. But also I do mean to call out that you haven't read the doc
> > for the interface over which you're building an abstraction 😵‍💫️  
> 
> But I have...

Read, or saw it?

> >> I believe this nack is for generic Netlink stats, not the whole, right?
> >> In general, I wasn't sure about whether it would be better to leave
> >> Netlink stats per driver or write it in libeth, so I wanted to see
> >> opinions of others. I'm fine with either way.  
> > 
> > We (I?) keep pushing more and more stats into the generic definitions,
> > mostly as I find clear need for them in Meta's monitoring system.
> > My main concern is that if you hide the stats collecting in a library
> > it will make ensuring the consistency of the definition much harder,
> > and it will propagate the use of old APIs (dreaded ethtool -S) into new
> > drivers.  
> 
> But why should it propagate? 

I'm saying it shouldn't. The next NIC driver Intel (inevitably :))
creates should not report generic stuff via ethtool -S.
If it plugs in your library - it will.

> People who want to use these generic stats
> will read the code and see which fields are collected and exported, so
> that they realize that, for example, packets, bytes and all that stuff
> are already exported and and they need to export only driver-specific
> ones...
> 
> Or do you mean the thing that this code exports stuff like packets/bytes
> to ethtool -S apart from the NL stats as well? 

Yes, this.

> I'll be happy to remove that for basic Rx/Tx queues (and leave only
> those which don't exist yet in the NL stats) and when you introduce
> more fields to NL stats, removing more from ethtool -S in this
> library will be easy.

I don't scale to remembering 1 easy thing for each driver we have.

> But let's say what should we do with XDP Tx
> queues? They're invisible to rtnl as they are past real_num_tx_queues.

They go to ethtool -S today. It should be relatively easy to start
reporting them. I didn't add them because I don't have a clear use 
case at the moment.

> > If you have useful helpers that can be broadly applicable that's
> > great. This library as it stands will need a lot of work and a lot
> > of convincing to go in.  
> 
> Be more precise and I'll rework the stuff you find bad/confusing/etc,
> excluding the points we discuss above* as I already noted them. Just
> saying "need a lot of work and a lot of convincing" doesn't help much.
> You can take a driver as an example (fbnic?) and elaborate why you
> wouldn't use this lib to implement the stats there.

The other way around. Why would I? What is this layer of indirection
buying us? To add a statistic today I have to plug it into 4 places:
 - queue struct
 - the place it gets incremented
 - the place it gets reported
 - aggregation when ring is freed (BUILD_BUG_ON() will remind of this)

This is pretty intuitive and not much work at all. The complexity of
the library and how hard it is to read the 200 LoC macros by far
outweighs the 2 LoC I can save. Not to mention that I potentially
save space on all the stats I'm not implementing.

I'd argue that anything that the library can usefully do can be just
moved into the core.

> * implementing NL stats in drivers, not here; not exporting NL stats
> to ethtool -S
> 
> A driver wants to export a field which is missing in the lib? It's a
> couple lines to add it. Another driver doesn't support this field and
> you want it to still be 0xff there? Already noted and I'm already
> implementing a different model.

I think it will be very useful if you could step back and put on paper
what your goals are with this work, exactly.

Powered by blists - more mailing lists