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

On Thu, Jul 08, 2021 at 08:41:08AM -0700, Alexander Duyck wrote:

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

quoted

On Thu, Jul 08, 2021 at 08:29:56AM -0700, Alexander Duyck wrote:

quoted

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

<snip>

quoted

quoted

quoted

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

I assume you mean specifically in the cloned case?

Yes. Even if we do it unconditionally we'll just loose non-cloned buffers from
the recycling.
I'll send a patch later today.

If you do it unconditionally you could leak DMA mappings since in the
non-cloned case we don't bother with releasing the shared info since
we just did a memcpy of it without the reference count tweaks. We have
to be really careful here. The idea is that we have to make exactly
one call to the __page_pool_put_page function for this page.

quoted

quoted

quoted

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.

Exactly

quoted

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.

Correct. I'll keep looking around in case there's something less fragile we
can do

That is the risk to this kind of thing. We have to make the call once
and only once and if we either miss it or call it too many times we
can introduce some serious issues.

And I fully agree. Let me fix the obvious one now and I'll have a closer
look on the recycling function it self. I can probably pick up the
"changed head"/expanded SKB in the generic recycling code and refuse to recycle
these packets. Then we'll just accept the fact that if those kind of
packets are freed last, we won't recycle.

Thanks, that was a very nice catch
/Ilias

Thanks.

- Alex