[PATCH] cpuidle: delay enabling interrupts until all coupled CPUs leave idle

[PATCH] cpuidle: delay enabling interrupts until all coupled CPUs leave idle

[Paul Burton]

As described by a comment at the end of cpuidle_enter_state_coupled it
can be inefficient for coupled idle states to return with IRQs enabled
since they may proceed to service an interrupt instead of clearing the
coupled idle state. Until they have finished & cleared the idle state
all CPUs coupled with them will spin rather than being able to enter a
safe idle state.

Commits e1689795a784 "cpuidle: Add common time keeping and irq
enabling" and 554c06ba3ee2 "cpuidle: remove en_core_tk_irqen flag" led
to the cpuidle_enter_state enabling interrupts for all idle states,
including coupled ones, making this inefficiency unavoidable by drivers
& the local_irq_enable near the end of cpuidle_enter_state_coupled
redundant. This patch avoids enabling interrupts in cpuidle_enter_state
after a coupled state has been entered, allowing them to remain disabled
until all coupled CPUs have exited the idle state and
cpuidle_enter_state_coupled re-enables them.

Signed-off-by: Paul Burton <paul.burton@imgtec.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-pm@vger.kernel.org
---
 drivers/cpuidle/cpuidle.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index a55e68f..366e684 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -85,7 +85,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 
 	time_end = ktime_get();
 
-	local_irq_enable();
+	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
+		local_irq_enable();
 
 	diff = ktime_to_us(ktime_sub(time_end, time_start));
 	if (diff > INT_MAX)
-- 
1.8.5.3

Re: [PATCH] cpuidle: delay enabling interrupts until all coupled CPUs leave idle

[Rafael J. Wysocki]

On Thursday, March 06, 2014 11:02:01 AM Paul Burton wrote:
> As described by a comment at the end of cpuidle_enter_state_coupled it
> can be inefficient for coupled idle states to return with IRQs enabled
> since they may proceed to service an interrupt instead of clearing the
> coupled idle state. Until they have finished & cleared the idle state
> all CPUs coupled with them will spin rather than being able to enter a
> safe idle state.
> 
> Commits e1689795a784 "cpuidle: Add common time keeping and irq
> enabling" and 554c06ba3ee2 "cpuidle: remove en_core_tk_irqen flag" led
> to the cpuidle_enter_state enabling interrupts for all idle states,
> including coupled ones, making this inefficiency unavoidable by drivers
> & the local_irq_enable near the end of cpuidle_enter_state_coupled
> redundant. This patch avoids enabling interrupts in cpuidle_enter_state
> after a coupled state has been entered, allowing them to remain disabled
> until all coupled CPUs have exited the idle state and
> cpuidle_enter_state_coupled re-enables them.

This appears to be a regression.

Are there any bug reports related to it?  Alternatively, can you reproduce
it and if so, then on what hardware?

> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: linux-pm@vger.kernel.org
> ---
>  drivers/cpuidle/cpuidle.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index a55e68f..366e684 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -85,7 +85,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
>  
>  	time_end = ktime_get();
>  
> -	local_irq_enable();
> +	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
> +		local_irq_enable();
>  
>  	diff = ktime_to_us(ktime_sub(time_end, time_start));
>  	if (diff > INT_MAX)
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] cpuidle: delay enabling interrupts until all coupled CPUs leave idle

[Paul Burton]

On Thu, Mar 06, 2014 at 01:55:06PM +0100, Rafael J. Wysocki wrote:
> On Thursday, March 06, 2014 11:02:01 AM Paul Burton wrote:
> > As described by a comment at the end of cpuidle_enter_state_coupled it
> > can be inefficient for coupled idle states to return with IRQs enabled
> > since they may proceed to service an interrupt instead of clearing the
> > coupled idle state. Until they have finished & cleared the idle state
> > all CPUs coupled with them will spin rather than being able to enter a
> > safe idle state.
> > 
> > Commits e1689795a784 "cpuidle: Add common time keeping and irq
> > enabling" and 554c06ba3ee2 "cpuidle: remove en_core_tk_irqen flag" led
> > to the cpuidle_enter_state enabling interrupts for all idle states,
> > including coupled ones, making this inefficiency unavoidable by drivers
> > & the local_irq_enable near the end of cpuidle_enter_state_coupled
> > redundant. This patch avoids enabling interrupts in cpuidle_enter_state
> > after a coupled state has been entered, allowing them to remain disabled
> > until all coupled CPUs have exited the idle state and
> > cpuidle_enter_state_coupled re-enables them.
> 
> This appears to be a regression.
> 
> Are there any bug reports related to it?  Alternatively, can you reproduce
> it and if so, then on what hardware?

I'm not aware of any bug reports related to this (though I haven't gone
looking). I can see the issue occur with (a further developed version
of) the MIPS CPS cpuidle driver I posted a while ago. I haven't measured
the impact on exit latency (which I suppose would be variable anyway
depending on the pending interrupt(s)) but have seen the issue with
interrupts processed whilst a coupled CPU spins. Given that there is
clearly an inconsistency between cpuidle_enter_state &
cpuidle_enter_state_coupled, and given that the latter documents the
issue & its intent with regards to IRQs pretty clearly, this would seem
to be the obvious fix.

Thanks,
    Paul

> 
> > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: linux-pm@vger.kernel.org
> > ---
> >  drivers/cpuidle/cpuidle.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > index a55e68f..366e684 100644
> > --- a/drivers/cpuidle/cpuidle.c
> > +++ b/drivers/cpuidle/cpuidle.c
> > @@ -85,7 +85,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> >  
> >  	time_end = ktime_get();
> >  
> > -	local_irq_enable();
> > +	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
> > +		local_irq_enable();
> >  
> >  	diff = ktime_to_us(ktime_sub(time_end, time_start));
> >  	if (diff > INT_MAX)
> > 
> 
> -- 
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cpuidle: delay enabling interrupts until all coupled CPUs leave idle

[Rafael J. Wysocki]

On Tuesday, March 11, 2014 09:41:36 AM Paul Burton wrote:
> On Thu, Mar 06, 2014 at 01:55:06PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, March 06, 2014 11:02:01 AM Paul Burton wrote:
> > > As described by a comment at the end of cpuidle_enter_state_coupled it
> > > can be inefficient for coupled idle states to return with IRQs enabled
> > > since they may proceed to service an interrupt instead of clearing the
> > > coupled idle state. Until they have finished & cleared the idle state
> > > all CPUs coupled with them will spin rather than being able to enter a
> > > safe idle state.
> > > 
> > > Commits e1689795a784 "cpuidle: Add common time keeping and irq
> > > enabling" and 554c06ba3ee2 "cpuidle: remove en_core_tk_irqen flag" led
> > > to the cpuidle_enter_state enabling interrupts for all idle states,
> > > including coupled ones, making this inefficiency unavoidable by drivers
> > > & the local_irq_enable near the end of cpuidle_enter_state_coupled
> > > redundant. This patch avoids enabling interrupts in cpuidle_enter_state
> > > after a coupled state has been entered, allowing them to remain disabled
> > > until all coupled CPUs have exited the idle state and
> > > cpuidle_enter_state_coupled re-enables them.
> > 
> > This appears to be a regression.
> > 
> > Are there any bug reports related to it?  Alternatively, can you reproduce
> > it and if so, then on what hardware?
> 
> I'm not aware of any bug reports related to this (though I haven't gone
> looking). I can see the issue occur with (a further developed version
> of) the MIPS CPS cpuidle driver I posted a while ago. I haven't measured
> the impact on exit latency (which I suppose would be variable anyway
> depending on the pending interrupt(s)) but have seen the issue with
> interrupts processed whilst a coupled CPU spins. Given that there is
> clearly an inconsistency between cpuidle_enter_state &
> cpuidle_enter_state_coupled, and given that the latter documents the
> issue & its intent with regards to IRQs pretty clearly, this would seem
> to be the obvious fix.

OK, thanks for the info.


> > > Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> > > Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > > Cc: linux-pm@vger.kernel.org
> > > ---
> > >  drivers/cpuidle/cpuidle.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > index a55e68f..366e684 100644
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -85,7 +85,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
> > >  
> > >  	time_end = ktime_get();
> > >  
> > > -	local_irq_enable();
> > > +	if (!cpuidle_state_is_coupled(dev, drv, entered_state))
> > > +		local_irq_enable();
> > >  
> > >  	diff = ktime_to_us(ktime_sub(time_end, time_start));
> > >  	if (diff > INT_MAX)
> > > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.