Re: [PATCH net-next v22 07/14] mm: page_frag: some minor refactoring before adding new API

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/10/20 23:45, Alexander Duyck wrote:

...

> 
>>>
>>>
>>>> @@ -132,8 +156,6 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>>>>                          return NULL;
>>>>                  }
>>>>
>>>> -               page = encoded_page_decode_page(encoded_page);
>>>> -
>>>>                  if (!page_ref_sub_and_test(page, nc->pagecnt_bias))
>>>>                          goto refill;
>>>>
>>>> @@ -148,15 +170,17 @@ void *__page_frag_alloc_align(struct page_frag_cache *nc,
>>>>
>>>>                  /* reset page count bias and offset to start of new frag */
>>>>                  nc->pagecnt_bias = PAGE_FRAG_CACHE_MAX_SIZE + 1;
>>>> +               nc->offset = 0;
>>>>                  offset = 0;
>>>>          }
>>>>
>>>> -       nc->pagecnt_bias--;
>>>> -       nc->offset = offset + fragsz;
>>>> +       pfrag->page = page;
>>>> +       pfrag->offset = offset;
>>>> +       pfrag->size = size - offset;
>>>
>>> I really think we should still be moving the nc->offset forward at
>>> least with each allocation. It seems like you end up doing two flavors
>>> of commit, one with and one without the decrement of the bias. So I
>>> would be okay with that being pulled out into some separate logic to
>>> avoid the extra increment in the case of merging the pages. However in
>>> both cases you need to move the offset, so I would recommend keeping
>>> that bit there as it would allow us to essentially call this multiple
>>> times without having to do a commit in between to keep the offset
>>> correct. With that your commit logic only has to verify nothing
>>> changes out from underneath us and then update the pagecnt_bias if
>>> needed.
>>
>> The problem is that we don't really know how much the nc->offset
>> need to be moved forward to and the caller needs the original offset
>> for skb_fill_page_desc() related calling when prepare API is used as
>> an example in 'Preparation & committing API' section of patch 13:
> 
> The thing is you really have 2 different APIs. You have one you were
> doing which was a alloc/abort approach and another that is a
> probe/commit approach. I think for the probe/commit you could probably
> get away with using an "alloc" type approach with a size of 0 which
> would correctly set the start of your offset and then you would need
> to update it later once you know the total size for your commit. For

It seems there are some issues with the above approach as below as I
can see for now:
1. when nc->encoded_page is 0, Calling the "alloc" type API with
fragsz being zero may still allocate a new page from allocator, which
seems to against the purpose of probe API, right?

2. It doesn't allow the caller to specify a fragsz for probing, instead
   it rely on the caller to check if the size of probed fragment is bigger
   enough for its use case.

> the probe/commit we could use the nc->offset as a kind of cookie to
> verify we are working with the expected page and offset.

I am not sure if I am following the above, but I should mention that
nc->offset is not updated for prepare/probe API because the original
offset might be used for calculating the truesize of the fragment
when commit API is called, and the offset returned to the caller might
need to be updated according to alignment requirement, so I am not sure
how nc->offset can be used to verify the exact offset here.

If it is realy about catching misuse of the page_frag API, it might be
better to add something like nc->last_offset to record the offset of
allocated fragment under some config like PAGE_FRAG_DEBUG, as there
are other ways that the caller might mess up here like messing up the
allocation context assumtion.

> 
> For the alloc/abort it would be something similar but more the
> reverse. With that one we would need to have the size + offset and
> then verify the current offset is equal to that before we allow
> reverting the previous nc->offset update. The current patch set is a
> bit too permissive on the abort in my opinion and should be verifying
> that we are updating the correct offset.

I am not sure if I understand what is your idea about how to do an
exact verifying for abort API here.
For abort API, it seems we can do an exact verifying if the 'va' is
also passed to the abort API as the nc->offset is already updated,
something like below:

static inline void page_frag_alloc_abort(struct page_frag_cache *nc,
					 void *va, unsigned int fragsz)
{
        VM_BUG_ON((nc->offset - fragsz) !=
		  (encoded_page_decode_virt(nc->encoded_page) - va));

        nc->pagecnt_bias++;
        nc->offset -= fragsz;
}

But it also might mean we need to put page_frag_alloc_abort() in
page_frag_cache.c instead of a inline helper in page_frag_cache.h, as
the encoded_page_decode_virt() is a static function in c file. Or put
encoded_page_decode_virt() in the h file.