Re: [PATCH] srcutiny: Enable atomic SRCU flavor tracking

[KunWu Chan]

On Wed, Sep 2, 2026 at 11:43 PM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
>
> On Wed, Sep 02, 2026 at 05:18:50PM +0800, KunWu Chan wrote:
> > On Wed, Sep 2, 2026 at 1:11 AM Paul E. McKenney <paulmck@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Sep 01, 2026 at 03:48:15PM +0800, Kunwu Chan wrote:
> > > > From: Kunwu Chan <kunwu.chan@xxxxxxxxx>
> > > >
> > > > Set srcu_reader_flavor to SRCU_READ_FLAVOR_ATOMIC in the Tiny SRCU
> > > > atomic initialization paths, so that the entry-point checks added by
> > > > the previous commit can identify atomic SRCU domains.
> > > >
> > > > For static initialization, add a flavor parameter to
> > > > __SRCU_STRUCT_INIT() and pass SRCU_READ_FLAVOR_ATOMIC through
> > > > DEFINE_SRCU_ATOMIC() and DEFINE_STATIC_SRCU_ATOMIC().
> > > >
> > > > For dynamic initialization, initialize srcu_reader_flavor to zero in
> > > > init_srcu_struct_fields() for the generic initialization path, and
> > > > set it to SRCU_READ_FLAVOR_ATOMIC in init_srcu_struct_atomic().
> > > > Handle both CONFIG_DEBUG_LOCK_ALLOC and non-debug initialization paths.
> > > >
> > > > Enable srcu_check_read_flavor() for Tiny SRCU, matching the Tree SRCU
> > > > behavior, so that readers can verify that the requested flavor matches
> > > > the SRCU domain.
> > > >
> > > > Signed-off-by: Kunwu Chan <kunwu.chan@xxxxxxxxx>
> > >
> > > Much better!  Please see below for additional questions and comments.
> >
> > Thanks Paul for the detailed reply.
> >
> > >
> > > > ---
> > > >  include/linux/srcutiny.h | 39 +++++++++++++++++++++++++++++----------
> > > >  kernel/rcu/srcutiny.c    |  1 +
> > > >  2 files changed, 30 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> > > > index 2b293336525a..9dce4b5aa084 100644
> > > > --- a/include/linux/srcutiny.h
> > > > +++ b/include/linux/srcutiny.h
> > > > @@ -40,7 +40,7 @@ void srcu_drive_gp(struct work_struct *wp);
> > > >  void srcu_tiny_irq_work(struct irq_work *irq_work);
> > > >  void srcu_defer_drain(struct irq_work *irq_work);
> > > >
> > > > -#define __SRCU_STRUCT_INIT(name, __ignored, ___ignored, ____ignored) \
> > > > +#define __SRCU_STRUCT_INIT(name, __ignored, ___ignored, flavor)              \
> > > >  {                                                                    \
> > > >       .srcu_wq = __SWAIT_QUEUE_HEAD_INITIALIZER(name.srcu_wq),        \
> > > >       .srcu_cb_tail = &name.srcu_cb_head,                             \
> > > > @@ -49,6 +49,7 @@ void srcu_defer_drain(struct irq_work *irq_work);
> > > >       .defer_cbs = LLIST_HEAD_INIT(name.defer_cbs),                   \
> > > >       .defer_iw = { .node = { .u_flags = IRQ_WORK_HARD_IRQ },         \
> > > >                     .func = srcu_defer_drain },                       \
> > > > +     .srcu_reader_flavor = flavor,                                   \
> > >
> > > Don't we also need to define srcu_reader_flavor?  Or is this patch
> > > supposed to be applied on top of your previous one?
> >
> > Yes, this is intended to be applied on top of the previous one [1].
> > [1] https://lore.kernel.org/rcu/20260831074937.2380914-1-kunwu.chan@xxxxxxxxx/
> >
> > > >       __SRCU_DEP_MAP_INIT(name)                                       \
> > > >  }
> > > >
> > > > @@ -57,29 +58,43 @@ void srcu_defer_drain(struct irq_work *irq_work);
> > > >   * Tree SRCU, which needs some per-CPU data.
> > > >   */
> > > >  #define DEFINE_SRCU(name) \
> > > > -     struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, name)
> > > > +     struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, 0)
> > > >  #define DEFINE_STATIC_SRCU(name) \
> > > > -     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, name)
> > > > +     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, 0)
> > > >  #define DEFINE_SRCU_FAST(name) DEFINE_SRCU(name)
> > > >  #define DEFINE_STATIC_SRCU_FAST(name) \
> > > > -     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, name)
> > > > +     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, 0)
> > > >  #define DEFINE_SRCU_FAST_UPDOWN(name) DEFINE_SRCU(name)
> > > >  #define DEFINE_STATIC_SRCU_FAST_UPDOWN(name) \
> > > > -     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, name)
> > > > -#define DEFINE_SRCU_ATOMIC(name) DEFINE_SRCU(name)
> > > > +     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, 0)
> > > > +#define DEFINE_SRCU_ATOMIC(name) \
> > > > +     struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, SRCU_READ_FLAVOR_ATOMIC)
> > > >  #define DEFINE_STATIC_SRCU_ATOMIC(name) \
> > > > -     static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name, name, name)
> > > > +     static struct srcu_struct name = \
> > > > +             __SRCU_STRUCT_INIT(name, name, name, SRCU_READ_FLAVOR_ATOMIC)
> > > >
> > > >  // Dummy structure for srcu_notifier_head.
> > > >  struct srcu_usage { };
> > > >  #define __SRCU_USAGE_INIT(name) { }
> > > >  #define __init_srcu_struct_fast __init_srcu_struct
> > > >  #define __init_srcu_struct_fast_updown __init_srcu_struct
> > > > -#define __init_srcu_struct_atomic __init_srcu_struct
> > > > +#define __init_srcu_struct_atomic(ssp, name, key) \
> > > > +({ \
> > > > +     int __ret = __init_srcu_struct(ssp, name, key); \
> > > > +     if (!__ret) \
> > > > +             (ssp)->srcu_reader_flavor = SRCU_READ_FLAVOR_ATOMIC; \
> > > > +     __ret; \
> > > > +})
> > > >  #ifndef CONFIG_DEBUG_LOCK_ALLOC
> > > >  #define init_srcu_struct_fast init_srcu_struct
> > > >  #define init_srcu_struct_fast_updown init_srcu_struct
> > > > -#define init_srcu_struct_atomic init_srcu_struct
> > > > +#define init_srcu_struct_atomic(ssp)                                 \
> > > > +     ({                                                              \
> > > > +             int __ret = init_srcu_struct(ssp);                      \
> > > > +             if (!__ret)                                             \
> > > > +                     (ssp)->srcu_reader_flavor = SRCU_READ_FLAVOR_ATOMIC;\
> > > > +             __ret;                                                  \
> > > > +     })
> > > >  #endif // #ifndef CONFIG_DEBUG_LOCK_ALLOC
> > > >
> > > >  void synchronize_srcu(struct srcu_struct *ssp);
> > > > @@ -148,7 +163,11 @@ static inline void synchronize_srcu_expedited(struct srcu_struct *ssp)
> > > >  void srcu_barrier(struct srcu_struct *ssp);
> > > >
> > > >  static inline void srcu_expedite_current(struct srcu_struct *ssp) { }
> > > > -#define srcu_check_read_flavor(ssp, read_flavor) do { } while (0)
> > > > +#define srcu_check_read_flavor(ssp, read_flavor) \
> > > > +     ({ \
> > > > +             u8 __f = (ssp)->srcu_reader_flavor; \
> > > > +             WARN_ON_ONCE(__f && !(__f & (read_flavor))); \
> > > > +     })
> > >
> > > Why not make this a static inline function, along with
> > > __init_srcu_struct_atomic() and init_srcu_struct_atomic() above?
> > > That should simplify the code.
> >
> > Thanks. I’ll rework the initialization helpers as static inline
> > functions as suggested.
> >
> > > >  /* Defined here to avoid size increase for non-torture kernels. */
> > > >  static inline void srcu_torture_stats_print(struct srcu_struct *ssp,
> > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> > > > index 22f7716cbb0e..873b30ccf563 100644
> > > > --- a/kernel/rcu/srcutiny.c
> > > > +++ b/kernel/rcu/srcutiny.c
> > > > @@ -42,6 +42,7 @@ static int init_srcu_struct_fields(struct srcu_struct *ssp)
> > > >       ssp->srcu_gp_running = false;
> > > >       ssp->srcu_gp_waiting = false;
> > > >       ssp->srcu_atomic_gp_flag = 0;
> > > > +     ssp->srcu_reader_flavor = 0;
> > > >       ssp->srcu_idx = 0;
> > > >       ssp->srcu_idx_max = 0;
> > > >       INIT_WORK(&ssp->srcu_work, srcu_drive_gp);
> > > > --
> > > > 2.43.0
> > >
> > > One of the purposes of Tiny SRCU is to be tiny, and one could argue
> > > that Tiny SRCU already has too much diagnostic code.  Should we include
> > > these patches at all?  If we should, shouldn't they be under something
> > > like CONFIG_PROVE_RCU?  Should some of the existing Tiny SRCU debugging
> > > code also be under something like CONFIG_PROVE_RCU?
> > >
> > > Thoughts?
> >
> > I agree that Tiny SRCU should keep its core as small as possible, so I think
> > it is worth reconsidering how much diagnostic machinery we want to add here.
> >
> > For srcu_reader_flavor, I did have some hesitation when adding it.
> > Tiny SRCU did not previously have the concept of a reader flavor; I
> > mainly added it
> > to distinguish Tiny Atomic SRCU, following the approach used by Tree
> > SRCU. If we still
> > need state for this purpose, perhaps a one-bit flag would be sufficient.
> >
> > One question is whether the flavor checking really needs to be part of
> > srcutiny.c.
> > I think there are two somewhat different cases:
> > 1: checking for invalid API usage, where the purpose is to detect or
> > stop misuse;
> > 2: defensive handling of an invalid call, where we redirect to a safe
> > implementation,
> >     like in synchronize_srcu.
> >
> > The first case seems more like diagnostic functionality and could
> > potentially be handled
> > in the common SRCU API layer under CONFIG_PROVE_RCU. The second case affects
> > runtime behavior and should remain independent of CONFIG_PROVE_RCU.
> >
> > This makes me wonder whether the common flavor validation could live in srcu.h,
> > as part of the common SRCU interface, rather than adding more checking
> > logic to srcutiny.c.
> > That could centralize the validation and keep the Tiny SRCU
> > implementation easier to follow,
> > although it would add another wrapper layer, such as a macro or static inline.
>
> Another approach is to let the checking in Tree SRCU cover this.
> That way, the only misuses of the atomic SRCU API that are missed are
> those that appear only in CONFIG_SMP=n code.  And right now, there isn't
> any such uses.  There might be in the future, but perhaps this is quite
> unlikely.

Thanks Paul. That makes sense to me.

If Tree SRCU already catches the relevant flavor misuse for CONFIG_SMP=y,
and there are currently no such uses that are specific to CONFIG_SMP=n, then
I agree that adding flavor tracking/checking to Tiny SRCU just for that
coverage would not be worthwhile. :)

I originally added the Tiny SRCU flavor tracking mainly for defensive handling
and to keep the behavior consistent with Tree SRCU. But given the Tiny SRCU
size goal, I agree that this additional diagnostic coverage is not worth
the extra complexity. :-)

I'll drop the Tiny SRCU flavor-tracking part.

Thanks,
KunWu

>
>                                                         Thanx, Paul