Re: [PATCH net-next RFC 1/2] page_pool: add page recycling support based on elevated refcnt

On Thu, Jul 8, 2021 at 8:17 AM Ilias Apalodimas
[off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

[...]

quoted

quoted

quoted

quoted

The above expectation is based on that the last user will always
call page_pool_put_full_page() in order to do the recycling or do
the resource cleanup(dma unmaping..etc).

As the skb_free_head() and skb_release_data() have both checked the
skb->pp_recycle to call the page_pool_put_full_page() if needed, I
think we are safe for most case, the one case I am not so sure above
is the rx zero copy, which seems to also bump up the refcnt before
mapping the page to user space, we might need to ensure rx zero copy
is not the last user of the page or if it is the last user, make sure
it calls page_pool_put_full_page() too.

Yes, but the skb->pp_recycle value is per skb, not per page. So my
concern is that carrying around that value can be problematic as there
are a number of possible cases where the pages might be
unintentionally recycled. All it would take is for a packet to get
cloned a few times and then somebody starts using pskb_expand_head and
you would have multiple cases, possibly simultaneously, of entities
trying to free the page. I just worry it opens us up to a number of
possible races.

Maybe I missde something, but I thought the cloned SKBs would never trigger
the recycling path, since they are protected by the atomic dataref check in
skb_release_data(). What am I missing?

Are you talking about the head frag? So normally a clone wouldn't
cause an issue because the head isn't changed. In the case of the
head_frag we should be safe since pskb_expand_head will just kmalloc
the new head and clears head_frag so it won't trigger
page_pool_return_skb_page on the head_frag since the dataref just goes
from 2 to 1.

The problem is that pskb_expand_head memcopies the page frags over and
takes a reference on the pages. At that point you would have two skbs
both pointing to the same set of pages and each one ready to call
page_pool_return_skb_page on the pages at any time and possibly racing
with the other.

Ok let me make sure I get the idea properly.
When pskb_expand_head is called, the new dataref will be 1, but the
head_frag will be set to 0, in which case the recycling code won't be
called for that skb.
So you are mostly worried about a race within the context of
pskb_expand_skb() between copying the frags, releasing the previous head
and preparing the new one (on a cloned skb)?

The race is between freeing the two skbs. So the original and the
clone w/ the expanded head will have separate instances of the page. I
am pretty certain there is a race if the two of them start trying to
free the page frags at the same time.

Right, I completely forgot calling __skb_frag_unref() before releasing the
head ...
You are right, this will be a race.  Let me go back to the original mail
thread and see what we can do

What do you think about resetting pp_recycle bit on pskb_expand_head()?

I assume you mean specifically in the cloned case?

If my memory serves me right Eric wanted that from the beginning. Then the
cloned/expanded SKB won't trigger the recycling.  If that skb hits the free
path first, we'll end up recycling the fragments eventually.  If the
original one goes first, we'll just unmap the page(s) and freeing the cloned
one will free all the remaining buffers.

I *think* that should be fine. Effectively what we are doing is making
it so that if the original skb is freed first the pages are released,
and if it is released after the clone/expended skb then it can be
recycled.

The issue is we have to maintain it so that there will be exactly one
caller of the recycling function for the pages. So any spot where we
are updating skb->head we will have to see if there is a clone and if
so we have to clear the pp_recycle flag on our skb so that it doesn't
try to recycle the page frags as well.