Re: [Intel-wired-lan] [PATCH RFC net-next v4 3/9] iavf: drop page splitting and recycling

[Alexander Duyck]

On Thu, Jul 6, 2023 at 9:46 AM Alexander Lobakin
<aleksander.lobakin@xxxxxxxxx> wrote:
>
> From: Alexander Duyck <alexander.duyck@xxxxxxxxx>
> Date: Thu, 6 Jul 2023 07:47:03 -0700
>
> > On Wed, Jul 5, 2023 at 8:57 AM Alexander Lobakin
> > <aleksander.lobakin@xxxxxxxxx> 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.

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