On Wed, Sep 2, 2026 at 1:11 AM Paul E. McKenney <[email protected]> wrote:
>
> On Tue, Sep 01, 2026 at 03:48:15PM +0800, Kunwu Chan wrote:
> > From: Kunwu Chan <[email protected]>
> >
> > 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 <[email protected]>
>
> 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/[email protected]/
>
> > __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.
Thanks,
Kunwu
>
> Thanx, Paul
