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