On Mon, Nov 11, 2024 at 06:24:58PM +0530, Neeraj Upadhyay wrote:
>
> >
> > /*
> > - * Returns approximate total of the readers' ->srcu_lock_count[] values
> > - * for the rank of per-CPU counters specified by idx.
> > + * Computes approximate total of the readers' ->srcu_lock_count[] values
> > + * for the rank of per-CPU counters specified by idx, and returns true if
> > + * the caller did the proper barrier (gp), and if the count of the locks
> > + * matches that of the unlocks passed in.
> > */
> > -static unsigned long srcu_readers_lock_idx(struct srcu_struct *ssp, int idx)
> > +static bool srcu_readers_lock_idx(struct srcu_struct *ssp, int idx, bool gp, unsigned long unlocks)
> > {
> > int cpu;
> > + unsigned long mask = 0;
> > unsigned long sum = 0;
> >
> > for_each_possible_cpu(cpu) {
> > struct srcu_data *sdp = per_cpu_ptr(ssp->sda, cpu);
> >
> > sum += atomic_long_read(&sdp->srcu_lock_count[idx]);
> > + if (IS_ENABLED(CONFIG_PROVE_RCU))
> > + mask = mask | READ_ONCE(sdp->srcu_reader_flavor);
> > }
> > - return sum;
> > + WARN_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && (mask & (mask - 1)),
> > + "Mixed reader flavors for srcu_struct at %ps.\n", ssp);
>
> I am trying to understand the (unlikely) case where synchronize_srcu() is done before any
> srcu reader lock/unlock lite call is done. Can new SRCU readers fail to observe the
> updates?
If a SRCU reader fail to observe the index flip, then isn't it the case
that the synchronize_rcu() invoked from srcu_readers_active_idx_check()
must wait on it?
> > + if (mask & SRCU_READ_FLAVOR_LITE && !gp)
> > + return false;
>
> So, srcu_readers_active_idx_check() can potentially return false for very long
> time, until the CPU executing srcu_readers_active_idx_check() does
> at least one read lock/unlock lite call?
That is correct. The theory is that until after an srcu_read_lock_lite()
has executed, there is no need to wait on it. Does the practice match the
theory in this case, or is there some sequence of events that I missed?
> > + return sum == unlocks;
> > }
> >
> > /*
> > @@ -473,6 +482,7 @@ static unsigned long srcu_readers_unlock_idx(struct srcu_struct *ssp, int idx)
> > */
> > static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> > {
> > + bool did_gp = !!(raw_cpu_read(ssp->sda->srcu_reader_flavor) & SRCU_READ_FLAVOR_LITE);
>
> sda->srcu_reader_flavor is only set when CONFIG_PROVE_RCU is enabled. But we
> need the reader flavor information for srcu lite variant to work. So, lite
> variant does not work when CONFIG_PROVE_RCU is disabled. Am I missing something
> obvious here?
At first glance, it appears that I am the one who missed something obvious.
Including in testing, which failed to uncover this issue.
Thank you for the careful reviews!
Thanx, Paul
> - Neeraj
>
> > unsigned long unlocks;
> >
> > unlocks = srcu_readers_unlock_idx(ssp, idx);
> > @@ -482,13 +492,16 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> > * unlock is counted. Needs to be a smp_mb() as the read side may
> > * contain a read from a variable that is written to before the
> > * synchronize_srcu() in the write side. In this case smp_mb()s
> > - * A and B act like the store buffering pattern.
> > + * A and B (or X and Y) act like the store buffering pattern.
> > *
> > - * This smp_mb() also pairs with smp_mb() C to prevent accesses
> > - * after the synchronize_srcu() from being executed before the
> > - * grace period ends.
> > + * This smp_mb() also pairs with smp_mb() C (or, in the case of X,
> > + * Z) to prevent accesses after the synchronize_srcu() from being
> > + * executed before the grace period ends.
> > */
> > - smp_mb(); /* A */
> > + if (!did_gp)
> > + smp_mb(); /* A */
> > + else
> > + synchronize_rcu(); /* X */
> >
> > /*
> > * If the locks are the same as the unlocks, then there must have
> > @@ -546,7 +559,7 @@ static bool srcu_readers_active_idx_check(struct srcu_struct *ssp, int idx)
> > * which are unlikely to be configured with an address space fully
> > * populated with memory, at least not anytime soon.
> > */
> > - return srcu_readers_lock_idx(ssp, idx) == unlocks;
> > + return srcu_readers_lock_idx(ssp, idx, did_gp, unlocks);
> > }
> >
>
