Re: [PATCH net-next v2 3/3] skbuff: keep track of pp page when __skb_frag_ref() is called

On 2021/9/18 17:23, Ilias Apalodimas wrote:

[...]

[...]

quoted

quoted

quoted

quoted

quoted

quoted

IOW in skb_free_head() an we replace:

if (skb_pp_recycle(skb, head)) 
with
if (page->pp_magic & ~0x3UL) == PP_SIGNATURE)
and get rid of the 'bool recycle' argument in __skb_frag_unref()?

For the frag page of a skb, it seems ok to get rid of the 'bool recycle'
argument in __skb_frag_unref(), as __skb_frag_unref() and __skb_frag_ref()
is symmetrically called to put/get a page.

For the head page of a skb, we might need to make sure the head page
passed to __build_skb_around() meet below condition:
do pp_frag_count incrementing instead of _refcount incrementing when
the head page is not newly allocated and it is from page pool.
It seems hard to audit that?

Yea that seems a bit weird at least to me and I am not sure, it's the only
place we'll have to go and do that.

Yes, That is why I avoid changing the behavior of a head page for a skb.
In other word, maybe we should not track if head page for a skb is pp page
or not when the page'_refcount is incremented during network stack journey,
just treat it as normal page?
 

I am not sure I understand this.

I was saying only treat the head page of a skb as pp page when it is newly
allocated from page pool, if that page is reference-counted to build another
head page for another skb later, just treat it as normal page.

But the problem here is that a cloned/expanded SKB could trigger a race
when freeing the fragments.  That's why we reset the pp_recycle bit if
there's still references to the frags.  What does 'normal' page means here?
We'll have to at least unmap dma part.

'normal' page means non-pp page here. Maybe forget the above.

I read the code related to head page headling for a skb, it seems the
NAPI_GRO_FREE_STOLEN_HEAD and skb_head_frag_to_page_desc() case is just
fine as it is now when the page signature is used to identify a pp page
for the head page of a skb uniquely?

quoted

quoted

quoted

quoted

quoted

quoted

quoted

bit 0 of frag->bv_page is different way of indicatior for a pp page,
it is better we do not confuse with the page signature way. Using
a bit 0 may give us a free word in 'struct page' if we manage to
use skb->pp_recycle to indicate a head page of the skb uniquely, meaning
page->pp_magic can be used for future feature.

quoted

quoted

quoted

for pp_recycle right now?  __skb_frag_unref() in skb_shift() or
skb_try_coalesce() (the latter can probably be removed tbh).

If we decide to go with accurate indicator of a pp page, we just need
to make sure network stack use __skb_frag_unref() and __skb_frag_ref()
to put and get a page frag, the indicator checking need only done in
__skb_frag_unref() and __skb_frag_ref(), so the skb_shift() and
skb_try_coalesce() should be fine too.

quoted

quoted

Another way is to use the bit 0 of frag->bv_page ptr to indicate if a frag
page is from page pool.

Instead of the 'struct page' signature?  And the pp_recycle bit will
continue to exist?  

pp_recycle bit might only exist or is only used for the head page for the skb.
The bit 0 of frag->bv_page ptr can be used to indicate a frag page uniquely.
Doing a memcpying of shinfo or "*fragto = *fragfrom" automatically pass the
indicator to the new shinfo before doing a __skb_frag_ref(), and __skb_frag_ref()
will increment the _refcount or pp_frag_count according to the bit 0 of
frag->bv_page.

By the way, I also prototype the above idea, and it seems to work well too.

As long as no one else touches this, it's just another way of identifying a
page_pool allocated page.  But are we gaining by that?  Not using
virt_to_head_page() as stated above? But in that case you still need to
keep pp_recycle around. 

No, we do not need the pp_recycle, as long as the we make sure __skb_frag_ref()
is called after memcpying the shinfo or doing "*fragto = *fragfrom".

But we'll have to keep it for the skb head in this case.

As above, I am not really look into skb head case:)

Let me take a step back here, because I think we drifted a bit. 
The page signature was introduced in order to be able to identify skb
fragments. The problem was that you couldn't rely on the pp_recycle bit of
the skb head,  since fragments could come from anywhere.  So you use the
skb bit as a hint for skb frags, and you eventually decide using the page
signature.

So we got 3 options (Anything I've missed ?)
- try to remove pp_recycle bit, since the page signature is enough for the
  skb head and fragments.  That in my opinion is the cleanest option,  as
  long as we can prove there's no performance hit on the standard network
  path.

- Replace the page signature with frag->bv_page bit0.  In that case we
  still have to keep the pp_recycle bit,  but we do have an 'easier'
  indication that a skb frag comes from page_pool.  That's still pretty
  safe, since you now have unique identifiers for the skb and page
  fragments and you can be sure of their origin (page pool or not).
  What I am missing here, is what do we get out of this?  I think the
  advantage is not having to call virt_to_head_page() for frags ?

Not using the signature will free a word space in struct page for future
feature?

Yea that's another thing we gain,  but I am not sure how useful how this is
going to turn out.  

quoted

quoted

- Keep all of them(?) and use frag->bv_page bit0 similarly to pp_recycle
  bit?  I don't see much value on this one,  I am just keeping it here for
  completeness.

For safty and performance reason:
1. maybe we should move the pp_recycle bit from "struct sk_buff" to
   "struct skb_shared_info", and use it to only indicate if the head page of
   a skb is from page pool.

What's the safety or performance we gain out of this?  The only performance

safety is that we still have two ways to indicate a pp page.
the pp_recycle bit in  "struct skb_shared_info" or frag->bv_page bit0 tell
if we want to treat a page as pp page, the page signature checking is used
to tell if we if set those bits correctly?

Yea but in the long run we'll want the page signature.  So that's basically
(2) once we do that.

quoted

quoted

I can think of is the dirty cache line of the recycle bit we set to 0.
If we do move it to skb_shared)info we'll have to make sure it's on the
same cacheline as the ones we already change.

Yes, when we move the pp_recycle bit to skb_shared_info, that bit is only
set once, and we seems to not need to worry about skb doing cloning or
expanding as the it is part of head page(shinfo is part of head page).

quoted

quoted

2. The frag->bv_page bit0 is used to indicate if the frag page of a skb is
   from page pool, and modify __skb_frag_unref() and __skb_frag_ref() to keep
   track of it.

3. For safty or debugging reason, keep the page signature for now, and put a
   page signature WARN_ON checking in page pool to catch any misbehaviour?

If there is not bug showing up later, maybe we can free the page signature space
for other usage?

Yea that's essentially identical to (2) but we move the pp_recycle on the
skb_shared_info.  I'd really prefer getting rid of the pp_recycle entirely,

When also removing the pp_recycle for head page of a skb, it seems a little
risky as we are not sure when a not-newly-allocated pp page is called with
__build_skb_around() to build to head page?

Removing the pp_recyle, is only safe if we keep the page signature.  I was
suggesting we follow (1) first before starting moving things around.

I suppose (1) means the below, right:

- try to remove pp_recycle bit, since the page signature is enough for the
  skb head and fragments.  That in my opinion is the cleanest option,  as
  long as we can prove there's no performance hit on the standard network
  path.

It seems doable if my above analysis of head page headling for a skb does not
miss anything.

quoted

quoted

since it's the cleanest thing we can do in my head.  If we ever need an
extra 4/8 bytes in the future,  we can always go back and implement this.

Alexander/Jesper any additional thoughts?

Thanks
/Ilias
.