Re: [PATCH] rcupdate: fix 2 bugs of rcu_barrier*()

From: Lai Jiangshan
Date: Fri Oct 17 2008 - 01:50:18 EST


Paul E. McKenney wrote:
> 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?

by inspection. I was planning to put synchronize_rcu* back to
kernel/rcupdate.c and I found the code and the comments are
inconsistent suddenly when I was reviewing kernel/rcupdate.c.

>
>> 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. ;-)

I will remove this wmb.
Thank you a lot

Lai.

>
> 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@xxxxxxxxxxxxxxxxxx>
>
>> Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
>> ---
>> 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);
>> }
>>
>>
>>
>
>
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/