On Fri, Jan 06, 2023 at 02:59:30PM +0100, Jesper Dangaard Brouer wrote: > On 05/01/2023 22.46, Matthew Wilcox (Oracle) wrote: > > Convert __page_pool_alloc_page_order() and __page_pool_alloc_pages_slow() > > to use netmem internally. This removes a couple of calls > > to compound_head() that are hidden inside put_page(). > > Convert trace_page_pool_state_hold(), page_pool_dma_map() and > > page_pool_set_pp_info() to take a netmem argument. > > > > Saves 83 bytes of text in __page_pool_alloc_page_order() and 98 in > > __page_pool_alloc_pages_slow() for a total of 181 bytes. > > > > Signed-off-by: Matthew Wilcox (Oracle) <willy@xxxxxxxxxxxxx> > > --- > > include/trace/events/page_pool.h | 14 +++++------ > > net/core/page_pool.c | 42 +++++++++++++++++--------------- > > 2 files changed, 29 insertions(+), 27 deletions(-) > > Acked-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx> > > Question below. > > > diff --git a/net/core/page_pool.c b/net/core/page_pool.c > > index 437241aba5a7..4e985502c569 100644 > > --- a/net/core/page_pool.c > > +++ b/net/core/page_pool.c > [...] > > @@ -421,7 +422,8 @@ static struct page *__page_pool_alloc_pages_slow(struct page_pool *pool, > > page = NULL; > > } > > - /* When page just alloc'ed is should/must have refcnt 1. */ > > + /* When page just allocated it should have refcnt 1 (but may have > > + * speculative references) */ > > return page; > > What does it mean page may have speculative references ? > > And do I/we need to worry about that for page_pool? An excellent question. There are two code paths (known to me) which take speculative references on a page, and there may well be more. One is in GUP and the other is in the page cache. Both take the form of: rcu_read_lock(); again: look-up-page try-get-page-ref check-lookup if lookup-failed drop-page-ref; goto again; rcu_read_unlock(); If a page _has been_ in the page tables, then GUP can find it. If a page _has been_ in the page cache, then filemap can find it. Because there's no RCU grace period between freeing and reallocating a page, it actually means that any page can see its refcount temporarily raised. Usually the biggest problem is consumers assuming that they will be the last code to call put_page() / folio_put(), and can do their cleanup at that time (when the last caller of folio_put() may be GUP or filemap which knows nothing of what you're using the page for). I didn't notice any problems with temporarily elevated refcounts while doing the netmem conversion, and it's something I'm fairly sensitive to, so I think you got it all right and there is no need to be concerned. Hope that's helpful!
