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@xxxxxxxxxx> 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!
