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:   Fri, 13 Jan 2023 07:27:33 -0800
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Zhang, Qiang1" <qiang1.zhang@intel.com>
Cc:     "frederic@kernel.org" <frederic@kernel.org>,
        "quic_neeraju@quicinc.com" <quic_neeraju@quicinc.com>,
        "joel@joelfernandes.org" <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] rcu: Fix the start_poll_synchronize_rcu_expedited()
 be invoked very early
Message-ID: <20230113152733.GZ4028633@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20230112075629.1661429-1-qiang1.zhang@intel.com>
 <20230112190656.GR4028633@paulmck-ThinkPad-P17-Gen-1>
 <PH0PR11MB588074A87AC454A7F0E42A5ADAC29@PH0PR11MB5880.namprd11.prod.outlook.com>
 <PH0PR11MB588008A9BBAFA6BEEC8685ABDAC29@PH0PR11MB5880.namprd11.prod.outlook.com>
 <20230113144919.GY4028633@paulmck-ThinkPad-P17-Gen-1>
 <PH0PR11MB588004418C299781FE6738BFDAC29@PH0PR11MB5880.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <PH0PR11MB588004418C299781FE6738BFDAC29@PH0PR11MB5880.namprd11.prod.outlook.com>
Precedence: bulk

On Fri, Jan 13, 2023 at 03:02:54PM +0000, Zhang, Qiang1 wrote:
> On Fri, Jan 13, 2023 at 12:10:47PM +0000, Zhang, Qiang1 wrote:
> > 
> > > Currently, the start_poll_synchronize_rcu_expedited() can be invoked
> > > very early. before rcu_init(), the rcu_data structure's->mynode is not
> > > initialized, if invoke start_poll_synchronize_rcu_expedited() before
> > > rcu_init(), will trigger a null rcu_node structure's->exp_seq_poll access.
> > > 
> > > This commit add boot_exp_seq_poll_rq member to rcu_state structure to
> > > store seq number return by invoke start_poll_synchronize_rcu_expedited()
> > > very early.
> > > 
> > > Fixes: d96c52fe4907 ("rcu: Add polled expedited grace-period primitives")
> > > Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > >
> > >First off, excellent catch, Zqiang!!!
> > >
> > >And thank you for Frederic and Joel for your reviews.
> > >	
> > >But I believe that this can be simplified, for example, as shown in
> > >the (untested) patch below.
> > >
> > >Thoughts?
> > >
> > >Agree, thanks for wordsmithed  ????.
> > >
> > >
> > >And yes, I did presumptuously add Frederic's and Joel's reviews.
> > >Please let me know if you disagree, and if so what different approach
> > >you would prefer.  (Though of course simple disagreement is sufficient
> > >for me to remove your tag.  Not holding you hostage for improvements,
> > >not yet, anyway!)
> > >
> > >							Thanx, Paul
> > >
> > >------------------------------------------------------------------------
> > >
> > >commit e05af5cb3858e669c9e6b70e0aca708cc70457da
> > >Author: Zqiang <qiang1.zhang@intel.com>
> > >Date:   Thu Jan 12 10:48:29 2023 -0800
> > >
> > >    rcu: Permit start_poll_synchronize_rcu_expedited() to be invoked early
> > >    
> > >    According to the commit log of the patch that added it to the kernel,
> > >    start_poll_synchronize_rcu_expedited() can be invoked very early, as
> > >    in long before rcu_init() has been invoked.  But before rcu_init(),
> > >    the rcu_data structure's ->mynode field has not yet been initialized.
> > >    This means that the start_poll_synchronize_rcu_expedited() function's
> > >    attempt to set the CPU's leaf rcu_node structure's ->exp_seq_poll_rq
> > >    field will result in a segmentation fault.
> > >    
> > >    This commit therefore causes start_poll_synchronize_rcu_expedited() to
> > >    set ->exp_seq_poll_rq only after rcu_init() has initialized all CPUs'
> > >    rcu_data structures' ->mynode fields.  It also removes the check from
> > >    the rcu_init() function so that start_poll_synchronize_rcu_expedited(
> > >    is unconditionally invoked.  Yes, this might result in an unnecessary
> > >    boot-time grace period, but this is down in the noise.  Besides, there
> > >    only has to be one call_rcu() invoked prior to scheduler initialization
> > >    to make this boot-time grace period necessary.
> > 
> > A little confused, why  call_rcu() invoked prior to scheduler initialization to make this boot-time
> > grace period necessary ?????
> >
> >Because then there will be a callback queued that will require a grace
> >period to run anyway.
> >
> >Or maybe you are asking if those callbacks will really be able to use
> >that first grace period?
> 
> Yes, because even if we queue work to rcu_gp_wq workqueue, this also requires us to wait until
> the workqueue_init() is invoked, our work can be execute. and also need to wait for the
> rcu_gp kthread to be created, after this, our first grace period can begin, the callbacks has the
> opportunity to be called.  the call_rcu() require a grace period, but we require is expedited grace period.

Good catch, thank you!  I will update the commit log accordingly.

							Thanx, Paul

> Thanks
> Zqiang
> 
> >
> >							Thanx, Paul
> 
> > Thanks
> > Zqiang
> > 
> > 
> > >    
> > >    Signed-off-by: Zqiang <qiang1.zhang@intel.com>
> > >    Reviewed-by: Frederic Weisbecker <frederic@kernel.org>
> > >    Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > >    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > >
> > >diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > >index 63545d79da51c..f2e3a23778c06 100644
> > >--- a/kernel/rcu/tree.c
> > >+++ b/kernel/rcu/tree.c
> > >@@ -4937,9 +4937,8 @@ void __init rcu_init(void)
> > > 	else
> > > 		qovld_calc = qovld;
> > > 
> > >-	// Kick-start any polled grace periods that started early.
> > >-	if (!(per_cpu_ptr(&rcu_data, cpu)->mynode->exp_seq_poll_rq & 0x1))
> > >-		(void)start_poll_synchronize_rcu_expedited();
> > >+	// Kick-start in case any polled grace periods started early.
> > >+	(void)start_poll_synchronize_rcu_expedited();
> > > 
> > > 	rcu_test_sync_prims();
> > > }
> > >diff --git a/kernel/rcu/tree_exp.h b/kernel/rcu/tree_exp.h
> > >index 956cd459ba7f3..3b7abb58157df 100644
> > >--- a/kernel/rcu/tree_exp.h
> > >+++ b/kernel/rcu/tree_exp.h
> > >@@ -1068,9 +1068,10 @@ unsigned long start_poll_synchronize_rcu_expedited(void)
> > > 	if (rcu_init_invoked())
> > > 		raw_spin_lock_irqsave(&rnp->exp_poll_lock, flags);
> > > 	if (!poll_state_synchronize_rcu(s)) {
> > >-		rnp->exp_seq_poll_rq = s;
> > >-		if (rcu_init_invoked())
> > >+		if (rcu_init_invoked()) {
> > >+			rnp->exp_seq_poll_rq = s;
> > > 			queue_work(rcu_gp_wq, &rnp->exp_poll_wq);
> > >+		}
> > > 	}
> > > 	if (rcu_init_invoked())
> > > 		raw_spin_unlock_irqrestore(&rnp->exp_poll_lock, flags);