Messages in this thread | | | Date | Thu, 16 Oct 2008 08:45:48 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] rcupdate: fix 2 bugs of rcu_barrier*() |
| |
On Thu, Oct 16, 2008 at 04:51:56PM +0800, Lai Jiangshan wrote: > > current rcu_barrier_bh() is like this: > > void rcu_barrier_bh(void) > { > BUG_ON(in_interrupt()); > /* Take cpucontrol mutex to protect against CPU hotplug */ > mutex_lock(&rcu_barrier_mutex); > init_completion(&rcu_barrier_completion); > atomic_set(&rcu_barrier_cpu_count, 0); > /* > * The queueing of callbacks in all CPUs must be atomic with > * respect to RCU, otherwise one CPU may queue a callback, > * wait for a grace period, decrement barrier count and call > * complete(), while other CPUs have not yet queued anything. > * So, we need to make sure that grace periods cannot complete > * until all the callbacks are queued. > */ > rcu_read_lock(); > on_each_cpu(rcu_barrier_func, (void *)RCU_BARRIER_BH, 1); > rcu_read_unlock(); > wait_for_completion(&rcu_barrier_completion); > mutex_unlock(&rcu_barrier_mutex); > } > > this is bug, rcu_read_lock() cannot make sure that "grace periods for RCU_BH > cannot complete until all the callbacks are queued". > it only make sure that race periods for RCU cannot complete > until all the callbacks are queued. > > so we must use rcu_read_lock_bh() for rcu_barrier_bh(). > like this: > > void rcu_barrier_bh(void) > { > ...... > rcu_read_lock_bh(); > on_each_cpu(rcu_barrier_func, (void *)RCU_BARRIER_BH, 1); > rcu_read_unlock_bh(); > ...... > } > > and also rcu_barrier() rcu_barrier_sched() are implemented like this. > it will bring a lot of duplicate code. My patch uses another way to > fix this bug, please see the comment of my patch.
Excellent catch!!! I had incorrectly convinced myself that because RCU read-side implies an RCU_BH and RCU_SCHED that I could simply use an RCU read-side critical section. Thank you for finding this!
Just out of curiosity, did an actual oops/hang lead you to this bug, or did you find it by inspection?
> Bug 2: > on_each_cpu() do not imply wmb, so we need a explicit wmb. > I became a paranoid too.
Actually, there is a memory barrier in the list_add_tail_rcu() in the implementation of smp_call_function(), and furthermore, the way that atomic operations work on all architectures I am aware of removes the need for the memory barrier. Nevertheless, I have absolutely no objection to adding this memory barrier. This code path is used infrequently and has high overhead anyway, so I agree that making it easy to understand is the correct approach. If it were on the read side, I would argue. ;-)
In any case, I must agree that you are doing a good job of learning to be paranoid!
The only change I suggest is to rewrite the comments as shown below.
With that update, this change should be applied.
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com> > --- > diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c > index 467d594..a667e21 100644 > --- a/kernel/rcupdate.c > +++ b/kernel/rcupdate.c > @@ -119,18 +119,23 @@ static void _rcu_barrier(enum rcu_barrier type) > /* Take cpucontrol mutex to protect against CPU hotplug */ > mutex_lock(&rcu_barrier_mutex); > init_completion(&rcu_barrier_completion); > - atomic_set(&rcu_barrier_cpu_count, 0); > /* > - * The queueing of callbacks in all CPUs must be atomic with > - * respect to RCU, otherwise one CPU may queue a callback, > - * wait for a grace period, decrement barrier count and call > - * complete(), while other CPUs have not yet queued anything. > - * So, we need to make sure that grace periods cannot complete > - * until all the callbacks are queued. > + * init and set rcu_barrier_cpu_count to 1, otherwise(set it to 0) > + * one CPU may queue a callback, wait for a grace period, decrement > + * barrier count and call complete(), while other CPUs have not yet > + * queued anything. > + * So, we need to make sure that rcu_barrier_cpu_count cannot become > + * 0 until all the callbacks are queued.
* Initialize rcu_barrier_cpu_count to 1, then invoke * rcu_barrier_func() on each CPU, so that each CPU also has * incremented rcu_barrier_cpu_count. Only then is it safe to * decrement rcu_barrier_cpu_count -- otherwise the first CPU * might complete its grace period before all of the other CPUs * did their increment, causing this function to return too * early.
> */ > - rcu_read_lock(); > + atomic_set(&rcu_barrier_cpu_count, 1); > + /* > + * rcu_barrier_cpu_count = 1 must be visible to cpus before > + * them call rcu_barrier_func(). > + */ > + smp_wmb();
smp_wmb(); /* atomic_set() must precede all rcu_barrier_func()s. */
> on_each_cpu(rcu_barrier_func, (void *)type, 1); > - rcu_read_unlock(); > + if (atomic_dec_and_test(&rcu_barrier_cpu_count)) > + complete(&rcu_barrier_completion); > wait_for_completion(&rcu_barrier_completion); > mutex_unlock(&rcu_barrier_mutex); > } > > >
| |