Messages in this thread | | | Subject | Re: [PATCH v2] page_pool: add a comment explaining the fragment counter usage | From | Alexander H Duyck <> | Date | Tue, 14 Feb 2023 07:27:41 -0800 |
| |
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>
| |