Re: [PATCH][v2] page_pool: handle page recycle for NUMA_NO_NODE condition

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Sat, 7 Dec 2019 03:52:41 +0000
Saeed Mahameed <saeedm@mellanox.com> wrote:

> On Fri, 2019-12-06 at 17:32 +0800, Li RongQing wrote:
> > some drivers uses page pool, but not require to allocate
> > pages from bound node, or simply assign pool.p.nid to
> > NUMA_NO_NODE, and the commit d5394610b1ba ("page_pool:
> > Don't recycle non-reusable pages") will block this kind
> > of driver to recycle
> > 
> > so take page as reusable when page belongs to current
> > memory node if nid is NUMA_NO_NODE
> > 
> > v1-->v2: add check with numa_mem_id from Yunsheng
> > 
> > Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > Suggested-by: Yunsheng Lin <linyunsheng@huawei.com>
> > Cc: Saeed Mahameed <saeedm@mellanox.com>
> > ---
> >  net/core/page_pool.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> > index a6aefe989043..3c8b51ccd1c1 100644
> > --- a/net/core/page_pool.c
> > +++ b/net/core/page_pool.c
> > @@ -312,12 +312,17 @@ static bool __page_pool_recycle_direct(struct
> > page *page,
> >  /* page is NOT reusable when:
> >   * 1) allocated when system is under some pressure.
> > (page_is_pfmemalloc)
> >   * 2) belongs to a different NUMA node than pool->p.nid.
> > + * 3) belongs to a different memory node than current context
> > + * if pool->p.nid is NUMA_NO_NODE
> >   *
> >   * To update pool->p.nid users must call page_pool_update_nid.
> >   */
> >  static bool pool_page_reusable(struct page_pool *pool, struct page
> > *page)
> >  {
> > -	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool-  
> > >p.nid;  
> > +	return !page_is_pfmemalloc(page) &&
> > +		(page_to_nid(page) == pool->p.nid ||
> > +		(pool->p.nid == NUMA_NO_NODE &&
> > +		page_to_nid(page) == numa_mem_id()));
> >  }
> >    
> 
> Cc'ed Jesper, Ilias & Jonathan.
> 
> I don't think it is correct to check that the page nid is same as
> numa_mem_id() if pool is NUMA_NO_NODE. In such case we should allow all
> pages to recycle, because you can't assume where pages are allocated
> from and where they are being handled.
> 
> I suggest the following:
> 
> return !page_pfmemalloc() && 
> ( page_to_nid(page) == pool->p.nid || pool->p.nid == NUMA_NO_NODE );
> 
> 1) never recycle emergency pages, regardless of pool nid.
> 2) always recycle if pool is NUMA_NO_NODE.
> 
> the above change should not add any overhead, a modest branch predictor
> will handle this with no effort.
> 
> Jesper et al. what do you think?

The patch description doesn't explain the problem very well.

Lets first establish what the problem is.  After I took at closer look,
I do think we have a real problem here...

If function alloc_pages_node() is called with NUMA_NO_NODE (see below
signature), then the nid is re-assigned to numa_mem_id().

Our current code checks: page_to_nid(page) == pool->p.nid which seems
bogus, as pool->p.nid=NUMA_NO_NODE and the page NID will not return
NUMA_NO_NODE... as it was set to the local detect numa node, right?

So, we do need a fix... but the question is that semantics do we want?

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


/*
 * Allocate pages, preferring the node given as nid. When nid == NUMA_NO_NODE,
 * prefer the current CPU's closest node. Otherwise node must be valid and
 * online.
 */
static inline struct page *alloc_pages_node(int nid, gfp_t gfp_mask,
						unsigned int order)
{
	if (nid == NUMA_NO_NODE)
		nid = numa_mem_id();

	return __alloc_pages_node(nid, gfp_mask, order);
}

static bool pool_page_reusable(struct page_pool *pool, struct page *page)
{
	return !page_is_pfmemalloc(page) && page_to_nid(page) == pool->p.nid;
}