Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Apr 2021 08:43:26 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Xu, Yanfei" <yanfei.xu@windriver.com>
Cc:     rcu@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [Qestion] Is preempt_disable/enable needed in non-preemption
 code path
Message-ID: <20210415154326.GF4510@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <b068d707-0ac7-4840-a297-112731972d8a@windriver.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <b068d707-0ac7-4840-a297-112731972d8a@windriver.com>
Precedence: bulk

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.

							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