Message-ID: <247bdd01-830e-434a-ae38-0c68fcc62051@intel.com>
Date: Tue, 1 Apr 2025 15:11:50 +0200
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: Wed, 19 Mar 2025 17:19:44 +0100

> On Mon, Mar 17, 2025 at 04:26:04PM +0100, Alexander Lobakin wrote:
>> 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 =\
> 
> Do you mean doxygen descriptions or what kind of documentation - I must be
> missing something?

Yes and not only, I meant all of the comments. There are even some
pseudo-code example blocks for complicated stuff.

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

I haven't read all of them yet, a bit of patience. Of course I didn't
cut it to not address at all :D

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

Makes sense, but unfortunately this won't cut off a lot. But I'll try,
to the degree where I'd need to provide stubs.

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

Also correct.

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

I'll process the rest of your review really soon.

Thanks,
Olek

Powered by blists - more mailing lists