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:

> 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 agree, using numa_mem_id() is not valid, because it takes the numa
node id from the executing CPU and the call to __page_pool_put_page()
can happen on a remote CPU (e.g. cpumap redirect, and in future SKBs).


> I suggest the following:
> 
> return !page_pfmemalloc() && 
> ( page_to_nid(page) == pool->p.nid || pool->p.nid == NUMA_NO_NODE );

Above code doesn't generate optimal ASM code, I suggest:

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

I have compiled different variants and looked at the ASM code generated
by GCC.  This seems to give the best result.


> 1) never recycle emergency pages, regardless of pool nid.
> 2) always recycle if pool is NUMA_NO_NODE.

Yes, this defines the semantics, that a page_pool configured with
NUMA_NO_NODE means skip NUMA checks.  I think that sounds okay...


> the above change should not add any overhead, a modest branch
> predictor will handle this with no effort.

It still annoys me that we keep adding instructions to this code
hot-path (I counted 34 bytes and 11 instructions in my proposed
function).

I think that it might be possible to move these NUMA checks to
alloc-side (instead of return/recycles side as today), and perhaps only
on slow-path when dequeuing from ptr_ring (as recycles that call
__page_pool_recycle_direct() will be pinned during NAPI).  But lets
focus on a smaller fix for the immediate issue...

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