| Message ID | 20241010114019.1734573-1-0x1207@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] page_pool: check for dma_sync_size earlier | expand |
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? > } >
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
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
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.
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?
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.
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
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.
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); }
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(-)