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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
>>>
>>>> Hi Furong,
>>>>
>>>> On Fri, 11 Oct 2024 at 05:15, Furong Xu <0x1207@xxxxxxxxx> wrote:
>>>>>
>>>>> On Thu, 10 Oct 2024 19:53:39 +0800, Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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
> 
>