Re: [net-next v4 PATCH] page_pool: handle page recycle for NUMA_NO_NODE condition

[Yunsheng Lin]

On 2019/12/18 16:01, Jesper Dangaard Brouer wrote:
> The check in pool_page_reusable (page_to_nid(page) == pool->p.nid) is
> not valid if page_pool was configured with pool->p.nid = NUMA_NO_NODE.
> 
> The goal of the NUMA changes in commit d5394610b1ba ("page_pool: Don't
> recycle non-reusable pages"), were to have RX-pages that belongs to the
> same NUMA node as the CPU processing RX-packet during softirq/NAPI. As
> illustrated by the performance measurements.
> 
> This patch moves the NAPI checks out of fast-path, and at the same time
> solves the NUMA_NO_NODE issue.
> 
> First realize that alloc_pages_node() with pool->p.nid = NUMA_NO_NODE
> will lookup current CPU nid (Numa ID) via numa_mem_id(), which is used
> as the the preferred nid.  It is only in rare situations, where
> e.g. NUMA zone runs dry, that page gets doesn't get allocated from
> preferred nid.  The page_pool API allows drivers to control the nid
> themselves via controlling pool->p.nid.
> 
> This patch moves the NAPI check to when alloc cache is refilled, via
> dequeuing/consuming pages from the ptr_ring. Thus, we can allow placing
> pages from remote NUMA into the ptr_ring, as the dequeue/consume step
> will check the NUMA node. All current drivers using page_pool will
> alloc/refill RX-ring from same CPU running softirq/NAPI process.
> 
> Drivers that control the nid explicitly, also use page_pool_update_nid
> when changing nid runtime.  To speed up transision to new nid the alloc
> cache is now flushed on nid changes.  This force pages to come from
> ptr_ring, which does the appropate nid check.
> 
> For the NUMA_NO_NODE case, when a NIC IRQ is moved to another NUMA
> node, then ptr_ring will be emptied in 65 (PP_ALLOC_CACHE_REFILL+1)
> chunks per allocation and allocation fall-through to the real
> page-allocator with the new nid derived from numa_mem_id(). We accept
> that transitioning the alloc cache doesn't happen immediately.
> 
> Fixes: d5394610b1ba ("page_pool: Don't recycle non-reusable pages")
> Reported-by: Li RongQing <lirongqing@xxxxxxxxx>
> Reported-by: Yunsheng Lin <linyunsheng@xxxxxxxxxx>
> Signed-off-by: Jesper Dangaard Brouer <brouer@xxxxxxxxxx>
> ---
>  net/core/page_pool.c |   82 ++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 63 insertions(+), 19 deletions(-)
> 
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index a6aefe989043..bd4f8b2c46b6 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -96,10 +96,61 @@ struct page_pool *page_pool_create(const struct page_pool_params *params)
>  }
>  EXPORT_SYMBOL(page_pool_create);
>  
> +static void __page_pool_return_page(struct page_pool *pool, struct page *page);

It is possible to avoid forword-declare it by move the __page_pool_return_page()?
Maybe it is ok since this patch is targetting net-next?

> +
> +noinline
> +static struct page *page_pool_refill_alloc_cache(struct page_pool *pool,
> +						 bool refill)
> +{
> +	struct ptr_ring *r = &pool->ring;
> +	struct page *first_page, *page;
> +	int i, curr_nid;
> +
> +	/* Quicker fallback, avoid locks when ring is empty */
> +	if (__ptr_ring_empty(r))
> +		return NULL;
> +
> +	/* Softirq guarantee CPU and thus NUMA node is stable. This,
> +	 * assumes CPU refilling driver RX-ring will also run RX-NAPI.
> +	 */
> +	curr_nid = numa_mem_id();
> +
> +	/* Slower-path: Get pages from locked ring queue */
> +	spin_lock(&r->consumer_lock);
> +	first_page = __ptr_ring_consume(r);
> +
> +	/* Fallback to page-allocator if NUMA node doesn't match */
> +	if (first_page && unlikely(!(page_to_nid(first_page) == curr_nid))) {
> +		__page_pool_return_page(pool, first_page);
> +		first_page = NULL;
> +	}
> +
> +	if (unlikely(!refill))
> +		goto out;
> +
> +	/* Refill alloc array, but only if NUMA node match */
> +	for (i = 0; i < PP_ALLOC_CACHE_REFILL; i++) {
> +		page = __ptr_ring_consume(r);
> +		if (unlikely(!page))
> +			break;
> +
> +		if (likely(page_to_nid(page) == curr_nid)) {
> +			pool->alloc.cache[pool->alloc.count++] = page;
> +		} else {
> +			/* Release page to page-allocator, assume
> +			 * refcnt == 1 invariant of cached pages
> +			 */
> +			__page_pool_return_page(pool, page);
> +		}
> +	}

The above code seems to not clear all the pages in the ptr_ring that
is not in the local node in some case?

I am not so familiar with asm, but does below code make sense and
generate better asm code?

	struct page *page = NULL;

	while (pool->alloc.count < PP_ALLOC_CACHE_REFILL || !refill) {
		page = __ptr_ring_consume(r);

		if (unlikely(!page || !refill))
			break;

		if (likely(page_to_nid(page) == curr_nid)) {
			pool->alloc.cache[pool->alloc.count++] = page;
		} else {
			/* Release page to page-allocator, assume
			 * refcnt == 1 invariant of cached pages
			 */
			__page_pool_return_page(pool, page);
		}
	}

out:
	if (likely(refill && pool->alloc.count > 0))
		page = pool->alloc.cache[--pool->alloc.count];

	spin_unlock(&r->consumer_lock);
	
	return page;


"The above code does not compile or test yet".

the above will clear all the pages in the ptr_ring that is not in the local
node and treat the refill and !refill case consistently.

But for the refill case, the pool->alloc.count may be PP_ALLOC_CACHE_REFILL - 1
after page_pool_refill_alloc_cache() returns.


> +out:
> +	spin_unlock(&r->consumer_lock);
> +	return first_page;
> +}
> +
>  /* fast path */
>  static struct page *__page_pool_get_cached(struct page_pool *pool)
>  {
> -	struct ptr_ring *r = &pool->ring;
>  	bool refill = false;
>  	struct page *page;
>  
> @@ -113,20 +164,7 @@ static struct page *__page_pool_get_cached(struct page_pool *pool)
>  		refill = true;
>  	}
>  
> -	/* Quicker fallback, avoid locks when ring is empty */
> -	if (__ptr_ring_empty(r))
> -		return NULL;
> -
> -	/* Slow-path: Get page from locked ring queue,
> -	 * refill alloc array if requested.
> -	 */
> -	spin_lock(&r->consumer_lock);
> -	page = __ptr_ring_consume(r);
> -	if (refill)
> -		pool->alloc.count = __ptr_ring_consume_batched(r,
> -							pool->alloc.cache,
> -							PP_ALLOC_CACHE_REFILL);
> -	spin_unlock(&r->consumer_lock);
> +	page = page_pool_refill_alloc_cache(pool, refill);
>  	return page;
>  }
>  
> @@ -311,13 +349,10 @@ 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.
> - *
> - * 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);
>  }
>  
>  void __page_pool_put_page(struct page_pool *pool, struct page *page,
> @@ -484,7 +519,16 @@ EXPORT_SYMBOL(page_pool_destroy);
>  /* Caller must provide appropriate safe context, e.g. NAPI. */
>  void page_pool_update_nid(struct page_pool *pool, int new_nid)
>  {
> +	struct page *page;
> +
> +	WARN_ON(!in_serving_softirq());
>  	trace_page_pool_update_nid(pool, new_nid);
>  	pool->p.nid = new_nid;
> +
> +	/* Flush pool alloc cache, as refill will check NUMA node */
> +	while (pool->alloc.count) {
> +		page = pool->alloc.cache[--pool->alloc.count];
> +		__page_pool_return_page(pool, page);
> +	}
>  }
>  EXPORT_SYMBOL(page_pool_update_nid);
> 
> 
> 
> .
>