Messages in this thread Patch in this message | | | Subject | Re: [Qestion] Is preempt_disable/enable needed in non-preemption code path | From | "Xu, Yanfei" <> | Date | Fri, 16 Apr 2021 18:51:10 +0800 |
| |
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); }
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().
Hope I didn't misunderstand.
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. :)
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
| |