Messages in this thread | | | Date | Wed, 31 May 2023 17:22:37 +0200 | Subject | Re: [PATCH net-next v2 03/12] iavf: optimize Rx buffer allocation a bunch | From | Alexander Lobakin <> |
| |
From: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Date: Wed, 31 May 2023 13:14:12 +0200
> On Tue, May 30, 2023 at 09:18:40AM -0700, Alexander H Duyck wrote: > > FWIW I agree with what Alex is saying over here.
There are 2 Alexes, "choose wisely" :P
> >> On Thu, 2023-05-25 at 14:57 +0200, Alexander Lobakin wrote: >>> The Rx hotpath code of IAVF is not well-optimized TBH. Before doing any >>> further buffer model changes, shake it up a bit. Notably: >>> >>> 1. Cache more variables on the stack. >>> DMA device, Rx page size, NTC -- these are the most common things >>> used all throughout the hotpath, often in loops on each iteration. >>> Instead of fetching (or even calculating, as with the page size) them >>> from the ring all the time, cache them on the stack at the beginning >>> of the NAPI polling callback. NTC will be written back at the end, >>> the rest are used read-only, so no sync needed. >> >> The advantage of this is going to vary based on the attribute. One of >> the reasons why I left most of this on the ring is because the section >> of the ring most of these variables were meant to be read-mostly and >> shouldn't have resulted in any additional overhead versus accessing >> them from the stack. > > I believe it depends on ring struct layout which vary across our drivers, > no? On ice using making more usage of stack as described above improved > perf.
It's +/- the same, most layout changes usually come with us moving stuff around to optimize paddings and cachelines lol. Here's the same as with ice, I don't think it's driver specific to get some positive results from shortcutting more hotties. The sole time I was surprised is when you were getting worse results storing xdp_buff on the stack vs on the ring :D
> >> >>> 2. Don't move the recycled buffers around the ring. >>> The idea of passing the page of the right-now-recycled-buffer to a >>> different buffer, in this case, the first one that needs to be >>> allocated, moreover, on each new frame, is fundamentally wrong. It >>> involves a few o' fetches, branches and then writes (and one Rx >>> buffer struct is at least 32 bytes) where they're completely unneeded, >>> but gives no good -- the result is the same as if we'd recycle it >>> inplace, at the same position where it was used. So drop this and let >>> the main refilling function take care of all the buffers, which were >>> processed and now need to be recycled/refilled. >> >> The next_to_alloc logic was put in place to deal with systems that are >> experiencing memory issues. Specifically what can end up happening is >> that the ring can stall due to failing memory allocations and the >> memory can get stuck on the ring. For that reason we were essentially >> defragmenting the buffers when we started suffering memory pressure so >> that they could be reusued and/or freed following immediate use. >> >> Basically what you are trading off is some exception handling for >> performance by removing it. > > With all of the mix of the changes this patch carries, I find it hard to > follow from description which parts of diff I should be looking at.
Huge piece removed before add_rx_frag_blah.
> >> >>> 3. Don't allocate with %GPF_ATOMIC on ifup. >>> This involved introducing the @gfp parameter to a couple functions. >>> Doesn't change anything for Rx -> softirq.
[...]
>>> + up to 2% performance. >>> >> >> What is the test you saw the 2% performance improvement in? Is it >> something XDP related or a full stack test? > > +1, can you say more about that measurement?
My prev reply to Alex.
> >> >>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> >> >> Also one thing I am not a huge fan of is a patch that is really a >> patchset onto itself. With all 6 items called out here I would have >> preferred to see this as 6 patches as it would have been easier to >> review. > > +1
+1 :D
[...]
>>> /* if we are the last buffer then there is nothing else to do */ >>> #define IAVF_RXD_EOF BIT(IAVF_RX_DESC_STATUS_EOF_SHIFT) >>> if (likely(iavf_test_staterr(rx_desc, IAVF_RXD_EOF))) >> >> You may want to see if you can get rid of this function entirely, >> perhaps you do in a later patch. This function was added for ixgbe back >> in the day to allow us to place the skb back in the ring for the RSC >> based workloads where we had to deal with interleaved frames in the Rx >> path. >> >> For example, one question here would be why are we passing skb? It >> isn't used as far as I can tell. >> > this was used back when skb was stored within the Rx buffer and now we > just store skb on Rx ring struct, so good catch, this arg is redundant.
Also prev reply. I'm removing it later in the series hehe.
> > I'll go and take a look at code on v3.
No changes apart fixing OcteonTX2 compilation =\
Thanks, Olek
| |