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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Mon, Mar 17, 2025 at 04:26:04PM +0100, Alexander Lobakin wrote:
> From: Maciej Fijalkowski <maciej.fijalkowski@intel.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@intel.com> # lots of stuff
> >> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.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 =\

Do you mean doxygen descriptions or what kind of documentation - I must be
missing something?

You cut out all of the stuff I asked about in this review - are you going
to address any of those or what should I expect?

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

I just wanted to hint that it felt like this feature could be stripped
from this huge patch and then on of top of it you would have it as 'this
is my awesome feature that gave me X improvement, eat it'. As I tried to
say any small pullouts would make it easier to comprehend, at least from
reviewer's POV...

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

maybe ask yourself this - if you add a new ethernet driver, are you adding
this in a single commit or do you send a patch set that is structured in
some degree:) I have a feeling that this patch could be converted to a
patch set where each bullet from commit message is a separate patch.

> 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 agree here it's a tradeoff which given that user of lib is driver would
be tricky to split properly.

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

Not sure what to say here. Your time dedicated for making this work easier
to swallow means less time dedicated for going through this by reviewer.

I like the end result though and how driver side looks like when using
this lib. Sorry for trying to understand the internals:)

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