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@xxxxxxxxxx> wrote:
>
> On 2024/10/15 15:43, Ilias Apalodimas wrote:
> > Hi Yunsheng,
> >
> > On Mon, 14 Oct 2024 at 15:39, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
> >>
> >> On 2024/10/14 14:35, Furong Xu wrote:
> >>> Hi Yunsheng,
> >>>
> >>> On Sat, 12 Oct 2024 14:14:41 +0800, Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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().