netdev - Re: [PATCH v2 05/24] page_pool: Start using netmem in allocation path.

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y7g/4vLdpzGOxAKv@casper.infradead.org>
Date:   Fri, 6 Jan 2023 15:36:02 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc:     Jesper Dangaard Brouer <hawk@...nel.org>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        brouer@...hat.com, netdev@...r.kernel.org, linux-mm@...ck.org,
        Shakeel Butt <shakeelb@...gle.com>
Subject: Re: [PATCH v2 05/24] page_pool: Start using netmem in allocation
 path.

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@...radead.org>
> > ---
> >   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@...hat.com>
> 
> 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!