Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 24 Mar 2022 11:06:03 +0000
From:   Hyeonggon Yoo <42.hyeyoo@gmail.com>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>,
        Marco Elver <elver@google.com>,
        Matthew WilCox <willy@infradead.org>,
        Roman Gushchin <roman.gushchin@linux.dev>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 01/15] mm/slab: cleanup slab_alloc() and
 slab_alloc_node()
Message-ID: <20220324110603.GA2112827@odroid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5833607a-4444-206d-db4f-9f958653c5b0@suse.cz>
Precedence: bulk

Vlastimil wrote:
> On 3/8/22 12:41, Hyeonggon Yoo wrote:
> > +
> >  static __always_inline void *
> > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> > +		   unsigned long caller)
> >  {
> >  	unsigned long save_flags;
> > -	void *objp;
> > +	void *ptr;
> > +	int slab_node = numa_mem_id();
> >  	struct obj_cgroup *objcg = NULL;
> >  	bool init = false;
> >
> > @@ -3299,21 +3255,49 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned lo
> >  	if (unlikely(!cachep))
> >  		return NULL;
> >
> > -	objp = kfence_alloc(cachep, orig_size, flags);
> > -	if (unlikely(objp))
> > -		goto out;
> > +	ptr = kfence_alloc(cachep, orig_size, flags);
> > +	if (unlikely(ptr))
> > +		goto out_hooks;
> >
> >  	cache_alloc_debugcheck_before(cachep, flags);
> >  	local_irq_save(save_flags);
> > -	objp = __do_cache_alloc(cachep, flags);
>
> Looks like after this patch, slab_alloc() (without a node specified)
> will not end up in __do_cache_alloc() anymore, so there's no more
> possibility of alternate_node_alloc(), which looks like a functional
> regression?
>

Ah, that was not intended. Thank you for catching this!
Will fix in v2.

Thank you so much.

> > +
> > +	if (node_match(nodeid, slab_node)) {
> > +		/*
> > +		 * Use the locally cached objects if possible.
> > +		 * However ____cache_alloc does not allow fallback
> > +		 * to other nodes. It may fail while we still have
> > +		 * objects on other nodes available.
> > +		 */
> > +		ptr = ____cache_alloc(cachep, flags);
> > +		if (ptr)
> > +			goto out;
> > +	}
> > +#ifdef CONFIG_NUMA
> > +	else if (unlikely(!get_node(cachep, nodeid))) {
> > +		/* Node not bootstrapped yet */
> > +		ptr = fallback_alloc(cachep, flags);
> > +		goto out;
> > +	}
> > +
> > +	/* ___cache_alloc_node can fall back to other nodes */
> > +	ptr = ____cache_alloc_node(cachep, flags, nodeid);
> > +#endif
> > +out:
> >  	local_irq_restore(save_flags);
> > -	objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
> > -	prefetchw(objp);
> > +	ptr = cache_alloc_debugcheck_after(cachep, flags, ptr, caller);
> > +	prefetchw(ptr);
> >  	init = slab_want_init_on_alloc(flags, cachep);
> >
> > -out:
> > -	slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init);
> > -	return objp;
> > +out_hooks:
> > +	slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr, init);
> > +	return ptr;
> > +}
> > +
> > +static __always_inline void *
> > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> > +{
> > +	return slab_alloc_node(cachep, flags, NUMA_NO_NODE, orig_size, caller);
> >  }
> >
> >  /*

--
Thank you, You are awesome!
Hyeonggon :-)