Re: [PATCH v2 1/3] mm,slub: Do not special case N_NORMAL nodes for slab_nodes

[Harry Yoo]

On Tue, Apr 08, 2025 at 02:49:43PM +0200, Oscar Salvador wrote:
> On Tue, Apr 08, 2025 at 12:17:52PM +0200, David Hildenbrand wrote:
> > Feel free to add a Suggested-by: if you think it applies.
> 
> Sorry David, my bad, totally missed it.
> I shall add it.
> 
> > Do we have to take care of the N_NORMAL_MEMORY check in kmem_cache_init() ? Likely it
> > would have to be a N_MEMORY check.
> 
> Yes, should be N_MEMORY.
> 
> > But, I was wondering if we could get rid of the "slab_nodes" thingy as a first step?
> 
> I glanced over it and I did not see anything wrong with it, just a
> question below.
> 
> So, if Vlastimil and Harry think this is fine, we can indeed do this.

Looks fine to me.

> If so, I would combine this and the #1 first of this series and add
> your Signed-off-by as co-autor. Is that fine by you?

> > @@ -3706,10 +3698,9 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >  	if (!slab) {
> >  		/*
> >  		 * if the node is not online or has no normal memory, just
> > -		 * ignore the node constraint
> > +		 * ignore the node constraint.
> >  		 */
> > -		if (unlikely(node != NUMA_NO_NODE &&
> > -			     !node_isset(node, slab_nodes)))
> > +		if (unlikely(node != NUMA_NO_NODE && !node_state(node, N_NORMAL_MEMORY)))
> >  			node = NUMA_NO_NODE;
> 
> After my first patch, slab_nodes will also contain N_MEMORY nodes, which
> makes me think whether that check should be N_MEMORY?

Assuming that it's not frequent to call kmem_cache_alloc_node/kmalloc_node
with nid where the node has no normal memory, either way looks fine to
me.

It N_MEMORY check says it has some memory but it has no normal memory,
it will fail anyway because the kernel can't allocate pages from the
buddy.

But why not fall back to other nodes early when the kernel knows
it has no normal memory?

> -- 
> Oscar Salvador
> SUSE Labs

-- 
Cheers,
Harry / Hyeonggon