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

[Xu, Yanfei]

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

>                                                          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