Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 10 Jul 2023 15:13:22 +0200 | Subject | Re: [Intel-wired-lan] [PATCH RFC net-next v4 3/9] iavf: drop page splitting and recycling | From | Alexander Lobakin <> |
| |
From: Alexander Duyck <alexander.duyck@gmail.com> Date: Thu, 6 Jul 2023 10:06:29 -0700
> On Thu, Jul 6, 2023 at 9:46 AM Alexander Lobakin > <aleksander.lobakin@intel.com> wrote: >> >> From: Alexander Duyck <alexander.duyck@gmail.com> >> Date: Thu, 6 Jul 2023 07:47:03 -0700 >> >>> On Wed, Jul 5, 2023 at 8:57 AM Alexander Lobakin >>> <aleksander.lobakin@intel.com> wrote: > > [...] > >>>> @@ -1431,15 +1303,18 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget) >>>> else >>>> skb = iavf_build_skb(rx_ring, rx_buffer, size); >>>> >>>> + iavf_put_rx_buffer(rx_ring, rx_buffer); >>>> + >>> >>> This should stay below where it was. >> >> Wait-wait-wait. >> >> if (!skb) break breaks the loop. put_rx_buffer() unmaps the page. >> So in order to do the first, you need to do the second to avoid leaks. >> Or you meant "why unmapping and freeing if we fail, just leave it in >> place"? To make it easier to switch to Page Pool. > > Specifically you don't want to be unmapping and freeing this page > until after the !skb check. The problem is if skb is NULL the skb > allocation failed and so processing of Rx is meant to stop in place > without removing the page. It is where we will resume on the next pass > assuming memory has been freed that can then be used. The problem is > the skb allocation, not the page. We used to do the skb allocation > before we would acquire the buffer, but with XDP there are cases where > we aren't supposed to allocate it so it got moved to after which > causes this confusion. > >>> >>>> /* exit if we failed to retrieve a buffer */ >>>> if (!skb) { >>>> rx_ring->rx_stats.alloc_buff_failed++; >>>> - if (rx_buffer && size) >>>> - rx_buffer->pagecnt_bias++; >>>> + __free_pages(rx_buffer->page, >>>> + iavf_rx_pg_order(rx_ring)); >>>> + rx_buffer->page = NULL; >>>> break; >>>> } >>> >>> This code was undoing the iavf_get_rx_buffer decrement of pagecnt_bias >>> and then bailing since we have halted forward progress due to an skb >>> allocation failure. As such we should just be removing the if >>> statement and the increment of pagecnt_bias. > > The key bit here is the allocation failure is the reason why we halted > processing. So the page contains valid data and should not be freed. > We just need to leave it in place and wait for an allocation to > succeed and then we can resume processing.
Aaah, okay, you want to try once again with the same buffer next time. I see. Makes sense!
> >>> >>>> >>>> - iavf_put_rx_buffer(rx_ring, rx_buffer); >>>> + rx_buffer->page = NULL; >>>> cleaned_count++; >>>> >>>> if (iavf_is_non_eop(rx_ring, rx_desc, skb)) >>> >>> If iavf_put_rx_buffer just does the unmap and assignment of NULL then >>> it could just be left here as is. >> >> I guess those two are tied with the one above. > > Yeah, the iavf_put_rx_buffer should be left down here. Thanks, Olek
| ![\](/images/icornerr.gif) |