| Message ID | 20230217222130.85205-1-ilias.apalodimas@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4d4266e3fd321fadb628ce02de641b129522c39c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] page_pool: add a comment explaining the fragment counter usage | expand |
On 17/02/2023 23.21, Ilias Apalodimas wrote: > When reading the page_pool code the first impression is that keeping > two separate counters, one being the page refcnt and the other being > fragment pp_frag_count, is counter-intuitive. > > However without that fragment counter we don't know when to reliably > destroy or sync the outstanding DMA mappings. So let's add a comment > explaining this part. > > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > Changes since v2: > - Removed a uneeded commas on the comment > Changes since v1: > - Update the comment withe the correct description for pp_frag_count > include/net/page_pool.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/net/page_pool.h b/include/net/page_pool.h > index 34bf531ffc8d..ddfa0b328677 100644 > --- a/include/net/page_pool.h > +++ b/include/net/page_pool.h > @@ -277,6 +277,16 @@ void page_pool_put_defragged_page(struct page_pool *pool, struct page *page, > unsigned int dma_sync_size, > bool allow_direct); > > +/* pp_frag_count represents the number of writers who can update the page > + * either by updating skb->data or via DMA mappings for the device. > + * We can't rely on the page refcnt for that as we don't know who might be > + * holding page references and we can't reliably destroy or sync DMA mappings > + * of the fragments. > + * > + * When pp_frag_count reaches 0 we can either recycle the page if the page > + * refcnt is 1 or return it back to the memory allocator and destroy any > + * mappings we have. > + */ > static inline void page_pool_fragment_page(struct page *page, long nr) > { > atomic_long_set(&page->pp_frag_count, nr); > -- > 2.38.1 >
On Sat, 2023-02-18 at 00:21 +0200, Ilias Apalodimas wrote: > When reading the page_pool code the first impression is that keeping > two separate counters, one being the page refcnt and the other being > fragment pp_frag_count, is counter-intuitive. > > However without that fragment counter we don't know when to reliably > destroy or sync the outstanding DMA mappings. So let's add a comment > explaining this part. > > Reviewed-by: Alexander Duyck <alexanderduyck@fb.com> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> # Form letter - net-next is closed The merge window for v6.3 has begun and therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after Mar 6th. RFC patches sent for review only are obviously welcome at any time.
On Sat, 18 Feb 2023 00:21:30 +0200 Ilias Apalodimas wrote: > When reading the page_pool code the first impression is that keeping > two separate counters, one being the page refcnt and the other being > fragment pp_frag_count, is counter-intuitive. > > However without that fragment counter we don't know when to reliably > destroy or sync the outstanding DMA mappings. So let's add a comment > explaining this part. I discussed with Paolo off-list, since it's just a comment change I'll push it in.
On Tue, 21 Feb 2023 at 19:15, Jakub Kicinski <kuba@kernel.org> wrote: > > On Sat, 18 Feb 2023 00:21:30 +0200 Ilias Apalodimas wrote: > > When reading the page_pool code the first impression is that keeping > > two separate counters, one being the page refcnt and the other being > > fragment pp_frag_count, is counter-intuitive. > > > > However without that fragment counter we don't know when to reliably > > destroy or sync the outstanding DMA mappings. So let's add a comment > > explaining this part. > > I discussed with Paolo off-list, since it's just a comment change > I'll push it in. Fair enough. Thanks Jakub. Regards /Ilias
Hello: This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <kuba@kernel.org>: On Sat, 18 Feb 2023 00:21:30 +0200 you wrote: > When reading the page_pool code the first impression is that keeping > two separate counters, one being the page refcnt and the other being > fragment pp_frag_count, is counter-intuitive. > > However without that fragment counter we don't know when to reliably > destroy or sync the outstanding DMA mappings. So let's add a comment > explaining this part. > > [...] Here is the summary with links: - [v3] page_pool: add a comment explaining the fragment counter usage https://git.kernel.org/netdev/net-next/c/4d4266e3fd32 You are awesome, thank you!
diff --git a/include/net/page_pool.h b/include/net/page_pool.h index 34bf531ffc8d..ddfa0b328677 100644 --- a/include/net/page_pool.h +++ b/include/net/page_pool.h @@ -277,6 +277,16 @@ void page_pool_put_defragged_page(struct page_pool *pool, struct page *page, unsigned int dma_sync_size, bool allow_direct); +/* pp_frag_count represents the number of writers who can update the page + * either by updating skb->data or via DMA mappings for the device. + * We can't rely on the page refcnt for that as we don't know who might be + * holding page references and we can't reliably destroy or sync DMA mappings + * of the fragments. + * + * When pp_frag_count reaches 0 we can either recycle the page if the page + * refcnt is 1 or return it back to the memory allocator and destroy any + * mappings we have. + */ static inline void page_pool_fragment_page(struct page *page, long nr) { atomic_long_set(&page->pp_frag_count, nr);