[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <b5c91db4-c1b6-3849-d5a9-89ca07d2d020@windriver.com>
Date: Mon, 19 Apr 2021 12:58:58 +0800
From: "Xu, Yanfei" <yanfei.xu@...driver.com>
To: paulmck@...nel.org
Cc: rcu@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [Qestion] Is preempt_disable/enable needed in non-preemption code
path
On 4/17/21 1:26 AM, Paul E. McKenney wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On Fri, Apr 16, 2021 at 06:51:10PM +0800, Xu, Yanfei wrote:
>>
>>
>> On 4/16/21 1:07 AM, Paul E. McKenney wrote:
>>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>>
>>> On Fri, Apr 16, 2021 at 12:18:42AM +0800, Xu, Yanfei wrote:
>>>>
>>>>
>>>> On 4/15/21 11:43 PM, Paul E. McKenney wrote:
>>>>> [Please note: This e-mail is from an EXTERNAL e-mail address]
>>>>>
>>>>> On Thu, Apr 15, 2021 at 11:04:05PM +0800, Xu, Yanfei wrote:
>>>>>> Hi experts,
>>>>>>
>>>>>> I am learning rcu mechanism and its codes. When looking at the
>>>>>> rcu_blocking_is_gp(), I found there is a pair preemption disable/enable
>>>>>> operation in non-preemption code path. And it has been a long time. I can't
>>>>>> understand why we need it? Is there some thing I missed? If not, can we
>>>>>> remove the unnecessary operation like blow?
>>>>>
>>>>> Good point, you are right that preemption is disabled anyway in that block
>>>>> of code. However, preempt_disable() and preempt_enable() also prevent the
>>>>> compiler from moving that READ_ONCE() around. So my question to you is
>>>>> whether it is safe to remove those statements entirely or whether they
>>>>> should instead be replaced by barrier() or similar.
>>>>
>>>> Thanks for your reply! :)
>>>>
>>>> Yes, preempt_disable() and preempt_enable() defined in !preemption are
>>>> barrier(). barrier can prevent from reordering that READ_ONCE(), but base on
>>>> my current understanding, volatile in READ_ONCE can also tell the compiler
>>>> not to reorder it. So, I think it's safe?
>>>
>>> Maybe.
>>>
>>> Please keep in mind that although the compiler is prohibited from
>>> reordering volatile accesses with each other, there is nothing stopping
>>> it from reordering volatile accesses with non-volatile accesses.
>>
>> Thanks for your patient explanation!
>>
>> I am trying to absorb what you said. Blow are my understanding:
>> 1. "the compiler is prohibited from reordering volatile accesses with each
>> other" means these situations:
>> int a;
>> foo()
>> {
>> for(;;)
>> READ_ONCE(a);
>> }
>>
>> or
>>
>> int a,b;
>> foo()
>> {
>> int c,d;
>> c = READ_ONCE(a);
>> d = READ_ONCE(b);
>> }
>
> Yes, in both cases the load instructions emitted for the READ_ONCE()
> macros must be emitted in order. The underlying hardware is free
> to reorder.
Got it.
>
>> 2. "volatile accesses with non-volatile accesses" means d=b may happen
>> before c=READ_ONCE(a) :
>> int a;
>> foo()
>> {
>> int b = 2
>> int c,d;
>> c = READ_ONCE(a);
>> d = b;
>> }
>> if we want to keep the ordering of volatile access "c=READ_ONCE(a)" and
>> non-volatile access "d=b", we should use stronger barrier like barrier().
>
> Or an additional READ_ONCE() for b or a WRITE_ONCE() for d. But again,
> this would constrain only the compiler, not the hardware.
>
> But this wouldn't matter in most cases, because both b and d are local
> variables whose addresses were never taken. So someone would need to
> be using something crazy to poke into others' stacks for this to matter.
Agree.
>
>> Hope I didn't misunderstand.
>
> It looks like you have most of it.
>
>> Back to rcu_blocking_is_gp(), I find this link today
>> https://www.spinics.net/lists/rcu/msg03985.html
>> With the content in this link, I still haven't got the meaning of these two
>> barrier(). I think I should learn knowledge about cpu-hotplug and things
>> which talked in the link first to make sure if I am missing something, and
>> then consult you. :)
>
> That sounds like a very good approach!
>
> Keep in mind that I am worried not just about the current state of
> the code and compilers, but also their possible future states.
I see.
Thanks again.
Best regards,
Yanfei
>
> Thanx, Paul
>
>> Best regards,
>> Yanfei
>>
>>>
>>> Thanx, Paul
>>>
>>>> Best regards,
>>>> Yanfei
>>>>
>>>>>
>>>>> Thanx, Paul
>>>>>
>>>>>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>>>>>> index da6f5213fb74..c6d95a00715e 100644
>>>>>> --- a/kernel/rcu/tree.c
>>>>>> +++ b/kernel/rcu/tree.c
>>>>>> @@ -3703,7 +3703,6 @@ static int rcu_blocking_is_gp(void)
>>>>>> if (IS_ENABLED(CONFIG_PREEMPTION))
>>>>>> return rcu_scheduler_active == RCU_SCHEDULER_INACTIVE;
>>>>>> might_sleep(); /* Check for RCU read-side critical section. */
>>>>>> - preempt_disable();
>>>>>> /*
>>>>>> * If the rcu_state.n_online_cpus counter is equal to one,
>>>>>> * there is only one CPU, and that CPU sees all prior accesses
>>>>>> @@ -3718,7 +3717,6 @@ static int rcu_blocking_is_gp(void)
>>>>>> * Those memory barriers are provided by CPU-hotplug code.
>>>>>> */
>>>>>> ret = READ_ONCE(rcu_state.n_online_cpus) <= 1;
>>>>>> - preempt_enable();
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Yanfei
Powered by blists - more mailing lists