Re: [PATCH net-next v1] page_pool: check for dma_sync_size earlier

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2024/10/15 15:43, Ilias Apalodimas wrote:
> Hi Yunsheng,
> 
> On Mon, 14 Oct 2024 at 15:39, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2024/10/14 14:35, Furong Xu wrote:
>>> Hi Yunsheng,
>>>
>>> On Sat, 12 Oct 2024 14:14:41 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>
>>>> I would prefer to add a new api to do that, as it makes the semantic
>>>> more obvious and may enable removing some checking in the future.
>>>>
>>>> And we may need to disable this 'feature' for frag relate API for now,
>>>> as currently there may be multi callings to page_pool_put_netmem() for
>>>> the same page, and dma_sync is only done for the last one, which means
>>>> it might cause some problem for those usecases when using frag API.
>>>
>>> I am not an expert on page_pool.
>>> So would you mind sending a new patch to add a non-dma-sync version of
>>> page_pool_put_page() and CC it to me?
>>
>> As I have at least two patchsets pending for the net-next, which seems
>> it might take a while, so it might take a while for me to send another
>> new patch.
>>
>> Perhaps just add something like page_pool_put_page_nosync() as
>> page_pool_put_full_page() does for the case of dma_sync_size being
>> -1? and leave removing of extra checking as later refactoring and
>> optimization.
>>
>> As for the frag related API like page_pool_alloc_frag() and
>> page_pool_alloc(), we don't really have a corresponding free side
>> API for them, instead we reuse page_pool_put_page() for the free
>> side, and don't really do any dma sync unless it is the last frag
>> user of the same page, see the page_pool_is_last_ref() checking in
>> page_pool_put_netmem().
>>
>> So it might require more refactoring to support the usecase of
>> this patch for frag API, for example we might need to pull the
>> dma_sync operation out of __page_pool_put_page(), and put it in
>> page_pool_put_netmem() so that dma_sync is also done for the
>> non-last frag user too.
>> Or not support it for frag API for now as stmmac driver does not
>> seem to be using frag API, and put a warning to catch the case of
>> misusing of the 'feature' for frag API in the 'if' checking in
>> page_pool_put_netmem() before returning? something like below:
>>
>> --- a/include/net/page_pool/helpers.h
>> +++ b/include/net/page_pool/helpers.h
>> @@ -317,8 +317,10 @@ static inline void page_pool_put_netmem(struct page_pool *pool,
>>          * allow registering MEM_TYPE_PAGE_POOL, but shield linker.
>>          */
>>  #ifdef CONFIG_PAGE_POOL
>> -       if (!page_pool_is_last_ref(netmem))
>> +       if (!page_pool_is_last_ref(netmem)) {
>> +               /* Big comment why frag API is not support yet */
>> +               DEBUG_NET_WARN_ON_ONCE(!dma_sync_size);

Note, the above checking is not 100% reliable, as which frag user
is the last one depending on runtime excution.

> 
> Ok, since we do have a page_pool_put_full_page(), adding a variant for
> the nosync seems reasonable.
> But can't the check above be part of that function instead of the core code?

I was thinking about something like below mirroring page_pool_put_full_page()
for simplicity:
static inline void page_pool_put_page_nosync(struct page_pool *pool,
					     struct page *page, bool allow_direct)
{
	page_pool_put_netmem(pool, page_to_netmem(page), 0, allow_direct);
}

And do the dma_sync_size checking as this patch does in
page_pool_dma_sync_for_device().