Re: [PATCH RFC v2 tip/core/rcu 03/22] rcutorture: Add flag to produce non-busy-wait task stalls

From: "Paul E. McKenney" <paulmck@kernel.org>
To: Hillf Danton <hdanton@sina.com>
Cc: rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel-team@fb.com, mingo@kernel.org, jiangshanlai@gmail.com,
	dipankar@in.ibm.com, akpm@linux-foundation.org,
	mathieu.desnoyers@efficios.com, josh@joshtriplett.org,
	tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org,
	dhowells@redhat.com, edumazet@google.com, fweisbec@gmail.com,
	oleg@redhat.com, joel@joelfernandes.org
Subject: Re: [PATCH RFC v2 tip/core/rcu 03/22] rcutorture: Add flag to produce non-busy-wait task stalls
Date: Thu, 19 Mar 2020 08:22:53 -0700
Message-ID: <20200319152253.GE3199@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20200319133947.12172-1-hdanton@sina.com>

On Thu, Mar 19, 2020 at 09:39:47PM +0800, Hillf Danton wrote:
> 
> On Thu, 19 Mar 2020 05:38:12 -0700 "Paul E. McKenney" wrote:
> > 
> > On Thu, Mar 19, 2020 at 06:46:14PM +0800, Hillf Danton wrote:
> > > 
> > > On Wed, 18 Mar 2020 17:10:41 -0700
> > > >  static int rcu_torture_stall(void *args)
> > > >  {
> > > > +	int idx;
> > > >  	unsigned long stop_at;
> > > >  
> > > >  	VERBOSE_TOROUT_STRING("rcu_torture_stall task started");
> > > > @@ -1610,21 +1612,22 @@ static int rcu_torture_stall(void *args)
> > > >  	if (!kthread_should_stop()) {
> > > >  		stop_at = ktime_get_seconds() + stall_cpu;
> > > >  		/* RCU CPU stall is expected behavior in following code. */
> > > > -		rcu_read_lock();
> > > > +		idx = cur_ops->readlock();
> > > >  		if (stall_cpu_irqsoff)
> > > >  			local_irq_disable();
> > > > -		else
> > > > +		else if (!stall_cpu_block)
> > > >  			preempt_disable();
> > > >  		pr_alert("rcu_torture_stall start on CPU %d.\n",
> > > > -			 smp_processor_id());
> > > > +			 raw_smp_processor_id());
> > > >  		while (ULONG_CMP_LT((unsigned long)ktime_get_seconds(),
> > > >  				    stop_at))
> > > > -			continue;  /* Induce RCU CPU stall warning. */
> > > > +			if (stall_cpu_block)
> > > > +				schedule_timeout_uninterruptible(HZ);
> > > 
> > > Why is the scheduled-in task so special that it will be running on
> > > the current CPU with irq disabled?
> > 
> > You lost me on this one.
> 
> Quite likely :)
> 
> > IRQs are not at all disabled.
> > 
> > > >  		if (stall_cpu_irqsoff)
> > > >  			local_irq_enable();
> 
> Local IRQs get enabled here depending on stall_cpu_irqsoff.
> 
> What I was asking is the scheduling case like
> 
> 	local_irq_disable();
> 	schedule_timeout(HZ);
> 	local_irq_enable();
> 
> Is it likely going to be ruled out in this patch?

If an rcutorture run specified both the rcutorture.stall_cpu_irqsoff and
the rcutorture.stall_cpu_block module parameters, then yes, exactly the
sequence you call out should occur.  Can't say that I have tried this,
though.  Nor would I expect to have ever done so without your suggesting
that I do.

But why not try it on current -rcu?

tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 12 --duration 3 --configs "TRACE01" --bootargs "rcutorture.stall_cpu=25 rcutorture.stall_cpu_holdoff=30 rcutorture.stall_cpu_block=1 rcupdate.rcu_task_stall_timeout=10000 rcutorture.stall_cpu_irqsoff"

This tells rcutorture to use all 12 hardware threads, to run the kernel for
three minutes, to run only the TRACE01 rcutorture scenario, and to test
RCU CPU stall warnings:

rcutorture.stall_cpu=25: Stall the CPU for 25 seconds.

rcutorture.stall_cpu_holdoff=30: Wait 30 seconds after boot to start stalling.

rcutorture.stall_cpu_block=1: Do the schedule_timeout_uninterruptible()
	while stalling.

rcupdate.rcu_task_stall_timeout=10000: Set the stall-warning timeout
	to 10,000 jiffies, or ten seconds.

rcutorture.stall_cpu_irqsoff: This tells rcutorture to execute the
	local_irq_disable() that you called out above.

And this results in a couple of stall warning messages, as expected
given that you get two ten-second intervals in a 25-second interval.

No other complaints, though.  And of course what happens is that
__schedule() enables interrupts on first call (as it must do) and they
remain enabled past that point.  Then local_irq_enable() redundantly
enables them.

> Or is it anything by design?

So no, not by design.  I don't see any reason to change it.  After all,
if you are running rcutorture and also asking rcutorture to make CPU
stall warnings, you have to expect a bit of noise from the kernel.
Testing that noise is after all the whole point.  ;-)

							Thanx, Paul

> > > > -		else
> > > > +		else if (!stall_cpu_block)
> > > >  			preempt_enable();
> > > > -		rcu_read_unlock();
> > > > +		cur_ops->readunlock(idx);
> > > >  		pr_alert("rcu_torture_stall end.\n");
> > > >  	}
> > > >  	torture_shutdown_absorb("rcu_torture_stall");
> > > > -- 
> > > > 2.9.5
> 