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

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

[Tiejun Chen]

Any callers should make sure irq is disabled before calling rcu_preempt_qs().

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)
-- 
1.7.9.5

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

[Paul Gortmaker]

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.

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

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

["“tiejun.chen”"]

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()

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

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

[Paul Gortmaker]

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

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

[Sebastian Andrzej Siewior]

* Paul Gortmaker | 2013-12-06 12:13:04 [-0500]:

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

Tiejun please send a new patch once you have #1 and #3.

>Thanks,
>Paul.

Sebastian