Message-ID: <ee5eca5f-d545-4836-8775-c5f425adf1ed@intel.com>
Date: Tue, 27 Aug 2024 17:31:55 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
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

From: Jakub Kicinski <kuba@...nel.org>
Date: Mon, 26 Aug 2024 18:09:21 -0700

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

Potential reasons to push back (by "we" I mean some vendor X):

* we want our names for Ethtool stats, not yours ("txq_X" instead of
  "sqX" and so on), we have scripts which already parse our names blah
* we have our own infra and we just don't want / have time/resources to
  refactor and then test since it's not something critical

> 
>>>> 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?

	for (u32 i = 0; i < 50; i++)
		structX->field[i] = something * i;

open-coding this loop to assign 50 fields manually gives me +483 bytes
of object code on -O2.

But these two lines scale better than adding a new assignment for each
new field (and then forget to do that for some field and debug why the
stats are incorrect).

> 
>>>> 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?

Something in between these two, but I'll reread since you're insisting.

> 
>>>> 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 :))

FYI I already nack inside Intel any new drivers since I was promised
that each next generation will be based on top of idpf.

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

This was my mistake as generic per-queue NL stats went into the kernel
pretty recently... Removing Ethtool counterparts anyway.

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

Introducing a new field is adding 1 line with its name to the macro
since everything else gets expanded from these macros anyway.

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

The same as for regular Tx: debugging, imbalance etc.

> 
>>> 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

See below regarding "2 LoC"

> save space on all the stats I'm not implementing.

The stats I introduced here are supported by most, if not every, modern
NIC drivers. Not supporting header split or HW GRO will save you 16
bytes on the queue struct which I don't think is a game changer.

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

I don't say anything from the lib can be easily moved to the core. The
library was started to reduce copy-paste between Intel drivers and
resides right now inside intel/ folder.
But since this lib doesn't have any HW-specific code I wouldn't mind if
any other vendor would start using it, and I don't necessarily mean
stats here, but anything he might want to.

> 
>> * 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.

My goals:

* reduce boilerplate code in drivers: declaring stats structures,
Ethtool stats names, all these collecting, aggregating etc etc, you see
in the last commit of the series how many LoCs get deleted from idpf,
+/- the same amount would be removed from any other driver

* reduce the time people debug and fix bugs in stats since it will be
just in one place, not in each driver

* have more consistent names in ethtool -S

* have more consistent stats sets in drivers since even within Intel
drivers it's a bit of a mess which stats are exported etc.

Most of your pushback here sounds like if I would try to introduce this
in the core code, but I don't do that here. This infra saves a lot of
locs and time when used in the Intel drivers and it would be totally
fine for me if some pieces of the lib goes into the core, but these
stats don't.
I didn't declare anywhere that everyone must use it or that it's core
code, do you want me to change this MODULE_DESCRIPTION()?

Thanks,
Olek

Powered by blists - more mailing lists