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

[Yunsheng Lin]

On 2021/9/18 17:23, Ilias Apalodimas wrote:
> [...]
> 

[...]

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

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

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