Re: [PATCH 1/1] rcutree/rcu_bh_qs: disable irq while calling rcu_preempt_qs()

[Paul Gortmaker <paul.gortmaker@windriver.com>]

On 13-12-05 08:57 PM, "“tiejun.chen”" wrote:
> On 12/05/2013 11:26 PM, Paul Gortmaker wrote:
>> On 13-12-05 04:52 AM, Tiejun Chen wrote:
>>> Any callers should make sure irq is disabled before calling rcu_preempt_qs().
>>
>> Can we stick to the standard rule of three here for commit logs please?
>>
>> 1) Describe what the end user symptoms look like (crash, bug, splat)
>>
>> 2) Describe the underlying problem, i.e. why it happens.
>>
>> 3) Describe why the fix proposed is the _right_ fix, in cases
>> where it isn't obvious what the impact of the change will be etc.
>>
>> Maybe it seems obvious to you what the 1,2,3 are -- but it won't
>> be obvious to everyone, and I hate having to guess.
> 
> This is required according to that comments from rcu_preempt_qs():
> 
> rcutree_plugin.h:
> 
> /*
>   * Record a preemptible-RCU quiescent state for the specified CPU.  Note
>   * that this just means that the task currently running on the CPU is
>   * not in a quiescent state.  There might be any number of tasks blocked
>   * while in an RCU read-side critical section.
>   *
>   * Unlike the other rcu_*_qs() functions, callers to this function
>   * must disable irqs in order to protect the assignment to
>   * ->rcu_read_unlock_special.
>   */
> static void rcu_preempt_qs(int cpu)
> ...
> 
> But in RT case, rcu_bh_qs() as the wrapper of rcu_preempt_qs() is called in some 
> scenarios where irq is enabled, like this path,
> 
> do_single_softirq()
> 	|
> 	+ local_irq_enable();
> 	+ handle_softirq()
> 	|	|
> 	|	+ rcu_bh_qs()
> 	|		|
> 	|		+ rcu_preempt_qs()
> 	|
> 	+ local_irq_disable()

OK, that is a good start, but it largely only covers #2 in my list.
We still don't know what the symptoms are (#1), or whether the added
irqsave will be problematic for other rcu_bh_qs callers (i.e. #3).

It would be really nice to have that additional information.

Thanks,
Paul.
--

> 
> So I think we'd better disable irq directly inside of rcu_bh_qs() to fix this 
> problem, and especially this is also kind for the potential rcu_bh_qs() usage 
> elsewhere in the future.
> 
> If you guy like this explanation, I'm happy for posting this as a long log in 
> this patch head description.
> 
> Thanks,
> 
> Tiejun
> 
>>
>> Thanks,
>> Paul.
>> --
>>
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>>> ---
>>>   kernel/rcutree.c |    5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/kernel/rcutree.c b/kernel/rcutree.c
>>> index 7ec834d..6f6d133 100644
>>> --- a/kernel/rcutree.c
>>> +++ b/kernel/rcutree.c
>>> @@ -186,7 +186,12 @@ static void rcu_preempt_qs(int cpu);
>>>
>>>   void rcu_bh_qs(int cpu)
>>>   {
>>> +	unsigned long flags;
>>> +
>>> +	/* Callers to this function, rcu_preempt_qs(), must disable irqs. */
>>> +	local_irq_save(flags);
>>>   	rcu_preempt_qs(cpu);
>>> +	local_irq_restore(flags);
>>>   }
>>>   #else
>>>   void rcu_bh_qs(int cpu)
>>>
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html