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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Tue, May 6, 2025 at 5:31 PM Harry Yoo <harry.yoo@oracle.com> wrote:
>
> 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?

Good catch.
I need to add a check for s->object_size + 1 < KMALLOC_MAX_CACHE_SIZE.