From: Maciej Fijalkowski <maciej.fijalkow...@intel.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.fijalkow...@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.fijalkow...@intel.com> # lots of >>>> stuff >>>> Signed-off-by: Alexander Lobakin <aleksander.loba...@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? 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
