netdev - Re: [PATCH net-next v2 2/9] libeth: add common queue stats

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <bf736d48-813e-4bdd-b33f-23bb1c7d4c0a@intel.com>
Date: Thu, 29 Aug 2024 14:01:06 +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: Wed, 28 Aug 2024 13:22:35 -0700

> On Wed, 28 Aug 2024 17:06:17 +0200 Alexander Lobakin wrote:
>>>> 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.  
>>>
>>> You don't understand. I built some infra over the last 3 years.
>>> You didn't bother reading it. Now you pop our own thing to the side,
>>> extending ethtool -S which is _unusable_ in a multi-vendor, production
>>> environment.  
>>
>> I read everything at the time you introduced it. I remember Ethernet
>> standard stats, rmon, per-queue generic stats etc. I respect it and I
>> like it.
>> So just let me repeat my question so that all misunderstandings are
>> gone: did I get it correctly that instead of adding Ethtool stats, I
>> need to add new fields to the per-queue Netlink stats? I clearly don't
>> have any issues with that and I'll be happy to drop Ethtool stats from
>> the lib at all.
> 
> That's half of it, the other half is excess of macro magic.
> 
>> (except XDP stats, they still go to ethtool -S for now? Or should I try
>> making them generic as well?)
> 
> Either way is fine by me. You may want to float the XDP stats first as
> a smaller series, just extending the spec and exposing from some driver
> already implementing qstat. In case someone else does object.

I think I'll do that the following way. To not delay this series and XDP
for idpf in general, I'll drop these stats for now, leaving only onstack
containers (they will be used in libeth_xdp etc.), without the macro
magic. But at the same time I'll work on extending the NL queue stats,
incl. XDP ones, and send them separately when they're done. Would that
be fine?

> 
>>>> * 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  
>>>
>>>  21 files changed, 1634 insertions(+), 1002 deletions(-)  
>>
>> Did you notice my "in the last commit"?
> 
> I may not have. But as you said, most drivers end up with some level of
> boilerplate around stats. So unless the stuff is used by more than one
> driver, and the savings are realized, the argument about saving LoC has
> to be heavily discounted. Queue xkcd 927. I should reiterate that 
> I don't think LoC saving is a strong argument in the first place.

Yes, I got that. In general, you're right this hurts readability a lot.

Thanks,
Olek