lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 11 Dec 2019 19:49:33 +0100
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Saeed Mahameed <saeedm@...lanox.com>
Cc:     "jonathan.lemon@...il.com" <jonathan.lemon@...il.com>,
        "linyunsheng@...wei.com" <linyunsheng@...wei.com>,
        Li Rongqing <lirongqing@...du.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ilias.apalodimas@...aro.org" <ilias.apalodimas@...aro.org>,
        brouer@...hat.com
Subject: Re: [PATCH][v2] page_pool: handle page recycle for NUMA_NO_NODE
 condition

On Sat, 7 Dec 2019 03:52:41 +0000
Saeed Mahameed <saeedm@...lanox.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

Powered by blists - more mailing lists