Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 10:37:09 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Zqiang <qiang1.zhang@intel.com>
Cc:     frederic@kernel.org, quic_neeraju@quicinc.com,
        joel@joelfernandes.org, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] rcu-tasks: Make synchronize_rcu_tasks_generic()
 no-ops on early booting
Message-ID: <20220712173709.GX1790663@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220712082606.3662616-1-qiang1.zhang@intel.com>
 <20220712082606.3662616-3-qiang1.zhang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220712082606.3662616-3-qiang1.zhang@intel.com>
Precedence: bulk

On Tue, Jul 12, 2022 at 04:26:06PM +0800, Zqiang wrote:
> When the rcu_scheduler_active variable is RCU_SCHEDULER_INACTIVE and not
> yet converted to RCU_SCHEDULER_INIT, there is only idle task, any legal
> call synchronize_rcu_tasks_generic() is a quiescent state. this commit
> make synchronize_rcu_tasks_generic() no-ops when the rcu_scheduler_active
> variable is RCU_SCHEDULER_INACTIVE.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> ---

It looks like this would be a good way to provide early boot access
to synchronize_rcu_tasks(), synchronize_rcu_tasks_rude(), and
synchronize_rcu_tasks_trace().

But do we really need early boot access to these functions?  As in has
the below WARN_ON() actually triggered?

And if it has triggered, and in a context that means that these functions
really are needed during early boot, how should the testing strategy
change to test these at the relevant portions of the boot sequence?

From what I know, hitting these during early boot would indicate that
something was removing a trace during early boot, and I know of no way
to make that happen.  Hence my skepticism.  ;-)

But *if* this was really needed, this looks to be a reasonable way to
implement it.

							Thanx, Paul

>  kernel/rcu/tasks.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
> index 469bf2a3b505..0237e765c28e 100644
> --- a/kernel/rcu/tasks.h
> +++ b/kernel/rcu/tasks.h
> @@ -560,8 +560,9 @@ static int __noreturn rcu_tasks_kthread(void *arg)
>  static void synchronize_rcu_tasks_generic(struct rcu_tasks *rtp)
>  {
>  	/* Complain if the scheduler has not started.  */
> -	WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> -			 "synchronize_rcu_tasks called too soon");
> +	if (WARN_ONCE(rcu_scheduler_active == RCU_SCHEDULER_INACTIVE,
> +			 "synchronize_rcu_tasks called too soon"))
> +		return;
>  
>  	// If the grace-period kthread is running, use it.
>  	if (READ_ONCE(rtp->kthread_ptr)) {
> -- 
> 2.25.1
>