lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
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