Message-ID: <CAC_iWjKWjRbhfHz4CJbq-SXEd=rDJP+Go0bfLQ4pMxFNNuPXNQ@mail.gmail.com>
Date: Tue, 15 Oct 2024 10:43:49 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: Furong Xu <0x1207@...il.com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jesper Dangaard Brouer <hawk@...nel.org>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	xfr@...look.com
Subject: Re: [PATCH net-next v1] page_pool: check for dma_sync_size earlier

Hi Yunsheng,

On Mon, 14 Oct 2024 at 15:39, Yunsheng Lin <linyunsheng@...wei.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@...wei.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.
> >
> >

Powered by blists - more mailing lists