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 01:01:17 +0800 |
| |
On 4/16/21 12:18 AM, 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? > > Best regards, > Yanfei
Hi Paul, I objdump the function rcu_blocking_is_gp():
after dropping the barrier(): ffffffff81107c50 <rcu_blocking_is_gp>: ffffffff81107c50: e8 7b 2a f5 ff callq ffffffff8105a6d0 <__fentry__> ffffffff81107c55: 8b 05 41 fe 7c 01 mov 0x17cfe41(%rip),%eax # ffffffff828d7a9c <rcu_state+0x221c> ffffffff81107c5b: 55 push %rbp ffffffff81107c5c: 48 89 e5 mov %rsp,%rbp ffffffff81107c5f: 5d pop %rbp ffffffff81107c60: 83 f8 01 cmp $0x1,%eax ffffffff81107c63: 0f 9e c0 setle %al ffffffff81107c66: 0f b6 c0 movzbl %al,%eax ffffffff81107c69: c3 retq ffffffff81107c6a: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
the original codes: ffffffff81107ba0 <rcu_blocking_is_gp>: ffffffff81107ba0: e8 2b 2b f5 ff callq ffffffff8105a6d0 <__fentry__> ffffffff81107ba5: 55 push %rbp ffffffff81107ba6: 48 89 e5 mov %rsp,%rbp ffffffff81107ba9: 8b 05 ed fe 7c 01 mov 0x17cfeed(%rip),%eax # ffffffff828d7a9c <rcu_state+0x221c> ffffffff81107baf: 83 f8 01 cmp $0x1,%eax ffffffff81107bb2: 5d pop %rbp ffffffff81107bb3: 0f 9e c0 setle %al ffffffff81107bb6: 0f b6 c0 movzbl %al,%eax ffffffff81107bb9: c3 retq ffffffff81107bba: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1)
umm... It did been reordered by compiler after dropping the barrier(), however, I think the result will not be effected. Right?
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
| |