On Wed 26-02-20 22:45:52, Vlastimil Babka wrote:

On 2/26/20 7:41 PM, Michal Hocko wrote:

quoted

On Wed 26-02-20 18:25:28, Cristopher Lameter wrote:

quoted

On Mon, 24 Feb 2020, Michal Hocko wrote:

quoted

Hmm, nasty. Is there any reason why kmalloc_node behaves differently
from the page allocator?

The page allocator will do the same thing if you pass GFP_THISNODE and
insist on allocating memory from a node that does not exist.

I do not think that the page allocator would blow up even with
GFP_THISNODE. The allocation would just fail on memory less node.

Besides that kmalloc_node shouldn't really have an implicit GFP_THISNODE
semantic right? At least I do not see anything like that documented
anywhere.

Seems like SLAB at least behaves like the page allocator. See
____cache_alloc_node() where it basically does:

page = cache_grow_begin(cachep, gfp_exact_node(flags), nodeid);
...
if (!page)
	fallback_alloc(cachep, flags)

gfp_exact_node() adds __GFP_THISNODE among other things, so the initial
attempt does try to stick only to the given node. But fallback_alloc()
doesn't. In fact, even if kmalloc_node() was called with __GFP_THISNODE
then it wouldn't work as intended, as fallback_alloc() doesn't get the
nodeid, but instead will use numa_mem_id(). That part could probably be
improved.

SLUB's ___slab_alloc() has for example this:
if (node != NUMA_NO_NODE && !node_present_pages(node))

Hmm, just a quick note. Shouldn't this be node_managed_pages? In most
cases the difference is negligible but I can imagine crazy setups where
all present pages are simply consumed.

    searchnode = node_to_mem_node(node);

That's from Joonsoo's 2014 commit a561ce00b09e ("slub: fall back to
node_to_mem_node() node if allocating on memoryless node"), suggesting
that the scenario in this bug report should work. Perhaps it just got
broken unintentionally later.

A very good reference. Thanks!

And AFAICS the whole path leading to alloc_slab_page() also doesn't add
__GFP_THISNODE, but will keep it if caller passed it, and ultimately it
does:


if (node == NUMA_NO_NODE)
    page = alloc_pages(flags, order);
else
    page = __alloc_pages_node(node, flags, order);

So yeah looks like SLUB's kmalloc_node() is supposed to behave like the
page allocator's __alloc_pages_node() and respect __GFP_THISNODE but not
enforce it by itself. There's probably just some missing data structure
initialization somewhere right now for memoryless nodes.

Thanks for the confirmation!
-- 
Michal Hocko
SUSE Labs