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

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

[Furong Xu]

Setting dma_sync_size to 0 is not illegal, and several drivers already did.
We can save a couple of function calls if check for dma_sync_size earlier.

Signed-off-by: Furong Xu <0x1207@gmail.com>
---
 net/core/page_pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a813d30d2135..fac52ba3f7c4 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -454,7 +454,7 @@ page_pool_dma_sync_for_device(const struct page_pool *pool,
 			      netmem_ref netmem,
 			      u32 dma_sync_size)
 {
-	if (pool->dma_sync && dma_dev_need_sync(pool->p.dev))
+	if (dma_sync_size && pool->dma_sync && dma_dev_need_sync(pool->p.dev))
 		__page_pool_dma_sync_for_device(pool, netmem, dma_sync_size);
 }
 
-- 
2.34.1

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

[Yunsheng Lin]

On 2024/10/10 19:40, Furong Xu wrote:
> Setting dma_sync_size to 0 is not illegal, and several drivers already did.
> We can save a couple of function calls if check for dma_sync_size earlier.
> 
> Signed-off-by: Furong Xu <0x1207@gmail.com>
> ---
>  net/core/page_pool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a813d30d2135..fac52ba3f7c4 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -454,7 +454,7 @@ page_pool_dma_sync_for_device(const struct page_pool *pool,
>  			      netmem_ref netmem,
>  			      u32 dma_sync_size)
>  {
> -	if (pool->dma_sync && dma_dev_need_sync(pool->p.dev))
> +	if (dma_sync_size && pool->dma_sync && dma_dev_need_sync(pool->p.dev))
>  		__page_pool_dma_sync_for_device(pool, netmem, dma_sync_size);

Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
when calling page_pool_create()?
Does it only need dma sync for some cases and not need dma sync for other
cases? if so, why not do the dma sync in the driver instead?

>  }
>  

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

[Furong Xu]

On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:

> Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
> when calling page_pool_create()?
> Does it only need dma sync for some cases and not need dma sync for other
> cases? if so, why not do the dma sync in the driver instead?

The answer is in this commit:
https://git.kernel.org/netdev/net/c/5546da79e6cc

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

[Ilias Apalodimas]

Hi Furong,

On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@gmail.com> wrote:
>
> On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> > Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
> > when calling page_pool_create()?
> > Does it only need dma sync for some cases and not need dma sync for other
> > cases? if so, why not do the dma sync in the driver instead?
>
> The answer is in this commit:
> https://git.kernel.org/netdev/net/c/5546da79e6cc

I am not sure I am following. Where does the stmmac driver call a sync
with len 0?

Thanks
/Ilias

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

[Furong Xu]

Hi Ilias,

On Fri, 11 Oct 2024 08:06:04 +0300, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:

> Hi Furong,
> 
> On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@gmail.com> wrote:
> >
> > On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >  
> > > Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
> > > when calling page_pool_create()?
> > > Does it only need dma sync for some cases and not need dma sync for other
> > > cases? if so, why not do the dma sync in the driver instead?  
> >
> > The answer is in this commit:
> > https://git.kernel.org/netdev/net/c/5546da79e6cc  
> 
> I am not sure I am following. Where does the stmmac driver call a sync
> with len 0?
For now, only drivers/net/ethernet/freescale/fec_main.c does.
And stmmac driver does not yet, but I will send another patch to make it call sync with
len 0. This is a proper fix as Jakub Kicinski suggested.

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

[Yunsheng Lin]

On 2024/10/11 14:31, Furong Xu wrote:
> Hi Ilias,
> 
> On Fri, 11 Oct 2024 08:06:04 +0300, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> 
>> Hi Furong,
>>
>> On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@gmail.com> wrote:
>>>
>>> On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>  
>>>> Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
>>>> when calling page_pool_create()?
>>>> Does it only need dma sync for some cases and not need dma sync for other
>>>> cases? if so, why not do the dma sync in the driver instead?  
>>>
>>> The answer is in this commit:
>>> https://git.kernel.org/netdev/net/c/5546da79e6cc  
>>
>> I am not sure I am following. Where does the stmmac driver call a sync
>> with len 0?
> For now, only drivers/net/ethernet/freescale/fec_main.c does.
> And stmmac driver does not yet, but I will send another patch to make it call sync with
> len 0. This is a proper fix as Jakub Kicinski suggested.

In order to support the above use case, it seems there might be two
options here:
1. Driver calls page_pool_create() without PP_FLAG_DMA_SYNC_DEV and
   handle the dma sync itself.
2. Page_pool may provides a non-dma-sync version of page_pool_put_page()
   API even when Driver calls page_pool_create() with PP_FLAG_DMA_SYNC_DEV.

Maybe option 2 is better one in the longer term as it may provide some
flexibility for the user and enable removing of the DMA_SYNC_DEV in the
future?

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

[Furong Xu]

Hi Jakub,

On Fri, 11 Oct 2024 16:55:34 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:

> On 2024/10/11 14:31, Furong Xu wrote:
> > Hi Ilias,
> > 
> > On Fri, 11 Oct 2024 08:06:04 +0300, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> >   
> >> Hi Furong,
> >>
> >> On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@gmail.com> wrote:  
> >>>
> >>> On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>>    
> >>>> Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
> >>>> when calling page_pool_create()?
> >>>> Does it only need dma sync for some cases and not need dma sync for other
> >>>> cases? if so, why not do the dma sync in the driver instead?    
> >>>
> >>> The answer is in this commit:
> >>> https://git.kernel.org/netdev/net/c/5546da79e6cc    
> >>
> >> I am not sure I am following. Where does the stmmac driver call a sync
> >> with len 0?  
> > For now, only drivers/net/ethernet/freescale/fec_main.c does.
> > And stmmac driver does not yet, but I will send another patch to make it call sync with
> > len 0. This is a proper fix as Jakub Kicinski suggested.  
> 
> In order to support the above use case, it seems there might be two
> options here:
> 1. Driver calls page_pool_create() without PP_FLAG_DMA_SYNC_DEV and
>    handle the dma sync itself.
> 2. Page_pool may provides a non-dma-sync version of page_pool_put_page()
>    API even when Driver calls page_pool_create() with PP_FLAG_DMA_SYNC_DEV.
> 
> Maybe option 2 is better one in the longer term as it may provide some
> flexibility for the user and enable removing of the DMA_SYNC_DEV in the
> future?

What is your opinion about this?
Thanks.

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

[Jakub Kicinski]

On Fri, 11 Oct 2024 17:26:05 +0800 Furong Xu wrote:
> > In order to support the above use case, it seems there might be two
> > options here:
> > 1. Driver calls page_pool_create() without PP_FLAG_DMA_SYNC_DEV and
> >    handle the dma sync itself.
> > 2. Page_pool may provides a non-dma-sync version of page_pool_put_page()
> >    API even when Driver calls page_pool_create() with PP_FLAG_DMA_SYNC_DEV.
> > 
> > Maybe option 2 is better one in the longer term as it may provide some
> > flexibility for the user and enable removing of the DMA_SYNC_DEV in the
> > future?  
> 
> What is your opinion about this?

I think your patch is fine, but it's a micro optimization so you need 
to provide some measurement data to show how much of a performance
improvement you're getting.

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

[Ilias Apalodimas]

On Fri, 11 Oct 2024 at 11:55, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2024/10/11 14:31, Furong Xu wrote:
> > Hi Ilias,
> >
> > On Fri, 11 Oct 2024 08:06:04 +0300, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> >
> >> Hi Furong,
> >>
> >> On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@gmail.com> wrote:
> >>>
> >>> On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
> >>>
> >>>> Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
> >>>> when calling page_pool_create()?
> >>>> Does it only need dma sync for some cases and not need dma sync for other
> >>>> cases? if so, why not do the dma sync in the driver instead?
> >>>
> >>> The answer is in this commit:
> >>> https://git.kernel.org/netdev/net/c/5546da79e6cc
> >>
> >> I am not sure I am following. Where does the stmmac driver call a sync
> >> with len 0?
> > For now, only drivers/net/ethernet/freescale/fec_main.c does.
> > And stmmac driver does not yet, but I will send another patch to make it call sync with
> > len 0. This is a proper fix as Jakub Kicinski suggested.
>
> In order to support the above use case, it seems there might be two
> options here:
> 1. Driver calls page_pool_create() without PP_FLAG_DMA_SYNC_DEV and
>    handle the dma sync itself.
> 2. Page_pool may provides a non-dma-sync version of page_pool_put_page()
>    API even when Driver calls page_pool_create() with PP_FLAG_DMA_SYNC_DEV.
>
> Maybe option 2 is better one in the longer term as it may provide some
> flexibility for the user and enable removing of the DMA_SYNC_DEV in the
> future?

Case 2 would be what this patch does. We already treat -1 as the
maximum DMA memory size. I think it's ok to treat 0 as 'don't sync'. I
need to figure out why people need this.
IOW you still have to sync the page to use it. Now you can do it when
allocating it, but why is that cheaper or preferable?

Thanks
/Ilias

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

[Yunsheng Lin]

On 2024/10/11 20:13, Ilias Apalodimas wrote:
> On Fri, 11 Oct 2024 at 11:55, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2024/10/11 14:31, Furong Xu wrote:
>>> Hi Ilias,
>>>
>>> On Fri, 11 Oct 2024 08:06:04 +0300, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
>>>
>>>> Hi Furong,
>>>>
>>>> On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@gmail.com> wrote:
>>>>>
>>>>> On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>>>
>>>>>> Is there any reason that those drivers not to unset the PP_FLAG_DMA_SYNC_DEV
>>>>>> when calling page_pool_create()?
>>>>>> Does it only need dma sync for some cases and not need dma sync for other
>>>>>> cases? if so, why not do the dma sync in the driver instead?
>>>>>
>>>>> The answer is in this commit:
>>>>> https://git.kernel.org/netdev/net/c/5546da79e6cc
>>>>
>>>> I am not sure I am following. Where does the stmmac driver call a sync
>>>> with len 0?
>>> For now, only drivers/net/ethernet/freescale/fec_main.c does.
>>> And stmmac driver does not yet, but I will send another patch to make it call sync with
>>> len 0. This is a proper fix as Jakub Kicinski suggested.
>>
>> In order to support the above use case, it seems there might be two
>> options here:
>> 1. Driver calls page_pool_create() without PP_FLAG_DMA_SYNC_DEV and
>>    handle the dma sync itself.
>> 2. Page_pool may provides a non-dma-sync version of page_pool_put_page()
>>    API even when Driver calls page_pool_create() with PP_FLAG_DMA_SYNC_DEV.
>>
>> Maybe option 2 is better one in the longer term as it may provide some
>> flexibility for the user and enable removing of the DMA_SYNC_DEV in the
>> future?
> 
> Case 2 would be what this patch does. We already treat -1 as the

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.

> maximum DMA memory size. I think it's ok to treat 0 as 'don't sync'. I
> need to figure out why people need this.
> IOW you still have to sync the page to use it. Now you can do it when
> allocating it, but why is that cheaper or preferable?
> 
> Thanks
> /Ilias
> 
> 

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

[Furong Xu]

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?
I am so glad to test it on my device ;)
Thanks.

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

[Yunsheng Lin]

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

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

[Ilias Apalodimas]

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

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?

Thanks
/Ilias
>                 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.
> >
> >

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

[Yunsheng Lin]

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

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

[Ilias Apalodimas]

Apologies for the noise. The last message was not clear text...


On Tue, 15 Oct 2024 at 14:06, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> 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 execution.

I am not sure I understand the problem here. If we are about to call
page_pool_return_page() we don't care what happens to that page.
If we end up calling __page_pool_put_page() it's the *callers* job now
to sync the page now once all fragments are released. So why is this
different from syncing an entire page?

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

Yes, that's ok. But the question was about moving the !dma_sync_size warning.
On second thought I think it's better if we leave it on the core code.
But as I said above I am not sure why we need it.

Thanks
/Ilias
> And do the dma_sync_size checking as this patch does in
> page_pool_dma_sync_for_device().

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

[Furong Xu]

Hi Ilias,

On Tue, 15 Oct 2024 16:25:51 +0300, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:

> I am not sure I understand the problem here. If we are about to call
> page_pool_return_page() we don't care what happens to that page.
> If we end up calling __page_pool_put_page() it's the *callers* job now
> to sync the page now once all fragments are released. So why is this
> different from syncing an entire page?
> 
> >  
> > >
> > > 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);
> > }
> >  
> 
> Yes, that's ok. But the question was about moving the !dma_sync_size warning.
> On second thought I think it's better if we leave it on the core code.
> But as I said above I am not sure why we need it.

You can read this:
https://git.kernel.org/netdev/net/c/b514c47ebf41
https://git.kernel.org/netdev/net/c/5546da79e6cc

drivers/net/ethernet/renesas/ravb_main.c and
drivers/net/ethernet/freescale/fec_main.c are calling page_pool_put_page() with
dma_sync_size setting to 0.

I will send another patchset to add page_pool_put_page_nosync() as
Yunsheng Lin suggested, then convert drivers/net/ethernet/renesas/ravb_main.c
and drivers/net/ethernet/freescale/fec_main.c to the new
page_pool_put_page_nosync().

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

[Yunsheng Lin]

On 2024/10/15 21:25, Ilias Apalodimas wrote:

...

>>>>
>>>> --- 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 execution.
> 
> I am not sure I understand the problem here. If we are about to call
> page_pool_return_page() we don't care what happens to that page.
> If we end up calling __page_pool_put_page() it's the *callers* job now
> to sync the page now once all fragments are released. So why is this
> different from syncing an entire page?

It also would be that we end up calling nothing for non-last-frag-user
too when page_pool_is_last_ref() return false as the above 'if' checking
in page_pool_put_netmem() if frag related API is used.

If the above happens, there is no dma_sync done here even if the caller
passes a non-zero 'dma_sync_size' which means it is not supposed to call
page_pool_put_unrefed_netmem() for the same page with both zero and
non-zero 'dma_sync_size'.

For example:
1. If page_pool_put_page() with dma_sync_size being non-zero is first
   called for a page, and there might be no dma_sync operation if it is
   not the last frag.
2. Then page_pool_put_page() with dma_sync_size being zero is called for
   the same page to skip the dma_sync operation.

Then we might have problem here as the dma_sync operation is only expected
to be skipped for a specific page fragment, but the above calling order causes
the dma_sync operation to be skipped for the whole page.

IOW, there is currently no way to tell which fragment is being freed when
page_pool_put_page() API is called for a page split into multi fragments now,
so we might need to call page_pool_put_page() with 'dma_sync_size' all being
'-1' if the page_pool is expected to do the dma sync operation for now.

> 
>>