Re: [PATCH 6/6] slab: Introduce kmalloc_nolock() and kfree_nolock().

[Harry Yoo <harry.yoo@oracle.com>]

On Tue, May 06, 2025 at 02:01:48PM +0200, Vlastimil Babka wrote:
> On 5/1/25 05:27, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@kernel.org>
> > 
> > kmalloc_nolock() relies on ability of local_lock to detect the situation
> > when it's locked.
> > In !PREEMPT_RT local_lock_is_locked() is true only when NMI happened in
> > irq saved region that protects _that specific_ per-cpu kmem_cache_cpu.
> > In that case retry the operation in a different kmalloc bucket.
> > The second attempt will likely succeed, since this cpu locked
> > different kmem_cache_cpu.
> > When lock_local_is_locked() sees locked memcg_stock.stock_lock
> > fallback to atomic operations.
> > 
> > Similarly, in PREEMPT_RT local_lock_is_locked() returns true when
> > per-cpu rt_spin_lock is locked by current task. In this case re-entrance
> > into the same kmalloc bucket is unsafe, and kmalloc_nolock() tries
> > a different bucket that is most likely is not locked by current
> > task. Though it may be locked by a different task it's safe to
> > rt_spin_lock() on it.
> > 
> > Similar to alloc_pages_nolock() the kmalloc_nolock() returns NULL
> > immediately if called from hard irq or NMI in PREEMPT_RT.
> > 
> > Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> 

... snip ...

> > @@ -4354,6 +4406,88 @@ void *__kmalloc_noprof(size_t size, gfp_t flags)
> >  }
> >  EXPORT_SYMBOL(__kmalloc_noprof);
> >  
> > +/**
> > + * kmalloc_nolock - Allocate an object of given size from any context.
> > + * @size: size to allocate
> > + * @gfp_flags: GFP flags. Only __GFP_ACCOUNT, __GFP_ZERO allowed.
> > + * @node: node number of the target node.
> > + *
> > + * Return: pointer to the new object or NULL in case of error.
> > + * NULL does not mean EBUSY or EAGAIN. It means ENOMEM.
> > + * There is no reason to call it again and expect !NULL.
> > + */
> > +void *kmalloc_nolock_noprof(size_t size, gfp_t gfp_flags, int node)
> > +{
> > +	gfp_t alloc_gfp = __GFP_NOWARN | __GFP_NOMEMALLOC | gfp_flags;
> > +	struct kmem_cache *s;
> > +	bool can_retry = true;
> > +	void *ret = ERR_PTR(-EBUSY);
> > +
> > +	VM_WARN_ON_ONCE(gfp_flags & ~(__GFP_ACCOUNT | __GFP_ZERO));
> > +
> > +	if (unlikely(size > KMALLOC_MAX_CACHE_SIZE))
> > +		return NULL;
> > +	if (unlikely(!size))
> > +		return ZERO_SIZE_PTR;
> > +
> > +	if (!USE_LOCKLESS_FAST_PATH() && (in_nmi() || in_hardirq()))
> > +		/* kmalloc_nolock() in PREEMPT_RT is not supported from irq */
> > +		return NULL;
> > +retry:
> > +	s = kmalloc_slab(size, NULL, alloc_gfp, _RET_IP_);
> 
> The idea of retrying on different bucket is based on wrong assumptions and
> thus won't work as you expect. kmalloc_slab() doesn't select buckets truly
> randomly, but deterministically via hashing from a random per-boot seed and
> the _RET_IP_, as the security hardening goal is to make different kmalloc()
> callsites get different caches with high probability.

It's not retrying with the same size, so I don't think it's relying on any
assumption about random kmalloc caches. (yeah, it wastes some memory if
allocated from the next size bucket)

	if (PTR_ERR(ret) == -EBUSY) {
		if (can_retry) {
			/* pick the next kmalloc bucket */
			size = s->object_size + 1;
			/*
			 * Another alternative is to
			 * if (memcg) alloc_gfp &= ~__GFP_ACCOUNT;
			 * else if (!memcg) alloc_gfp |= __GFP_ACCOUNT;
			 * to retry from bucket of the same size.
			 */
			can_retry = false;
			goto retry;
		}
		ret = NULL;
	}

By the way, it doesn't check if a kmalloc cache that can serve
(s->object_size + 1) allocations actually exists, which is not true for
the largest kmalloc cache?

-- 
Cheers,
Harry / Hyeonggon