Messages in this thread | | | Subject | Re: [Qestion] Is preempt_disable/enable needed in non-preemption code path | From | "Xu, Yanfei" <> | Date | Mon, 19 Apr 2021 12:58:58 +0800 |
| |
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
| |