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

From: Saeed Mahameed <saeedm@mellanox.com>
To: "ilias.apalodimas@linaro.org" <ilias.apalodimas@linaro.org>,
	"brouer@redhat.com" <brouer@redhat.com>
Cc: "peterz@infradead.org" <peterz@infradead.org>,
	"linyunsheng@huawei.com" <linyunsheng@huawei.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Li Rongqing <lirongqing@baidu.com>,
	"mhocko@kernel.org" <mhocko@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [net-next v5 PATCH] page_pool: handle page recycle for NUMA_NO_NODE condition
Date: Fri, 20 Dec 2019 21:27:03 +0000	[thread overview]
Message-ID: <ac43d22859fa5a5145cc52e4673de01209818b9d.camel@mellanox.com> (raw)
In-Reply-To: <20191220102314.GB14269@apalos.home>

On Fri, 2019-12-20 at 12:23 +0200, Ilias Apalodimas wrote:
> Hi Jesper, 
> 
> I like the overall approach since this moves the check out of  the
> hotpath. 
> @Saeed, since i got no hardware to test this on, would it be possible
> to check
> that it still works fine for mlx5?

The idea seems reasonable,
I will need a day or two to test and review this.

The only thing we need to be careful about is how heavy the flush
operation on numa changes, holding a spin lock and releasing all pages
at once ..
prior to this patch, page releasing was done per packet, so there
should be an improvement here of bulk page flush, but on the other hand
we will be holding a spin lock.. i am not worried about spin lock
contention though, just about the potential cpu spikes.

Thanks,
Saeed.

> 
> [...]
> > +	struct ptr_ring *r = &pool->ring;
> > +	struct page *page;
> > +	int pref_nid; /* preferred NUMA node */
> > +
> > +	/* 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.
> > +	 */
> > +	pref_nid = (pool->p.nid == NUMA_NO_NODE) ? numa_mem_id() :
> > pool->p.nid;
> 
> One of the use cases for this is that during the allocation we are
> not
> guaranteed to pick up the correct NUMA node. 
> This will get automatically fixed once the driver starts recycling
> packets. 
> 
> I don't feel strongly about this, since i don't usually like hiding
> value
> changes from the user but, would it make sense to move this into 
> __page_pool_alloc_pages_slow() and change the pool->p.nid?
> 
> Since alloc_pages_node() will replace NUMA_NO_NODE with numa_mem_id()
> regardless, why not store the actual node in our page pool
> information?
> You can then skip this and check pool->p.nid == numa_mem_id(),
> regardless of
> what's configured. 
> 
> Thanks
> /Ilias