| Message ID | 20230214104316.2254814-1-ilias.apalodimas@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] page_pool: add a comment explaining the fragment counter usage | expand |
On Tue, 2023-02-14 at 12:43 +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. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > 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..277e215cfb58 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, 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. > + */ I would get rid of the comma between "page" and "if" in the second paragraph. It breaks things up and makes it a bit harder to read. What we want to emphasize is that there are two possible paths. The extra comma makes it almost appear as if there are 3 options. Otherwise it looks good to me. Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
diff --git a/include/net/page_pool.h b/include/net/page_pool.h index 34bf531ffc8d..277e215cfb58 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, 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);
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. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- 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(+) -- 2.38.1