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@xxxxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
