Re: [Qestion] Is preempt_disable/enable needed in non-preemption code path

[Xu, Yanfei]

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