Date:   Wed, 4 Jan 2017 12:00:55 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     linux-mm@...ck.org, Alexander Duyck <alexander.duyck@...il.com>,
        willemdebruijn.kernel@...il.com, netdev@...r.kernel.org,
        john.fastabend@...il.com, Saeed Mahameed <saeedm@...lanox.com>,
        bjorn.topel@...el.com,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Tariq Toukan <tariqt@...lanox.com>, brouer@...hat.com,
        Mel Gorman <mgorman@...hsingularity.net>
Subject: Re: [RFC PATCH 2/4] page_pool: basic implementation of page_pool


On Tue, 3 Jan 2017 17:07:49 +0100 Vlastimil Babka <vbabka@...e.cz> wrote:

> On 12/20/2016 02:28 PM, Jesper Dangaard Brouer wrote:
> > The focus in this patch is getting the API around page_pool figured out.
> >
> > The internal data structures for returning page_pool pages is not optimal.
> > This implementation use ptr_ring for recycling, which is known not to scale
> > in case of multiple remote CPUs releasing/returning pages.  
> 
> Just few very quick impressions...
> 
> > A bulking interface into the page allocator is also left for later. (This
> > requires cooperation will Mel Gorman, who just send me some PoC patches for this).
> > ---
[...]
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 4424784ac374..11b4d8fb280b 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
[...]
> > @@ -765,6 +766,11 @@ static inline void put_page(struct page *page)
> >  {
> >  	page = compound_head(page);
> >
> > +	if (PagePool(page)) {
> > +		page_pool_put_page(page);
> > +		return;
> > +	}  
> 
> Can't say I'm thrilled about a new page flag and a test in put_page(). 

In patch 4/4, I'm scaling this back.  Avoiding to modify the inlined
put_page(), by letting refcnt reach zero and catching pages belonging to
a page_pool in __free_pages_ok() and free_hot_cold_page(). (Result
in being more dependent on page-refcnt and loosing some performance).

Still needing a new page flag, or some other method of identifying when
a page belongs to a page_pool.


> I don't know the full life cycle here, but isn't it that these pages
> will be specifically allocated and used in page pool aware drivers,
> so maybe they can be also specifically freed there without hooking to
> the generic page refcount mechanism?

Drivers are already manipulating refcnt, to "splitup" the page (to
save memory) for storing more RX frames per page.  Which is something
the page_pool still need to support. (XDP can request one page per
packet and gain the direct recycle optimization and instead waste mem).

Notice, a page_pool aware driver doesn't handle the "free-side".  Free
happens when the packet/page is being consumed, spliced or transmitted
out another non-page_pool-aware NIC driver.  An interresting case is
packet-page waiting for DMA TX completion (on another NIC), thus need
to async-store info on page_pool and DMA-addr.

Could extend the SKB (with a page_pool pointer)... BUT it defeats the
purpose of avoiding to allocate the SKB.  E.g. in the cases where XDP
takes the route-decision and transmit/forward the "raw"-page (out
another NIC or into a "raw" socket), then we don't have a meta-data
structure to store this info in. Thus, this info is stored in struct
page.


More arguing why a tight MM integration is prefered here[1]
 [1] http://prototype-kernel.readthedocs.io/en/latest/vm/page_pool/design/design.html#memory-model
besides in makes it easier to convert drivers to use a page_pool.
 
> > +
> >  	if (put_page_testzero(page))
> >  		__put_page(page);
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 08d947fc4c59..c74dea967f99 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -47,6 +47,12 @@ struct page {
> >  	unsigned long flags;		/* Atomic flags, some possibly
> >  					 * updated asynchronously */
> >  	union {
> > +		/* DISCUSS: Considered moving page_pool pointer here,
> > +		 * but I'm unsure if 'mapping' is needed for userspace
> > +		 * mapping the page, as this is a use-case the
> > +		 * page_pool need to support in the future. (Basically
> > +		 * mapping a NIC RX ring into userspace).  
> 
> I think so, but might be wrong here. In any case mapping usually goes with 
> index, and you put dma_addr in union with index below...

Good point, thanks.

> > +		 */
> >  		struct address_space *mapping;	/* If low bit clear, points to
> >  						 * inode address_space, or NULL.
> >  						 * If page mapped as anonymous
> > @@ -63,6 +69,7 @@ struct page {
> >  	union {
> >  		pgoff_t index;		/* Our offset within mapping. */
> >  		void *freelist;		/* sl[aou]b first free object */
> > +		dma_addr_t dma_addr;    /* used by page_pool */
> >  		/* page_deferred_list().prev	-- second tail page */
> >  	};
> >
> > @@ -117,6 +124,8 @@ struct page {
> >  	 * avoid collision and false-positive PageTail().
> >  	 */
> >  	union {
> > +		/* XXX: Idea reuse lru list, in page_pool to align with PCP */
> > +
> >  		struct list_head lru;	/* Pageout list, eg. active_list
> >  					 * protected by zone_lru_lock !
> >  					 * Can be used as a generic list

Guess, I can move it here, as the page cannot be on the LRU-list, while
being used (or VMA mapped). Right?

> > @@ -189,6 +198,8 @@ struct page {
> >  #endif
> >  #endif
> >  		struct kmem_cache *slab_cache;	/* SL[AU]B: Pointer to slab */
> > +		/* XXX: Sure page_pool will have no users of "private"? */
> > +		struct page_pool *pool;
> >  	};
> >
> >  #ifdef CONFIG_MEMCG  

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Powered by blists - more mailing lists