linux-kernel - Re: [PATCH net-next 03/16] libeth: add a couple of XDP helpers (libeth

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 for Android: free password hash cracker in your pocket

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

Message-ID: <fc94190c-3ea1-4034-a65d-7b5e8684812d@intel.com>
Date: Mon, 17 Mar 2025 16:26:04 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, Michal Kubiak
	<michal.kubiak@...el.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
	"Jesper Dangaard Brouer" <hawk@...nel.org>, John Fastabend
	<john.fastabend@...il.com>, Simon Horman <horms@...nel.org>,
	<bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next 03/16] libeth: add a couple of XDP helpers
 (libeth_xdp)

From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Date: Tue, 11 Mar 2025 15:05:38 +0100

> On Wed, Mar 05, 2025 at 05:21:19PM +0100, Alexander Lobakin wrote:
>> "Couple" is a bit humbly... Add the following functionality to libeth:
>>
>> * XDP shared queues managing
>> * XDP_TX bulk sending infra
>> * .ndo_xdp_xmit() infra
>> * adding buffers to &xdp_buff
>> * running XDP prog and managing its verdict
>> * completing XDP Tx buffers
>>
>> Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com> # lots of stuff
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> 
> Patch is really big and I'm not sure how to trim this TBH to make my
> comments bearable. I know this is highly optimized but it's rather hard to
> follow with all of the callbacks, defines/aligns and whatnot. Any chance
> to chop this commit a bit?

Sometimes "highly optimized" code means "not really readable". See
PeterZ's code :D I mean, I'm not able to write it to look more readable
without hurting object code or not provoking code duplications. Maybe
it's an art which I don't possess.
I tried by best and left the documentation, even with pseudo-examples.
Sorry if it doesn't help =\

> 
> Timers and locking logic could be pulled out to separate patches I think.
> You don't ever say what improvement gave you the __LIBETH_WORD_ACCESS
> approach. You've put a lot of thought onto this work and I feel like this

I don't record/remember all of the perf changes. Couple percent for
sure. Plus lighter object code.
I can recall ~ -50-60 bytes in libeth_xdp_process_buff(), even though
there's only 1 64-bit write replacing 2 32-bit writes. When there's a
lot, like descriptor filling, it was 100+ bytes off, esp. when unrolling.

> is not explained/described thoroughly. What would be nice to see is to
> have this in the separate commit as well with a comment like 'this gave me
> +X% performance boost on Y workload'. That would be probably a non-zero
> effort to restructure it but generally while jumping back and forth

Yeah it would be quite a big. I had a bit of hard time splitting it into
2 commits (XDP and XSk) from one, that request would cost a bunch more.

Dunno if it would make sense at all? Defines, alignments etc, won't go
away. Same for "head-scratching moments". Moreover, sometimes splitting
the code borns more questions as it feels incomplete until the last
patch and then there'll be a train of replies like "this will be
added/changes in patch number X", which I don't like to do :s
I mean, I would like to not sacrifice time splitting it only for the
sake of split, depends on how critical this is and what it would give.

> through this code I had a lot of head-scratching moments.
> 
>> ---
>>  drivers/net/ethernet/intel/libeth/Kconfig  |   10 +-
>>  drivers/net/ethernet/intel/libeth/Makefile |    7 +-
>>  include/net/libeth/types.h                 |  106 +-
>>  drivers/net/ethernet/intel/libeth/priv.h   |   26 +
>>  include/net/libeth/tx.h                    |   30 +-
>>  include/net/libeth/xdp.h                   | 1827 ++++++++++++++++++++
>>  drivers/net/ethernet/intel/libeth/tx.c     |   38 +
>>  drivers/net/ethernet/intel/libeth/xdp.c    |  431 +++++
>>  8 files changed, 2467 insertions(+), 8 deletions(-)
>>  create mode 100644 drivers/net/ethernet/intel/libeth/priv.h
>>  create mode 100644 include/net/libeth/xdp.h
>>  create mode 100644 drivers/net/ethernet/intel/libeth/tx.c
>>  create mode 100644 drivers/net/ethernet/intel/libeth/xdp.c

Thanks,
Olek