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

[Alexander Lobakin]

From: Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx>
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@xxxxxxxxx> # lots of stuff
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> 
> 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