Re: [PATCH] mm: slub: annotate kmem_cache_node->list_lock as raw_spinlock

[Joel Fernandes <joel@joelfernandes.org>]

On Wed, Apr 12, 2023 at 2:57 AM Qi Zheng <zhengqi.arch@bytedance.com> wrote:
>
>
>
> On 2023/4/12 14:44, Zhang, Qiang1 wrote:
[..]
> > Maybe no need to convert ->list_lock to raw_spinlock.
> >
> > --- a/lib/debugobjects.c
> > +++ b/lib/debugobjects.c
> > @@ -562,10 +562,10 @@ __debug_object_init(void *addr, const struct debug_obj_descr *descr, int onstack
> >          unsigned long flags;
> >
> >          /*
> > -        * On RT enabled kernels the pool refill must happen in preemptible
> > +        * The pool refill must happen in preemptible
> >           * context:
> >           */
> > -       if (!IS_ENABLED(CONFIG_PREEMPT_RT) || preemptible())
> > +       if (preemptible())
> >                  fill_pool();
> >
> >          db = get_bucket((unsigned long) addr);
>
> Ah, this does fix the warning I was encountered!

Actually fill_pool() should be safe to call on !CONFIG_PREEMPT_RT
kernels as it is GFP_ATOMIC, however with the above change, that goes
away just to satisfy a false-positive report. Because now all
!preemptible() sections on !CONFIG_PREEMPT_RT kernels cannot call
fill_pool(), right? So you will not end up filling the pool when it is
safe to do so?

I think it would be better to fix PROVE_LOCKING / CONFIG_PREEMPT_RT
instead of degrading !CONFIG_PREEMPT_RT just to satisfy a
false-positive report.

+Steven Rostedt as well.

thanks,

 - Joel


>
> >
> >
> >
> > Thanks
> > Zqiang
> >
> >>
> >>
> >> Regards,
> >> Boqun
> >>
> >>>>
> >>>> It's indeed unfortunate for the warning in the commit message. But
> >>>> functions like kmem_cache_alloc(GFP_ATOMIC) may indeed be called
> >>>> in the critical section of raw_spinlock or in the hardirq context, which
> >>>
> >>> Hmm, I thought they may not, actually.
> >>>
> >>>> will cause problem in the PREEMPT_RT kernel. So I still think it is
> >>>> reasonable to convert kmem_cache_node->list_lock to raw_spinlock type.
> >>>
> >>> It wouldn't be the complete solution anyway. Once we allow even a GFP_ATOMIC
> >>> slab allocation for such context, it means also page allocation can happen
> >>> to refill the slabs, so lockdep will eventually complain about zone->lock,
> >>> and who knows what else.
> >>
> >> Oh, indeed. :(
> >>
> >>>
> >>>> In addition, there are many fix patches for this kind of warning in the
> >>>> git log, so I also think there should be a general and better solution. :)
> >>>
> >>> Maybe, but given above, I doubt it's this one.
> >>>
> >>>>
> >>>>>
> >>>>
> >>>
> >>
> >> --
> >> Thanks,
> >> Qi
>
> --
> Thanks,
> Qi