Thanks Paul for the review and suggestions!
On Thu, Oct 1, 2026 at 12:42 AM Paul E. McKenney <[email protected]> wrote:
>
> On Sun, Sep 27, 2026 at 06:03:59PM +0800, Kunwu Chan wrote:
> > Add support for scaling atomic SRCU grace periods using
> > srcu_read_lock_atomic()/srcu_read_unlock_atomic() and
> > synchronize_srcu_atomic().
> >
> > Signed-off-by: Kunwu Chan <[email protected]>
>
> Nice, thank you!!!
>
> Some suggestions below.
>
> Thanx, Paul
>
> > ---
> > kernel/rcu/rcuscale.c | 47 ++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/rcu/rcuscale.c b/kernel/rcu/rcuscale.c
> > index 5aca31c3c01d..86f1a912ebee 100644
> > --- a/kernel/rcu/rcuscale.c
> > +++ b/kernel/rcu/rcuscale.c
> > @@ -264,6 +264,51 @@ static struct rcu_scale_ops srcu_ops = {
> > .name = "srcu"
> > };
> >
> > +static struct srcu_struct srcu_atomic_ctlp;
>
> The trailing "p" suggests that this is a pointer, for example, see the
> difference between srcu_ctl_scale and srcu_ctlp. You could make this
> consistent with "srcud" and save a few lines of code as follows:
>
> static struct srcu_struct srcua;
>
The `srcu_ctlp` reuse approach looks good. I will switch to a separate
`srcua` instance, assign `srcu_ctlp` during init, and reuse the existing
SRCU helpers.
> > +static int srcu_scale_atomic_read_lock(void)
> > +{
> > + return srcu_read_lock_atomic(&srcu_atomic_ctlp);
> > +}
> > +
> > +static void srcu_scale_atomic_read_unlock(int idx)
> > +{
> > + srcu_read_unlock_atomic(&srcu_atomic_ctlp, idx);
> > +}
> > +
> > +static void srcu_scale_atomic_synchronize(void)
> > +{
> > + synchronize_srcu_atomic(&srcu_atomic_ctlp);
> > +}
> > +
> > +static void srcu_atomic_scale_init(void)
> > +{
> srcu_ctlp = &srcua;
> > + init_srcu_struct_atomic(srcu_ctlp);
> > +}
> > +
> > +static void srcu_atomic_scale_cleanup(void)
> > +{
> > + cleanup_srcu_struct(&srcu_atomic_ctlp);
> > +}
>
> You can then drop srcu_atomic_scale_cleanup() and just use the existing
> srcu_sync_scale_cleanup().
>
> > +static unsigned long srcu_atomic_scale_completed(void)
> > +{
> > + return srcu_batches_completed(&srcu_atomic_ctlp);
> > +}
>
> You can also drop srcu_atomic_scale_completed() and just use the existing
> srcu_scale_completed(). Of course, both this and the above require
> adjusting the srcu_atomic_ops initializer.
>
That makes sense. I will drop `srcu_atomic_scale_cleanup()` and
`srcu_atomic_scale_completed()`, and adjust the ops initializer
accordingly.
> > +static struct rcu_scale_ops srcu_atomic_ops = {
> > + .ptype = SRCU_FLAVOR,
> > + .init = srcu_atomic_scale_init,
> > + .cleanup = srcu_atomic_scale_cleanup,
> > + .readlock = srcu_scale_atomic_read_lock,
> > + .readunlock = srcu_scale_atomic_read_unlock,
> > + .get_gp_seq = srcu_atomic_scale_completed,
> > + .gp_diff = rcu_seq_diff,
> > + .exp_completed = srcu_atomic_scale_completed,
> > + .sync = srcu_scale_atomic_synchronize,
>
> You can also then do this to get more debug data:
>
> .stats = srcu_scale_stats,
I will add .stats = srcu_scale_stats as suggested.
>
> > + .name = "srcu_atomic"
>
> Then this could be "srcua". Or maybe more typing on the rcuscale
> scale_type module parameter is OK. If so, maybe we should change
> "srcud" to "srcu_dynamic", but this would require quite a few other
> changes as well. (At least if we want the rcutorture names to be
> consistent.)
>
> Thoughts?
>
For the name, I will use `srcua` to stay consistent with the existing
`srcud` naming style.
If we want to improve readability without renaming the existing scale
types, we could also
add a short comment or update the test scripts to mention that `srcua`
refers to atomic SRCU.
If we prefer more descriptive names such as `srcu_atomic` and `srcu_dynamic`,
I can help with a follow-up cleanup to rename the existing `srcud` as
well and keep
the related rcutorture and rcuscale names consistent.
Thanks,
Kunwu
> > +};
> > +
> > static struct srcu_struct srcud;
> >
> > static void srcu_sync_scale_init(void)
> > @@ -1176,7 +1221,7 @@ rcu_scale_init(void)
> > long i;
> > long j;
> > static struct rcu_scale_ops *scale_ops[] = {
> > - &rcu_ops, &srcu_ops, &srcud_ops,
> > + &rcu_ops, &srcu_ops, &srcu_atomic_ops, &srcud_ops,
> > TASKS_OPS TASKS_RUDE_OPS TASKS_TRACING_OPS
> > HAZPTR_SCALE_OPS
> > };
> > --
> > 2.43.0
> >
