Message-ID: <b5271512-f4bd-434c-858e-9f16fe707a5a@intel.com>
Date: Fri, 23 Aug 2024 14:59:17 +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: Thu, 22 Aug 2024 16:17:18 -0700

> On Thu, 22 Aug 2024 17:13:57 +0200 Alexander Lobakin wrote:
>>> BTW for Intel? Or you want this to be part of the core?
>>> I thought Intel, but you should tell us if you have broader plans.  
>>
>> 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

[...]

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

> 
>> Sure, that would be no problem. Object code doesn't even
>> change (my first approach was per-field).
>>
>>>> +									      \
>>>> +static void								      \
>>>> +libeth_get_##pfx##_base_stats(const struct net_device *dev,		      \
>>>> +			      struct netdev_queue_stats_##gpfx *stats)	      \
>>>> +{									      \
>>>> +	const struct libeth_netdev_priv *priv = netdev_priv(dev);	      \
>>>> +	u64 *raw = (u64 *)stats;					      \
>>>> +									      \
>>>> +	memset(stats, 0, sizeof(*(stats)));				      \  
>>>
>>> Have you read the docs for any of the recent stats APIs?  
>>
>> 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...

> 
>>> Nack. Just implement the APIs in the driver, this does not seem like 
>>> a sane starting point _at all_. You're going to waste more time coming
>>> up with such abstraction than you'd save implementing it for 10 drivers.  
>>
>> 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? 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? 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.
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.

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

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

Thanks,
Olek

Powered by blists - more mailing lists