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:   Mon, 10 Aug 2020 13:31:09 -0400
From:   Joel Fernandes <joel@joelfernandes.org>
To:     "Paul E. McKenney" <paulmck@kernel.org>
Cc:     linux-kernel@vger.kernel.org, Davidlohr Bueso <dave@stgolabs.net>,
        Jonathan Corbet <corbet@lwn.net>,
        Josh Triplett <josh@joshtriplett.org>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        linux-doc@vger.kernel.org,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
        neeraju@codeaurora.org, peterz@infradead.org,
        Randy Dunlap <rdunlap@infradead.org>, rcu@vger.kernel.org,
        Steven Rostedt <rostedt@goodmis.org>, tglx@linutronix.de,
        vineethrp@gmail.com
Subject: Re: [PATCH v4 4/5] rcutorture: Force synchronizing of RCU flavor
 from hotplug notifier
Message-ID: <20200810173109.GA2253395@google.com>
References: <20200807170722.2897328-1-joel@joelfernandes.org>
 <20200807170722.2897328-5-joel@joelfernandes.org>
 <20200810161945.GK4295@paulmck-ThinkPad-P72>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200810161945.GK4295@paulmck-ThinkPad-P72>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Paul,

On Mon, Aug 10, 2020 at 09:19:45AM -0700, Paul E. McKenney wrote:
> On Fri, Aug 07, 2020 at 01:07:21PM -0400, Joel Fernandes (Google) wrote:
> > RCU has had deadlocks in the past related to synchronizing in a hotplug
> > notifier. Typically, this has occurred because timer callbacks did not get
> > migrated before the CPU hotplug notifier requesting RCU's services is
> > called. If RCU's grace period processing has a timer callback queued in
> > the meanwhile, it may never get called causing RCU stalls.
> > 
> > These issues have been fixed by removing such dependencies from grace
> > period processing, however there are no testing scenarios for such
> > cases.
> > 
> > This commit therefore reuses rcutorture's existing hotplug notifier to
> > invoke the flavor-specific synchronize callback. If anything locks up,
> > we expect stall warnings and/or other splats.
> > 
> > Obviously, we need not test for rcu_barrier from a notifier, since those
> > are not allowed from notifiers. This fact is already detailed in the
> > documentation as well.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Given that rcutorture_booster_init() is invoked on the CPU in question
> only after it is up and running, and that (if I remember correctly)
> rcutorture_booster_cleanup() is invoked on the outgoing CPU before it
> has really started going away, would this code really have caught that
> timer/CPU-hotplug/RCU bug?

You are right, it would not have caught that particular one because the timer
callbacks would have been migrated by the time the rcutorture_booster_init()
is called.

I still thought it is a good idea anyway to test if the dynamic hotplug
notifiers don't have these issues.

Did you have a better idea on how to test the timer/hotplug/rcu bug?

thanks,

 - Joel


> >  kernel/rcu/rcutorture.c | 81 +++++++++++++++++++++--------------------
> >  1 file changed, 42 insertions(+), 39 deletions(-)
> > 
> > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > index 92cb79620939..083b65e4877d 100644
> > --- a/kernel/rcu/rcutorture.c
> > +++ b/kernel/rcu/rcutorture.c
> > @@ -1645,12 +1645,37 @@ rcu_torture_print_module_parms(struct rcu_torture_ops *cur_ops, const char *tag)
> >  		 read_exit_delay, read_exit_burst);
> >  }
> >  
> > -static int rcutorture_booster_cleanup(unsigned int cpu)
> > +static bool rcu_torture_can_boost(void)
> > +{
> > +	static int boost_warn_once;
> > +	int prio;
> > +
> > +	if (!(test_boost == 1 && cur_ops->can_boost) && test_boost != 2)
> > +		return false;
> > +
> > +	prio = rcu_get_gp_kthreads_prio();
> > +	if (!prio)
> > +		return false;
> > +
> > +	if (prio < 2) {
> > +		if (boost_warn_once  == 1)
> > +			return false;
> > +
> > +		pr_alert("%s: WARN: RCU kthread priority too low to test boosting.  Skipping RCU boost test. Try passing rcutree.kthread_prio > 1 on the kernel command line.\n", KBUILD_MODNAME);
> > +		boost_warn_once = 1;
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +static int rcutorture_hp_cleanup(unsigned int cpu)
> >  {
> >  	struct task_struct *t;
> >  
> > -	if (boost_tasks[cpu] == NULL)
> > +	if (!rcu_torture_can_boost() || boost_tasks[cpu] == NULL)
> >  		return 0;
> > +
> >  	mutex_lock(&boost_mutex);
> >  	t = boost_tasks[cpu];
> >  	boost_tasks[cpu] = NULL;
> > @@ -1662,11 +1687,14 @@ static int rcutorture_booster_cleanup(unsigned int cpu)
> >  	return 0;
> >  }
> >  
> > -static int rcutorture_booster_init(unsigned int cpu)
> > +static int rcutorture_hp_init(unsigned int cpu)
> >  {
> >  	int retval;
> >  
> > -	if (boost_tasks[cpu] != NULL)
> > +	/* Force synchronizing from hotplug notifier to ensure it is safe. */
> > +	cur_ops->sync();
> > +
> > +	if (!rcu_torture_can_boost() || boost_tasks[cpu] != NULL)
> >  		return 0;  /* Already created, nothing more to do. */
> >  
> >  	/* Don't allow time recalculation while creating a new task. */
> > @@ -2336,30 +2364,6 @@ static void rcu_torture_barrier_cleanup(void)
> >  	}
> >  }
> >  
> > -static bool rcu_torture_can_boost(void)
> > -{
> > -	static int boost_warn_once;
> > -	int prio;
> > -
> > -	if (!(test_boost == 1 && cur_ops->can_boost) && test_boost != 2)
> > -		return false;
> > -
> > -	prio = rcu_get_gp_kthreads_prio();
> > -	if (!prio)
> > -		return false;
> > -
> > -	if (prio < 2) {
> > -		if (boost_warn_once  == 1)
> > -			return false;
> > -
> > -		pr_alert("%s: WARN: RCU kthread priority too low to test boosting.  Skipping RCU boost test. Try passing rcutree.kthread_prio > 1 on the kernel command line.\n", KBUILD_MODNAME);
> > -		boost_warn_once = 1;
> > -		return false;
> > -	}
> > -
> > -	return true;
> > -}
> > -
> >  static bool read_exit_child_stop;
> >  static bool read_exit_child_stopped;
> >  static wait_queue_head_t read_exit_wq;
> > @@ -2503,8 +2507,7 @@ rcu_torture_cleanup(void)
> >  		 rcutorture_seq_diff(gp_seq, start_gp_seq));
> >  	torture_stop_kthread(rcu_torture_stats, stats_task);
> >  	torture_stop_kthread(rcu_torture_fqs, fqs_task);
> > -	if (rcu_torture_can_boost())
> > -		cpuhp_remove_state(rcutor_hp);
> > +	cpuhp_remove_state(rcutor_hp);
> >  
> >  	/*
> >  	 * Wait for all RCU callbacks to fire, then do torture-type-specific
> > @@ -2773,21 +2776,21 @@ rcu_torture_init(void)
> >  		if (firsterr)
> >  			goto unwind;
> >  	}
> > +
> > +	firsterr = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "RCU_TORTURE",
> > +			rcutorture_hp_init,
> > +			rcutorture_hp_cleanup);
> > +	if (firsterr < 0)
> > +		goto unwind;
> > +	rcutor_hp = firsterr;
> > +
> >  	if (test_boost_interval < 1)
> >  		test_boost_interval = 1;
> >  	if (test_boost_duration < 2)
> >  		test_boost_duration = 2;
> > -	if (rcu_torture_can_boost()) {
> > -
> > +	if (rcu_torture_can_boost())
> >  		boost_starttime = jiffies + test_boost_interval * HZ;
> >  
> > -		firsterr = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "RCU_TORTURE",
> > -					     rcutorture_booster_init,
> > -					     rcutorture_booster_cleanup);
> > -		if (firsterr < 0)
> > -			goto unwind;
> > -		rcutor_hp = firsterr;
> > -	}
> >  	shutdown_jiffies = jiffies + shutdown_secs * HZ;
> >  	firsterr = torture_shutdown_init(shutdown_secs, rcu_torture_cleanup);
> >  	if (firsterr)
> > -- 
> > 2.28.0.236.gb10cc79966-goog
> >