Re: [RFC][PATCH v3 6/6] rcu/tree: Use irq_work_queue_remote()

[Paul E. McKenney]

On Thu, Oct 29, 2020 at 10:10:53AM +0100, Peter Zijlstra wrote:
> On Wed, Oct 28, 2020 at 01:15:54PM -0700, Paul E. McKenney wrote:
> > On Wed, Oct 28, 2020 at 09:02:43PM +0100, Peter Zijlstra wrote:
> 
> > > Subject: rcu/tree: Use irq_work_queue_remote()
> > > From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > Date: Wed Oct 28 11:53:40 CET 2020
> > > 
> > > All sites that consume rcu_iw_gp_seq seem to have rcu_node lock held,
> > > so setting it probably should too. Also the effect of self-IPI here
> > > would be setting rcu_iw_gp_seq to the value we just set it to
> > > (pointless) and clearing rcu_iw_pending, which we just set, so don't
> > > set it.
> > > 
> > > Passes TREE01.
> > > 
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > > ---
> > >  kernel/rcu/tree.c |   10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -1308,14 +1308,16 @@ static int rcu_implicit_dynticks_qs(stru
> > >  			resched_cpu(rdp->cpu);
> > >  			WRITE_ONCE(rdp->last_fqs_resched, jiffies);
> > >  		}
> > > -#ifdef CONFIG_IRQ_WORK
> > > +		raw_spin_lock_rcu_node(rnp);
> > 
> > The caller of rcu_implicit_dynticks_qs() already holds this lock.
> > Please see the force_qs_rnp() function and its second call site,
> > to which rcu_implicit_dynticks_qs() is passed as an argument.
> > 
> > But other than that, this does look plausible.  And getting rid of
> > that #ifdef is worth something.  ;-)
> 
> Dang, clearly TREE01 didn't actually hit any of this code :/ Is there
> another test I should be running?

TREE01 is fine, but you have to tell rcutorture to actually generate an
RCU CPU stall warning.  Like this for a 25-second stall with interrupts
disabled:

tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 3 --configs "10*TREE04" --bootargs "rcutorture.stall_cpu=25 rcutorture.stall_cpu_irqsoff=1" --trust-make

And like this to stall with interrupts enabled:

tools/testing/selftests/rcutorture/bin/kvm.sh --allcpus --duration 3 --configs "10*TREE04" --bootargs "rcutorture.stall_cpu=25" --trust-make

I use TREE04 instead of TREE01 because I have recently seen the pattern
you are looking for in a similar configuration.  But any of the TREE
scenarios should work.

And you need a bit of luck.  The CPU that rcutorture forces to stall
needs to be blocking the current grace period at the beginning of that
stall, or, alternatively, another grace period needs to start while
the CPU is stalled.  But the first command (with stall_cpu_irqsoff=1)
will sometimes get you something like this:

rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
rcu:     3-...0: (1 GPs behind) idle=156/1/0x4000000000000000 softirq=493/494 fqs=4570 last_accelerate: 0000/eaad dyntick_enabled: 0
 (detected by 7, t=21003 jiffies, g=693, q=35687)

The zero in "...0" on the second line indicates that the RCU CPU stall
warning found that the stalled CPU had its interrupts disabled for the
last portion of that stall.

This is probabilistic.  Much depends on the state of both RCU and
rcutorture at the time that the stall starts.  I got the above results
once in 20 attempts, with other attempts having variously prevented
RCU's grace-period kthread from running, gotten the irq-work executed
just before the stall was reported, and so on.

Of course, to test your change, you also need the grace-period kthread to
migrate to the stalled CPU just after interrupts are enabled.  For this,
you need something like an 11-second stall plus something to move the
grace-period kthread at the right (wrong?) time.  Or just run the above
commands in a loop on a system with ample storage over night or some such.
I see about 70MB of storage per run, so disk size shouldn't be too much
of a problem.

If you do wish to muck with RCU and rcutorture to migrate
RCU's grace-period kthread, look for uses of stall_cpu_irqsoff in
kernel/rcu/rcutorture.c on the one hand and rcu_gp_kthread() along with
the functions that it calls in kernel/rcu/tree.c on the other.
For the latter, maybe force_qs_rnp() would be a good choice.

						Thanx, Paul