| Message ID | alpine.LFD.2.11.1401291526320.1652@knanqh.ubzr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
Hi Nicolas, On 01/30/2014 02:01 AM, Nicolas Pitre wrote: > On Wed, 29 Jan 2014, Nicolas Pitre wrote: > >> In order to integrate cpuidle with the scheduler, we must have a better >> proximity in the core code with what cpuidle is doing and not delegate >> such interaction to arch code. >> >> Architectures implementing arch_cpu_idle() should simply enter >> a cheap idle mode in the absence of a proper cpuidle driver. >> >> Signed-off-by: Nicolas Pitre <nico@linaro.org> >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > As mentioned in my reply to Olof's comment on patch #5/6, here's a new > version of this patch adding the safety local_irq_enable() to the core > code. > > ----- >8 > > From: Nicolas Pitre <nicolas.pitre@linaro.org> > Subject: idle: move the cpuidle entry point to the generic idle loop > > In order to integrate cpuidle with the scheduler, we must have a better > proximity in the core code with what cpuidle is doing and not delegate > such interaction to arch code. > > Architectures implementing arch_cpu_idle() should simply enter > a cheap idle mode in the absence of a proper cpuidle driver. > > In both cases i.e. whether it is a cpuidle driver or the default > arch_cpu_idle(), the calling convention expects IRQs to be disabled > on entry and enabled on exit. There is a warning in place already but > let's add a forced IRQ enable here as well. This will allow for > removing the forced IRQ enable some implementations do locally and Why would this patch allow for removing the forced IRQ enable that are being done on some archs in arch_cpu_idle()? Isn't this patch expecting the default arch_cpu_idle() to have re-enabled the interrupts after exiting from the default idle state? Its supposed to only catch faulty cpuidle drivers that haven't enabled IRQs on exit from idle state but are expected to have done so, isn't it? Thanks Regards Preeti U Murthy > allowing for the warning to trig. > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c > index 988573a9a3..14ca43430a 100644 > --- a/kernel/cpu/idle.c > +++ b/kernel/cpu/idle.c > @@ -3,6 +3,7 @@ > */ > #include <linux/sched.h> > #include <linux/cpu.h> > +#include <linux/cpuidle.h> > #include <linux/tick.h> > #include <linux/mm.h> > #include <linux/stackprotector.h> > @@ -95,8 +96,10 @@ static void cpu_idle_loop(void) > if (!current_clr_polling_and_test()) { > stop_critical_timings(); > rcu_idle_enter(); > - arch_cpu_idle(); > - WARN_ON_ONCE(irqs_disabled()); > + if (cpuidle_idle_call()) > + arch_cpu_idle(); > + if (WARN_ON_ONCE(irqs_disabled())) > + local_irq_enable(); > rcu_idle_exit(); > start_critical_timings(); > } else { > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 30 Jan 2014, Preeti U Murthy wrote: > Hi Nicolas, > > On 01/30/2014 02:01 AM, Nicolas Pitre wrote: > > On Wed, 29 Jan 2014, Nicolas Pitre wrote: > > > >> In order to integrate cpuidle with the scheduler, we must have a better > >> proximity in the core code with what cpuidle is doing and not delegate > >> such interaction to arch code. > >> > >> Architectures implementing arch_cpu_idle() should simply enter > >> a cheap idle mode in the absence of a proper cpuidle driver. > >> > >> Signed-off-by: Nicolas Pitre <nico@linaro.org> > >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > > > As mentioned in my reply to Olof's comment on patch #5/6, here's a new > > version of this patch adding the safety local_irq_enable() to the core > > code. > > > > ----- >8 > > > > From: Nicolas Pitre <nicolas.pitre@linaro.org> > > Subject: idle: move the cpuidle entry point to the generic idle loop > > > > In order to integrate cpuidle with the scheduler, we must have a better > > proximity in the core code with what cpuidle is doing and not delegate > > such interaction to arch code. > > > > Architectures implementing arch_cpu_idle() should simply enter > > a cheap idle mode in the absence of a proper cpuidle driver. > > > > In both cases i.e. whether it is a cpuidle driver or the default > > arch_cpu_idle(), the calling convention expects IRQs to be disabled > > on entry and enabled on exit. There is a warning in place already but > > let's add a forced IRQ enable here as well. This will allow for > > removing the forced IRQ enable some implementations do locally and > > Why would this patch allow for removing the forced IRQ enable that are > being done on some archs in arch_cpu_idle()? Isn't this patch expecting > the default arch_cpu_idle() to have re-enabled the interrupts after > exiting from the default idle state? Its supposed to only catch faulty > cpuidle drivers that haven't enabled IRQs on exit from idle state but > are expected to have done so, isn't it? Exact. However x86 currently does this: if (cpuidle_idle_call()) x86_idle(); else local_irq_enable(); So whenever cpuidle_idle_call() is successful then IRQs are unconditionally enabled whether or not the underlying cpuidle driver has properly done it or not. And the reason is that some of the x86 cpuidle do fail to enable IRQs before returning. So the idea is to get rid of this unconditional IRQ enabling and let the core issue a warning instead (as well as enabling IRQs to allow the system to run). Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Nicolas, On 01/30/2014 10:58 AM, Nicolas Pitre wrote: > On Thu, 30 Jan 2014, Preeti U Murthy wrote: > >> Hi Nicolas, >> >> On 01/30/2014 02:01 AM, Nicolas Pitre wrote: >>> On Wed, 29 Jan 2014, Nicolas Pitre wrote: >>> >>>> In order to integrate cpuidle with the scheduler, we must have a better >>>> proximity in the core code with what cpuidle is doing and not delegate >>>> such interaction to arch code. >>>> >>>> Architectures implementing arch_cpu_idle() should simply enter >>>> a cheap idle mode in the absence of a proper cpuidle driver. >>>> >>>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> As mentioned in my reply to Olof's comment on patch #5/6, here's a new >>> version of this patch adding the safety local_irq_enable() to the core >>> code. >>> >>> ----- >8 >>> >>> From: Nicolas Pitre <nicolas.pitre@linaro.org> >>> Subject: idle: move the cpuidle entry point to the generic idle loop >>> >>> In order to integrate cpuidle with the scheduler, we must have a better >>> proximity in the core code with what cpuidle is doing and not delegate >>> such interaction to arch code. >>> >>> Architectures implementing arch_cpu_idle() should simply enter >>> a cheap idle mode in the absence of a proper cpuidle driver. >>> >>> In both cases i.e. whether it is a cpuidle driver or the default >>> arch_cpu_idle(), the calling convention expects IRQs to be disabled >>> on entry and enabled on exit. There is a warning in place already but >>> let's add a forced IRQ enable here as well. This will allow for >>> removing the forced IRQ enable some implementations do locally and >> >> Why would this patch allow for removing the forced IRQ enable that are >> being done on some archs in arch_cpu_idle()? Isn't this patch expecting >> the default arch_cpu_idle() to have re-enabled the interrupts after >> exiting from the default idle state? Its supposed to only catch faulty >> cpuidle drivers that haven't enabled IRQs on exit from idle state but >> are expected to have done so, isn't it? > > Exact. However x86 currently does this: > > if (cpuidle_idle_call()) > x86_idle(); > else > local_irq_enable(); > > So whenever cpuidle_idle_call() is successful then IRQs are > unconditionally enabled whether or not the underlying cpuidle driver has > properly done it or not. And the reason is that some of the x86 cpuidle > do fail to enable IRQs before returning. > > So the idea is to get rid of this unconditional IRQ enabling and let the > core issue a warning instead (as well as enabling IRQs to allow the > system to run). Oh ok, thank you for clarifying this:) Regards Preeti U Murthy > > > Nicolas > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/30/2014 06:28 AM, Nicolas Pitre wrote: > On Thu, 30 Jan 2014, Preeti U Murthy wrote: > >> Hi Nicolas, >> >> On 01/30/2014 02:01 AM, Nicolas Pitre wrote: >>> On Wed, 29 Jan 2014, Nicolas Pitre wrote: >>> >>>> In order to integrate cpuidle with the scheduler, we must have a better >>>> proximity in the core code with what cpuidle is doing and not delegate >>>> such interaction to arch code. >>>> >>>> Architectures implementing arch_cpu_idle() should simply enter >>>> a cheap idle mode in the absence of a proper cpuidle driver. >>>> >>>> Signed-off-by: Nicolas Pitre <nico@linaro.org> >>>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> >>> As mentioned in my reply to Olof's comment on patch #5/6, here's a new >>> version of this patch adding the safety local_irq_enable() to the core >>> code. >>> >>> ----- >8 >>> >>> From: Nicolas Pitre <nicolas.pitre@linaro.org> >>> Subject: idle: move the cpuidle entry point to the generic idle loop >>> >>> In order to integrate cpuidle with the scheduler, we must have a better >>> proximity in the core code with what cpuidle is doing and not delegate >>> such interaction to arch code. >>> >>> Architectures implementing arch_cpu_idle() should simply enter >>> a cheap idle mode in the absence of a proper cpuidle driver. >>> >>> In both cases i.e. whether it is a cpuidle driver or the default >>> arch_cpu_idle(), the calling convention expects IRQs to be disabled >>> on entry and enabled on exit. There is a warning in place already but >>> let's add a forced IRQ enable here as well. This will allow for >>> removing the forced IRQ enable some implementations do locally and >> >> Why would this patch allow for removing the forced IRQ enable that are >> being done on some archs in arch_cpu_idle()? Isn't this patch expecting >> the default arch_cpu_idle() to have re-enabled the interrupts after >> exiting from the default idle state? Its supposed to only catch faulty >> cpuidle drivers that haven't enabled IRQs on exit from idle state but >> are expected to have done so, isn't it? > > Exact. However x86 currently does this: > > if (cpuidle_idle_call()) > x86_idle(); > else > local_irq_enable(); > > So whenever cpuidle_idle_call() is successful then IRQs are > unconditionally enabled whether or not the underlying cpuidle driver has > properly done it or not. And the reason is that some of the x86 cpuidle > do fail to enable IRQs before returning. > > So the idea is to get rid of this unconditional IRQ enabling and let the > core issue a warning instead (as well as enabling IRQs to allow the > system to run). But what I don't get with your comment is the local_irq_enable is done from the cpuidle common framework in 'cpuidle_enter_state' it is not done from the arch specific backend cpuidle driver. So the code above could be: if (cpuidle_idle_call()) x86_idle(); without the else section, this local_irq_enable is pointless. Or may be I missed something ?
On Thu, 30 Jan 2014, Daniel Lezcano wrote: > On 01/30/2014 06:28 AM, Nicolas Pitre wrote: > > On Thu, 30 Jan 2014, Preeti U Murthy wrote: > > > > > Hi Nicolas, > > > > > > On 01/30/2014 02:01 AM, Nicolas Pitre wrote: > > > > On Wed, 29 Jan 2014, Nicolas Pitre wrote: > > > > > > > > > In order to integrate cpuidle with the scheduler, we must have a > > > > > better > > > > > proximity in the core code with what cpuidle is doing and not delegate > > > > > such interaction to arch code. > > > > > > > > > > Architectures implementing arch_cpu_idle() should simply enter > > > > > a cheap idle mode in the absence of a proper cpuidle driver. > > > > > > > > > > Signed-off-by: Nicolas Pitre <nico@linaro.org> > > > > > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > > > > > > > > As mentioned in my reply to Olof's comment on patch #5/6, here's a new > > > > version of this patch adding the safety local_irq_enable() to the core > > > > code. > > > > > > > > ----- >8 > > > > > > > > From: Nicolas Pitre <nicolas.pitre@linaro.org> > > > > Subject: idle: move the cpuidle entry point to the generic idle loop > > > > > > > > In order to integrate cpuidle with the scheduler, we must have a better > > > > proximity in the core code with what cpuidle is doing and not delegate > > > > such interaction to arch code. > > > > > > > > Architectures implementing arch_cpu_idle() should simply enter > > > > a cheap idle mode in the absence of a proper cpuidle driver. > > > > > > > > In both cases i.e. whether it is a cpuidle driver or the default > > > > arch_cpu_idle(), the calling convention expects IRQs to be disabled > > > > on entry and enabled on exit. There is a warning in place already but > > > > let's add a forced IRQ enable here as well. This will allow for > > > > removing the forced IRQ enable some implementations do locally and > > > > > > Why would this patch allow for removing the forced IRQ enable that are > > > being done on some archs in arch_cpu_idle()? Isn't this patch expecting > > > the default arch_cpu_idle() to have re-enabled the interrupts after > > > exiting from the default idle state? Its supposed to only catch faulty > > > cpuidle drivers that haven't enabled IRQs on exit from idle state but > > > are expected to have done so, isn't it? > > > > Exact. However x86 currently does this: > > > > if (cpuidle_idle_call()) > > x86_idle(); > > else > > local_irq_enable(); > > > > So whenever cpuidle_idle_call() is successful then IRQs are > > unconditionally enabled whether or not the underlying cpuidle driver has > > properly done it or not. And the reason is that some of the x86 cpuidle > > do fail to enable IRQs before returning. > > > > So the idea is to get rid of this unconditional IRQ enabling and let the > > core issue a warning instead (as well as enabling IRQs to allow the > > system to run). > > But what I don't get with your comment is the local_irq_enable is done from > the cpuidle common framework in 'cpuidle_enter_state' it is not done from the > arch specific backend cpuidle driver. Oh well... This certainly means we'll have to clean this mess as some drivers do it on their own while some others don't. Some drivers also loop on !need_resched() while some others simply return on the first interrupt. > So the code above could be: > > if (cpuidle_idle_call()) > x86_idle(); > > without the else section, this local_irq_enable is pointless. Or may be I > missed something ? A later patch removes it anyway. But if it is really necessary to enable interrupts then the core will do it but with a warning now. Nicolas -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c index 988573a9a3..14ca43430a 100644 --- a/kernel/cpu/idle.c +++ b/kernel/cpu/idle.c @@ -3,6 +3,7 @@ */ #include <linux/sched.h> #include <linux/cpu.h> +#include <linux/cpuidle.h> #include <linux/tick.h> #include <linux/mm.h> #include <linux/stackprotector.h> @@ -95,8 +96,10 @@ static void cpu_idle_loop(void) if (!current_clr_polling_and_test()) { stop_critical_timings(); rcu_idle_enter(); - arch_cpu_idle(); - WARN_ON_ONCE(irqs_disabled()); + if (cpuidle_idle_call()) + arch_cpu_idle(); + if (WARN_ON_ONCE(irqs_disabled())) + local_irq_enable(); rcu_idle_exit(); start_critical_timings(); } else {