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

[Yunsheng Lin <linyunsheng@huawei.com>]

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);
                return;
+       }

        page_pool_put_unrefed_netmem(pool, netmem, dma_sync_size, allow_direct);
 #endif


> I am so glad to test it on my device ;)
> Thanks.
> 
>