On Wed, Feb 26, 2020 at 02:14:30PM +0800, Boqun Feng wrote:
> On Tue, Feb 25, 2020 at 07:14:55PM -0800, Paul E. McKenney wrote:
> > On Tue, Feb 25, 2020 at 06:24:36PM +0800, Boqun Feng wrote:
> > > Hi Paul,
> > >
> > > On Fri, Feb 14, 2020 at 03:56:07PM -0800, paulmck@xxxxxxxxxx wrote:
> > > > From: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> > > >
> > > > Currently, rcu_barrier() ignores offline CPUs, However, it is possible
> > > > for an offline no-CBs CPU to have callbacks queued, and rcu_barrier()
> > > > must wait for those callbacks. This commit therefore makes rcu_barrier()
> > > > directly invoke the rcu_barrier_func() with interrupts disabled for such
> > > > CPUs. This requires passing the CPU number into this function so that
> > > > it can entrain the rcu_barrier() callback onto the correct CPU's callback
> > > > list, given that the code must instead execute on the current CPU.
> > > >
> > > > While in the area, this commit fixes a bug where the first CPU's callback
> > > > might have been invoked before rcu_segcblist_entrain() returned, which
> > > > would also result in an early wakeup.
> > > >
> > > > Fixes: 5d6742b37727 ("rcu/nocb: Use rcu_segcblist for no-CBs CPUs")
> > > > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > > Cc: <stable@xxxxxxxxxxxxxxx> # 5.5.x
> > > > ---
> > > > include/trace/events/rcu.h | 1 +
> > > > kernel/rcu/tree.c | 32 ++++++++++++++++++++------------
> > > > 2 files changed, 21 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/include/trace/events/rcu.h b/include/trace/events/rcu.h
> > > > index 5e49b06..d56d54c 100644
> > > > --- a/include/trace/events/rcu.h
> > > > +++ b/include/trace/events/rcu.h
> > > > @@ -712,6 +712,7 @@ TRACE_EVENT_RCU(rcu_torture_read,
> > > > * "Begin": rcu_barrier() started.
> > > > * "EarlyExit": rcu_barrier() piggybacked, thus early exit.
> > > > * "Inc1": rcu_barrier() piggyback check counter incremented.
> > > > + * "OfflineNoCBQ": rcu_barrier() found offline no-CBs CPU with callbacks.
> > > > * "OnlineQ": rcu_barrier() found online CPU with callbacks.
> > > > * "OnlineNQ": rcu_barrier() found online CPU, no callbacks.
> > > > * "IRQ": An rcu_barrier_callback() callback posted on remote CPU.
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index d15041f..160643e 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -3098,9 +3098,10 @@ static void rcu_barrier_callback(struct rcu_head *rhp)
> > > > /*
> > > > * Called with preemption disabled, and from cross-cpu IRQ context.
> > > > */
> > > > -static void rcu_barrier_func(void *unused)
> > > > +static void rcu_barrier_func(void *cpu_in)
> > > > {
> > > > - struct rcu_data *rdp = raw_cpu_ptr(&rcu_data);
> > > > + uintptr_t cpu = (uintptr_t)cpu_in;
> > > > + struct rcu_data *rdp = per_cpu_ptr(&rcu_data, cpu);
> > > >
> > > > rcu_barrier_trace(TPS("IRQ"), -1, rcu_state.barrier_sequence);
> > > > rdp->barrier_head.func = rcu_barrier_callback;
> > > > @@ -3127,7 +3128,7 @@ static void rcu_barrier_func(void *unused)
> > > > */
> > > > void rcu_barrier(void)
> > > > {
> > > > - int cpu;
> > > > + uintptr_t cpu;
> > > > struct rcu_data *rdp;
> > > > unsigned long s = rcu_seq_snap(&rcu_state.barrier_sequence);
> > > >
> > > > @@ -3150,13 +3151,14 @@ void rcu_barrier(void)
> > > > rcu_barrier_trace(TPS("Inc1"), -1, rcu_state.barrier_sequence);
> > > >
> > > > /*
> > > > - * Initialize the count to one rather than to zero in order to
> > > > - * avoid a too-soon return to zero in case of a short grace period
> > > > - * (or preemption of this task). Exclude CPU-hotplug operations
> > > > - * to ensure that no offline CPU has callbacks queued.
> > > > + * Initialize the count to two rather than to zero in order
> > > > + * to avoid a too-soon return to zero in case of an immediate
> > > > + * invocation of the just-enqueued callback (or preemption of
> > > > + * this task). Exclude CPU-hotplug operations to ensure that no
> > > > + * offline non-offloaded CPU has callbacks queued.
> > > > */
> > > > init_completion(&rcu_state.barrier_completion);
> > > > - atomic_set(&rcu_state.barrier_cpu_count, 1);
> > > > + atomic_set(&rcu_state.barrier_cpu_count, 2);
> > > > get_online_cpus();
> > > >
> > > > /*
> > > > @@ -3166,13 +3168,19 @@ void rcu_barrier(void)
> > > > */
> > > > for_each_possible_cpu(cpu) {
> > > > rdp = per_cpu_ptr(&rcu_data, cpu);
> > > > - if (!cpu_online(cpu) &&
> > > > + if (cpu_is_offline(cpu) &&
> > > > !rcu_segcblist_is_offloaded(&rdp->cblist))
> > > > continue;
> > > > - if (rcu_segcblist_n_cbs(&rdp->cblist)) {
> > > > + if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_online(cpu)) {
> > > > rcu_barrier_trace(TPS("OnlineQ"), cpu,
> > > > rcu_state.barrier_sequence);
> > > > - smp_call_function_single(cpu, rcu_barrier_func, NULL, 1);
> > > > + smp_call_function_single(cpu, rcu_barrier_func, (void *)cpu, 1);
> > > > + } else if (cpu_is_offline(cpu)) {
> > >
> > > I wonder whether this should be:
> > >
> > > else if (rcu_segcblist_n_cbs(&rdp->cblist) && cpu_is_offline(cpu))
> > >
> > > ? Because I think we only want to queue the barrier call back if there
> > > are callbacks for a particular CPU. Am I missing something subtle?
> >
> > I don't believe that you are missing anything at all!
> >
> > Thank you very much -- this bug would not have shown up in any validation
> > setup that I am aware of. ;-)
> >
> > Thanx, Paul
> >
> > > Regards,
> > > Boqun
> > >
> > > > + rcu_barrier_trace(TPS("OfflineNoCBQ"), cpu,
> > > > + rcu_state.barrier_sequence);
> > > > + local_irq_disable();
> > > > + rcu_barrier_func((void *)cpu);
> > > > + local_irq_enable();
>
> Another (interesting) thing I found here is that we actually don't need
> the irq-off section to call rcu_barrier_func() in this branch. Because
> the target CPU is offlined, so only the cblist is only accessed at two
> places, IIUC, one is the rcuo kthread and one is here (in
> rcu_barrier()), and both places are in the process context rather than
> irq context, so irq-off is not required to prevent the deadlock.
>
> But yes, I know, if we drop the local_irq_disable/enable() pair here,
> it will make lockdep very unhappy ;-)
And acquiring ->nocb_lock with interrupts enabled would be rather scary.
And probably would be an accident waiting to happen. So I am happy to
disable interrupts on this path, given that it should be infrequent,
only being executed for a short time after a no-CBs CPU goes offline.
Much nicer to let lockdep do its thing than to have to second-guess it
on every change that involves acquiring ->nocb_lock in an interrupt
handler! ;-)
Thanx, Paul
> Regards,
> Boqun
>
> > > > } else {
> > > > rcu_barrier_trace(TPS("OnlineNQ"), cpu,
> > > > rcu_state.barrier_sequence);
> > > > @@ -3184,7 +3192,7 @@ void rcu_barrier(void)
> > > > * Now that we have an rcu_barrier_callback() callback on each
> > > > * CPU, and thus each counted, remove the initial count.
> > > > */
> > > > - if (atomic_dec_and_test(&rcu_state.barrier_cpu_count))
> > > > + if (atomic_sub_and_test(2, &rcu_state.barrier_cpu_count))
> > > > complete(&rcu_state.barrier_completion);
> > > >
> > > > /* Wait for all rcu_barrier_callback() callbacks to be invoked. */
> > > > --
> > > > 2.9.5
> > > >
