Date:   Sat, 29 Jan 2022 20:49:01 +0200
From:   Ilias Apalodimas <ilias.apalodimas@...aro.org>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Yunsheng Lin <linyunsheng@...wei.com>,
        Netdev <netdev@...r.kernel.org>, hawk@...nel.org,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexander Duyck <alexanderduyck@...com>
Subject: Re: [net-next PATCH v2] page_pool: Refactor page_pool to enable
 fragmenting after allocation

Hi Alexander,

On Sat, 29 Jan 2022 at 20:35, Alexander Duyck <alexander.duyck@...il.com> wrote:
>

[...]

> > >
> > > Also I am not sure exporting the frag count to the driver is a good
> > > idea, as the above example seems a little complex, maybe adding
> > > the fragmenting after allocation support for a existing driver
> > > is a good way to show if the API is really a good one.
> >
> > This is already kind of exposed since no one limits drivers from calling
> > page_pool_atomic_sub_frag_count_return() right?
> > What this patchset does is allow the drivers to actually use it and release
> > pages without having to atomically decrement all the refcnt bias.
> >
> > And I do get the point that a driver might choose to do the refcounting
> > internally.  That was the point all along with the fragment support in
> > page_pool.  There's a wide variety of interfaces out there and each one
> > handles buffers differently.
> >
> > What I am missing though is how this works with the current recycling
> > scheme? The driver will still have to to make sure that
> > page_pool_defrag_page(page, 1) == 0 for that to work no?
>
> The general idea here is that we are getting away from doing in-driver
> recycling and instead letting page pool take care of all that. That
> was the original idea behind page pool, however the original
> implementation was limited to a single use per page only.
>
> So most of the legacy code out there is having to use the
> page_ref_count == 1 or page_ref_count == bias trick in order to
> determine if it can recycle the page. The page pool already takes care
> of the page recycling by returning the pages to the pool when
> page_ref_count == 1, what we get by adding the frag count is the
> ability for the drivers to drop the need to perform their own ref
> count tricks and instead offloads that to the kernel so when
> page_pool_defrag_page(page, 1) == 0 it can then go immediately into
> the checks for page_ref_count == 1 and just recycle the page into the
> page pool.

Excellent, that's what I assumed tbh

Thanks!

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@...aro.org>
>
> > >
> > >
> > > >
> > > > The general idea is that we want to have the ability to allocate a page
> > > > with excess fragment count and then trim off the unneeded fragments.
> > > >
> > > > Signed-off-by: Alexander Duyck <alexanderduyck@...com>
> > > > ---
> > > >
> > > > v2: Added page_pool_is_last_frag
> > > >     Moved comment about CONFIG_PAGE_POOL to page_pool_put_page
> > > >     Wrapped statements for page_pool_is_last_frag in parenthesis
> > > >
> > > >  include/net/page_pool.h |   82 ++++++++++++++++++++++++++++++-----------------
> > > >  net/core/page_pool.c    |   23 ++++++-------
> > > >  2 files changed, 62 insertions(+), 43 deletions(-)
> > > >
> > > > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > > > index 79a805542d0f..fbed91469d42 100644
> > > > --- a/include/net/page_pool.h
> > > > +++ b/include/net/page_pool.h
> > > > @@ -201,21 +201,67 @@ static inline void page_pool_put_page_bulk(struct page_pool *pool, void **data,
> > > >  }
> > > >  #endif
> > > >
> > > > -void page_pool_put_page(struct page_pool *pool, struct page *page,
> > > > -                   unsigned int dma_sync_size, bool allow_direct);
> > > > +void page_pool_put_defragged_page(struct page_pool *pool, struct page *page,
> > > > +                             unsigned int dma_sync_size,
> > > > +                             bool allow_direct);
> > > >
> > > > -/* Same as above but will try to sync the entire area pool->max_len */
> > > > -static inline void page_pool_put_full_page(struct page_pool *pool,
> > > > -                                      struct page *page, bool allow_direct)
> > > > +static inline void page_pool_fragment_page(struct page *page, long nr)
> > > > +{
> > > > +   atomic_long_set(&page->pp_frag_count, nr);
> > > > +}
> > > > +
> > > > +static inline long page_pool_defrag_page(struct page *page, long nr)
> > > > +{
> > > > +   long ret;
> > > > +
> > > > +   /* If nr == pp_frag_count then we are have cleared all remaining
> >
> > s/are//
>
> Will fix for v3.
>
> Thanks,
>
> Alex

Powered by blists - more mailing lists