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;
MIME-Version: 1.0
References: <20230411130854.46795-1-zhengqi.arch@bytedance.com>
 <c6ea3b17-a89c-6f66-5c86-967f1da601b4@suse.cz> <ccaf5e8e-3457-a2cf-b6eb-794cbf1b46f5@bytedance.com>
 <f54cfeb9-f1c3-e656-d344-4cbf97a7c28a@suse.cz> <932bf921-a076-e166-4f95-1adb24d544cf@bytedance.com>
 <ZDZG6wlWIE4dzd4W@Boquns-Mac-mini.local> <PH0PR11MB58805856C1C47D49D1BA8092DA9B9@PH0PR11MB5880.namprd11.prod.outlook.com>
 <eef7fefe-3a0c-31de-0fe3-c8ca8294a0df@bytedance.com>
In-Reply-To: <eef7fefe-3a0c-31de-0fe3-c8ca8294a0df@bytedance.com>
From:   Joel Fernandes <joel@joelfernandes.org>
Date:   Wed, 12 Apr 2023 20:24:19 -0400
Message-ID: <CAEXW_YQhLsf7NsbGZa5u9kcr2hesAuK_5RKZh+qtSfn_Ha2ugg@mail.gmail.com>
Subject: Re: [PATCH] mm: slub: annotate kmem_cache_node->list_lock as raw_spinlock
To:     Qi Zheng <zhengqi.arch@bytedance.com>,
        Steven Rostedt <rostedt@goodmis.org>
Cc:     "Zhang, Qiang1" <qiang1.zhang@intel.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "42.hyeyoo@gmail.com" <42.hyeyoo@gmail.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "roman.gushchin@linux.dev" <roman.gushchin@linux.dev>,
        "iamjoonsoo.kim@lge.com" <iamjoonsoo.kim@lge.com>,
        "rientjes@google.com" <rientjes@google.com>,
        "penberg@kernel.org" <penberg@kernel.org>,
        "cl@linux.com" <cl@linux.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Zhao Gongyi <zhaogongyi@bytedance.com>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        RCU <rcu@vger.kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Apr 12, 2023 at 2:57=E2=80=AFAM Qi Zheng <zhengqi.arch@bytedance.co=
m> 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 debu=
g_obj_descr *descr, int onstack
> >          unsigned long flags;
> >
> >          /*
> > -        * On RT enabled kernels the pool refill must happen in preempt=
ible
> > +        * The pool refill must happen in preemptible
> >           * context:
> >           */
> > -       if (!IS_ENABLED(CONFIG_PREEMPT_RT) || preemptible())
> > +       if (preemptible())
> >                  fill_pool();
> >
> >          db =3D 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, w=
hich
> >>>
> >>> 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 typ=
e.
> >>>
> >>> 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 h=
appen
> >>> 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 soluti=
on. :)
> >>>
> >>> Maybe, but given above, I doubt it's this one.
> >>>
> >>>>
> >>>>>
> >>>>
> >>>
> >>
> >> --
> >> Thanks,
> >> Qi
>
> --
> Thanks,
> Qi