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

[Alexander Lobakin]

From: Alexander Duyck <alexander.duyck@xxxxxxxxx>
Date: Thu, 6 Jul 2023 10:06:29 -0700

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

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