Re: [PATCH v7 6/6] rcu/segcblist: Add additional comments to explain smp_mb()

[Frederic Weisbecker <frederic@kernel.org>]

On Wed, Oct 21, 2020 at 11:57:04AM -0700, Joel Fernandes wrote:
> On Mon, Oct 19, 2020 at 5:37 AM Frederic Weisbecker <frederic@kernel.org> wrote:
> > Now, reading the documentation of rcu_barrier() (thanks to you!):
> >
> >     Pseudo-code using rcu_barrier() is as follows:
> >
> >    1. Prevent any new RCU callbacks from being posted.
> >    2. Execute rcu_barrier().
> >    3. Allow the module to be unloaded.
> >
> 
> Basically, you are saying that if all CPUs agree that len == 0
> henceforth (through other memory barriers), then callback enqueuing
> does not need a memory barrier before setting length to 0.

I think setting length to 0 isn't much an issue. At worst we send a useless
IPI and queue a needless callback. But incrementing from 0 to 1 is precisely
what we don't want to miss.

> I think that makes sense but is it worth removing the memory barrier
> before WRITE(len, 1) and hoping after #1, the caller would have
> ensured things are fine? Also I am not sure if the above is the only
> usecase for rcu_barrier().

I'm not sure either. Also I need to check your scenario again.

> > cancel_work_sync() also seem to really sync as well. I'm less sure about del_timer_sync().
> >
> > Say we have:
> >
> > expire_timers (CPU 0)                               CPU 1
> > -------------                                       -----------
> > detach_timer(timer)
> > raw_spin_unlock(&base->lock);
> > call_timer_fn(timer, fn, baseclk);
> >    -> enqueue callback
> > //would need at least smp_wmb() here

Aah, my bad, the smp_mb() after inc_len does that.

> > base->running_timer = NULL;
> >
> >                                                     del_timer_sync() {
> >                                                         raw_spin_lock(&base->lock);
> >                                                         if (base->running_timer != timer)
> >                                                             ret = detach_if_pending(timer, base, true);
> >                                                                 if (!timer_pending())
> >                                                                     return 0;
> >                                                         raw_spin_unlock(&base->lock);
> >                                                     }
> >                                                     //would need at least smp_rmb() here

And rcu_seq_start() implies that, although I'm not sure that's what was intended.
So we are good.

> 
> Regarding "would need at least smp_rmb.." :
> But the rcu_barrier() has the control dependency we discussed in last
> emails, between READ(len) and whatever follows the rcu_barrier().
> That itself will provide the ordering right?

I'm not sure that was enough. The len itself has to be synchronized against
whatever callback enqueuer that got stopped.

> I could be missing something too :-/. But I'll include this patch in
> my next posting anyway and let us also maybe see if Paul disagrees.

Ok.

Thanks!