Folllowing up on LSF/MM RCU/idle discussion

Folllowing up on LSF/MM RCU/idle discussion

[Paul E. McKenney]

Hello, Jiri!

It was good chatting with you last week, and I hope that travels went
well!

Just wanted to follow up on the non-noinstr code between the call
to rcu_idle_enter() and rcu_idle_exit().  Although the most correct
approach is to never have non-noinstr code in arch_cpu_idle(), for all I
know there might well be architectures for which this is not feasible.
If so, one workaround would be to supply a flag set by each arch (or
subarch) that says that rcu_idle_enter() and rcu_idle_exit() are invoked
within arch_cpu_idle().

CCing Peter, who just might have an opinion on this.  ;-)

							Thanx, Paul

Re: Folllowing up on LSF/MM RCU/idle discussion

[Peter Zijlstra]

On Mon, May 09, 2022 at 08:56:33AM -0700, Paul E. McKenney wrote:
> Hello, Jiri!
> 
> It was good chatting with you last week, and I hope that travels went
> well!
> 
> Just wanted to follow up on the non-noinstr code between the call
> to rcu_idle_enter() and rcu_idle_exit().  Although the most correct
> approach is to never have non-noinstr code in arch_cpu_idle(), for all I
> know there might well be architectures for which this is not feasible.
> If so, one workaround would be to supply a flag set by each arch (or
> subarch) that says that rcu_idle_enter() and rcu_idle_exit() are invoked
> within arch_cpu_idle().
> 
> CCing Peter, who just might have an opinion on this.  ;-)

Definitely have an opinion; just lack the tools to enforce these rules.
I cleaned up the worst of it for x86 but it's a shit-show for most
others. ARM in particular has some 'issues'.

But yeah, noinstr only when you do rcu_idle_enter.

The problem with validating all this is that cpuidle is a rats nest of
indirect calls; in order to validate the noinstr'ness of something like
that we need compiler support for pointer address spaces such that we
can stick pointers to noinstr functions in a different address space and
get complaints etc..

Re: Folllowing up on LSF/MM RCU/idle discussion

[Mark Rutland]

On Tue, May 10, 2022 at 08:54:57AM +0200, Peter Zijlstra wrote:
> On Mon, May 09, 2022 at 08:56:33AM -0700, Paul E. McKenney wrote:
> > Hello, Jiri!
> > 
> > It was good chatting with you last week, and I hope that travels went
> > well!
> > 
> > Just wanted to follow up on the non-noinstr code between the call
> > to rcu_idle_enter() and rcu_idle_exit().  Although the most correct
> > approach is to never have non-noinstr code in arch_cpu_idle(), for all I
> > know there might well be architectures for which this is not feasible.
> > If so, one workaround would be to supply a flag set by each arch (or
> > subarch) that says that rcu_idle_enter() and rcu_idle_exit() are invoked
> > within arch_cpu_idle().
> > 
> > CCing Peter, who just might have an opinion on this.  ;-)
> 
> Definitely have an opinion; just lack the tools to enforce these rules.
> I cleaned up the worst of it for x86 but it's a shit-show for most
> others. ARM in particular has some 'issues'.

Probably worth pointing out that arch_cpu_idle() is the simple case (and I
fixed that for arm64 to be correct for RCU and noinstr). I think the same
applies for most architectures.

The real beast is the cpuidle framework, which is what I think you're referring
to below, and IIRC that does the rcu_idle_enter() ... rcu_idle_exit() itself?
Maybe that was just for suspend.

I have no strong feeling about where we call rcu_idle_{enter,exit}() for
arch_cpu_idle() specifically, but I think that case is generally simple enough
that it doesn't really matter?

For the cpuidle framework, punting this into the driver such that it can be
done at the last possible moment around entry/exit to a HW idle state does feel
like it's going to be more robust, even if that means altering all those
drivers.

Thanks,
Mark.

> But yeah, noinstr only when you do rcu_idle_enter.
> 
> The problem with validating all this is that cpuidle is a rats nest of
> indirect calls; in order to validate the noinstr'ness of something like
> that we need compiler support for pointer address spaces such that we
> can stick pointers to noinstr functions in a different address space and
> get complaints etc..

Re: Folllowing up on LSF/MM RCU/idle discussion

[Paul E. McKenney]

On Tue, May 10, 2022 at 10:43:51AM +0100, Mark Rutland wrote:
> On Tue, May 10, 2022 at 08:54:57AM +0200, Peter Zijlstra wrote:
> > On Mon, May 09, 2022 at 08:56:33AM -0700, Paul E. McKenney wrote:
> > > Hello, Jiri!
> > > 
> > > It was good chatting with you last week, and I hope that travels went
> > > well!
> > > 
> > > Just wanted to follow up on the non-noinstr code between the call
> > > to rcu_idle_enter() and rcu_idle_exit().  Although the most correct
> > > approach is to never have non-noinstr code in arch_cpu_idle(), for all I
> > > know there might well be architectures for which this is not feasible.
> > > If so, one workaround would be to supply a flag set by each arch (or
> > > subarch) that says that rcu_idle_enter() and rcu_idle_exit() are invoked
> > > within arch_cpu_idle().
> > > 
> > > CCing Peter, who just might have an opinion on this.  ;-)
> > 
> > Definitely have an opinion; just lack the tools to enforce these rules.
> > I cleaned up the worst of it for x86 but it's a shit-show for most
> > others. ARM in particular has some 'issues'.
> 
> Probably worth pointing out that arch_cpu_idle() is the simple case (and I
> fixed that for arm64 to be correct for RCU and noinstr). I think the same
> applies for most architectures.
> 
> The real beast is the cpuidle framework, which is what I think you're referring
> to below, and IIRC that does the rcu_idle_enter() ... rcu_idle_exit() itself?
> Maybe that was just for suspend.
> 
> I have no strong feeling about where we call rcu_idle_{enter,exit}() for
> arch_cpu_idle() specifically, but I think that case is generally simple enough
> that it doesn't really matter?
> 
> For the cpuidle framework, punting this into the driver such that it can be
> done at the last possible moment around entry/exit to a HW idle state does feel
> like it's going to be more robust, even if that means altering all those
> drivers.

There does seem to be some ability to select where the rcu_idle_enter()
and rcu_idle_exit() are executed using CPUIDLE_FLAG_RCU_IDLE.
So depending on exactly which configuration Jiri is running, maybe there
is already a straightforward fix.

However, default_idle_call() invokes rcu_idle_enter() and rcu_idle_exit()
unconditionally, perhaps because it is not considered to be part of
cpuidle.

							Thanx, Paul

> Thanks,
> Mark.
> 
> > But yeah, noinstr only when you do rcu_idle_enter.
> > 
> > The problem with validating all this is that cpuidle is a rats nest of
> > indirect calls; in order to validate the noinstr'ness of something like
> > that we need compiler support for pointer address spaces such that we
> > can stick pointers to noinstr functions in a different address space and
> > get complaints etc..

Re: Folllowing up on LSF/MM RCU/idle discussion

[Peter Zijlstra]

On Tue, May 10, 2022 at 10:43:51AM +0100, Mark Rutland wrote:
> On Tue, May 10, 2022 at 08:54:57AM +0200, Peter Zijlstra wrote:
> > On Mon, May 09, 2022 at 08:56:33AM -0700, Paul E. McKenney wrote:
> > > Hello, Jiri!
> > > 
> > > It was good chatting with you last week, and I hope that travels went
> > > well!
> > > 
> > > Just wanted to follow up on the non-noinstr code between the call
> > > to rcu_idle_enter() and rcu_idle_exit().  Although the most correct
> > > approach is to never have non-noinstr code in arch_cpu_idle(), for all I
> > > know there might well be architectures for which this is not feasible.
> > > If so, one workaround would be to supply a flag set by each arch (or
> > > subarch) that says that rcu_idle_enter() and rcu_idle_exit() are invoked
> > > within arch_cpu_idle().
> > > 
> > > CCing Peter, who just might have an opinion on this.  ;-)
> > 
> > Definitely have an opinion; just lack the tools to enforce these rules.
> > I cleaned up the worst of it for x86 but it's a shit-show for most
> > others. ARM in particular has some 'issues'.
> 
> Probably worth pointing out that arch_cpu_idle() is the simple case (and I
> fixed that for arm64 to be correct for RCU and noinstr). I think the same
> applies for most architectures.
> 
> The real beast is the cpuidle framework, which is what I think you're referring
> to below, and IIRC that does the rcu_idle_enter() ... rcu_idle_exit() itself?
> Maybe that was just for suspend.

The whole group idle nonsense on arm32 was the worse I think.