Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Daniel Lezcano]

On 02/11/2014 05:09 PM, Preeti U Murthy wrote:

Hi Daniel,

Thank you very much for the review.

On 02/11/2014 03:46 PM, Daniel Lezcano wrote:

On 02/07/2014 09:06 AM, Preeti U Murthy wrote:

From: Thomas Gleixner 

On some architectures, in certain CPU deep idle states the local
timers stop.
An external clock device is used to wakeup these CPUs. The kernel
support for the
wakeup of these CPUs is provided by the tick broadcast framework by
using the
external clock device as the wakeup source.

However not all implementations of architectures provide such an external
clock device. This patch includes support in the broadcast framework
to handle
the wakeup of the CPUs in deep idle states on such systems by queuing
a hrtimer
on one of the CPUs, which is meant to handle the wakeup of CPUs in
deep idle states.

This patchset introduces a pseudo clock device which can be registered
by the
archs as tick_broadcast_device in the absence of a real external clock
device. Once registered, the broadcast framework will work as is for
these
architectures as long as the archs take care of the BROADCAST_ENTER
notification failing for one of the CPUs. This CPU is made the stand
by CPU to
handle wakeup of the CPUs in deep idle and it *must not enter deep
idle states*.

The CPU with the earliest wakeup is chosen to be this CPU. Hence this
way the
stand by CPU dynamically moves around and so does the hrtimer which is
queued
to trigger at the next earliest wakeup time. This is consistent with
the case where
an external clock device is present. The smp affinity of this clock
device is
set to the CPU with the earliest wakeup.


Hi Preeti,

jumping a bit late in the thread...

Setting the smp affinity on the earliest timer should be handled
automatically with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using
this flag ?


This patch is not setting the smp affinity of the pseudo clock device at
all. Its not required to for the reason that it does not exist.

I mentioned this point because we assign a CPU with the earliest wakeup
as standby. I compared this logic to the one used by the tick broadcast
framework for archs which have an external clock device to set the smp
affinity of the device.

If these archs do not have the flag CLOCK_EVT_FEAT_DYNIRQ set for the
external clock device, the tick broadcast framework sets the smp
affinity of this device to the CPU with the earliest wakeup. We are
using the same logic in this patchset as well to assign the stand by CPU.



Another comment is the overall approach. We enter the cpuidle idle
framework with a specific state to go to and it is the tick framework
telling us we mustn't go to this state. IMO the logic is wrong, the
decision to not enter this state should be moved somewhere else.


Its not the tick framework which tells us that we cannot enter deep idle
state, its the *tick broadcast* framework specifically. The tick
broadcast framework was introduced with the primary intention of
handling wakeup of CPUs in deep idle states when the local timers become
non-functional. Therefore there is a co-operation between this tick
broadcast framework and cpuidle. This has always been the case.

That is why just before cpus go into deep idle, they call into the
broadcast framework. Till now it was assumed that the tick broadcast
framework would find no problems with the cpus entering deep idle.
Therefore cpuidle would simply assume that all is well and go ahead and
enter deep idle state.
   But today there is a scenario when there could be problems if all cpus
enter deep idle states and the tick broadcast framework now notifies the
cpuidle framework to hold back one cpu. This is just a simple extension
of the current interaction between cpuidle and tick broadcast framework.



Why don't you create a cpuidle driver with the shallow idle states
assigned to a cpu (let's say cpu0) and another one with all the deeper
idle states for the rest of the cpus ? Using the multiple cpuidle driver
support makes it possible. The timer won't be moving around and a cpu
will be dedicated to act as the broadcast timer.



Having a dedicated stand by cpu for broadcasting has some issues which
were pointed to when I posted the initial versions of this patchset.
https://lkml.org/lkml/2013/7/27/14

1. This could create power/thermal imbalance on the chip since only the
standby cpu cannot enter deep idle state at all times.

2. If it is cpu0 it is fine, else with the logic that you suggest,
hot-plugging out the dedicated stand by cpu would mean moving the work
of broadcasting to another cpu and modifying the cpuidle state table for
it. Even with cpu0, if support to hotplug it out is enabled (maybe it is
already), we will face the same issue and this gets very messy.


Wouldn't make sense and be less intrusive than the patchset you proposed ?


Actually this patchset brings in a solution that is as less intrusive as
possible. It makes the problem nearly invisible except for a failed
return from a call into the 

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Daniel Lezcano]

On 02/11/2014 04:58 PM, Thomas Gleixner wrote:

On Tue, 11 Feb 2014, Daniel Lezcano wrote:

On 02/07/2014 09:06 AM, Preeti U Murthy wrote:
Setting the smp affinity on the earliest timer should be handled automatically
with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using this flag ?


How should this flag help? Not at all, because the hrtimer based
broadcast device cannot assign affinities.


Another comment is the overall approach. We enter the cpuidle idle framework
with a specific state to go to and it is the tick framework telling us we
mustn't go to this state. IMO the logic is wrong, the decision to not enter
this state should be moved somewhere else.

Why don't you create a cpuidle driver with the shallow idle states assigned to
a cpu (let's say cpu0) and another one with all the deeper idle states for the
rest of the cpus ? Using the multiple cpuidle driver support makes it
possible. The timer won't be moving around and a cpu will be dedicated to act
as the broadcast timer.

Wouldn't make sense and be less intrusive than the patchset you proposed ?


How do you arm the broadcast timer on CPU0 from CPU1? You can't!

You cannot access the cpu local timer on a different cpu. So you would
have to send an IPI over to CPU0 so that it can reevaluate and
schedule the broadcast. That's even more backwards than telling the
cpuidle code that the CPU is not in a state to go deep.


Indeed :)

Thanks for the clarification.

  -- Daniel

--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Preeti U Murthy]

Hi Daniel,

Thank you very much for the review.

On 02/11/2014 03:46 PM, Daniel Lezcano wrote:
> On 02/07/2014 09:06 AM, Preeti U Murthy wrote:
>> From: Thomas Gleixner 
>>
>> On some architectures, in certain CPU deep idle states the local
>> timers stop.
>> An external clock device is used to wakeup these CPUs. The kernel
>> support for the
>> wakeup of these CPUs is provided by the tick broadcast framework by
>> using the
>> external clock device as the wakeup source.
>>
>> However not all implementations of architectures provide such an external
>> clock device. This patch includes support in the broadcast framework
>> to handle
>> the wakeup of the CPUs in deep idle states on such systems by queuing
>> a hrtimer
>> on one of the CPUs, which is meant to handle the wakeup of CPUs in
>> deep idle states.
>>
>> This patchset introduces a pseudo clock device which can be registered
>> by the
>> archs as tick_broadcast_device in the absence of a real external clock
>> device. Once registered, the broadcast framework will work as is for
>> these
>> architectures as long as the archs take care of the BROADCAST_ENTER
>> notification failing for one of the CPUs. This CPU is made the stand
>> by CPU to
>> handle wakeup of the CPUs in deep idle and it *must not enter deep
>> idle states*.
>>
>> The CPU with the earliest wakeup is chosen to be this CPU. Hence this
>> way the
>> stand by CPU dynamically moves around and so does the hrtimer which is
>> queued
>> to trigger at the next earliest wakeup time. This is consistent with
>> the case where
>> an external clock device is present. The smp affinity of this clock
>> device is
>> set to the CPU with the earliest wakeup.
> 
> Hi Preeti,
> 
> jumping a bit late in the thread...
> 
> Setting the smp affinity on the earliest timer should be handled
> automatically with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using
> this flag ?

This patch is not setting the smp affinity of the pseudo clock device at
all. Its not required to for the reason that it does not exist.

I mentioned this point because we assign a CPU with the earliest wakeup
as standby. I compared this logic to the one used by the tick broadcast
framework for archs which have an external clock device to set the smp
affinity of the device.

If these archs do not have the flag CLOCK_EVT_FEAT_DYNIRQ set for the
external clock device, the tick broadcast framework sets the smp
affinity of this device to the CPU with the earliest wakeup. We are
using the same logic in this patchset as well to assign the stand by CPU.

> 
> Another comment is the overall approach. We enter the cpuidle idle
> framework with a specific state to go to and it is the tick framework
> telling us we mustn't go to this state. IMO the logic is wrong, the
> decision to not enter this state should be moved somewhere else.

Its not the tick framework which tells us that we cannot enter deep idle
state, its the *tick broadcast* framework specifically. The tick
broadcast framework was introduced with the primary intention of
handling wakeup of CPUs in deep idle states when the local timers become
non-functional. Therefore there is a co-operation between this tick
broadcast framework and cpuidle. This has always been the case.

That is why just before cpus go into deep idle, they call into the
broadcast framework. Till now it was assumed that the tick broadcast
framework would find no problems with the cpus entering deep idle.
Therefore cpuidle would simply assume that all is well and go ahead and
enter deep idle state.
  But today there is a scenario when there could be problems if all cpus
enter deep idle states and the tick broadcast framework now notifies the
cpuidle framework to hold back one cpu. This is just a simple extension
of the current interaction between cpuidle and tick broadcast framework.

> 
> Why don't you create a cpuidle driver with the shallow idle states
> assigned to a cpu (let's say cpu0) and another one with all the deeper
> idle states for the rest of the cpus ? Using the multiple cpuidle driver
> support makes it possible. The timer won't be moving around and a cpu
> will be dedicated to act as the broadcast timer.
> 

Having a dedicated stand by cpu for broadcasting has some issues which
were pointed to when I posted the initial versions of this patchset.
https://lkml.org/lkml/2013/7/27/14

1. This could create power/thermal imbalance on the chip since only the
standby cpu cannot enter deep idle state at all times.

2. If it is cpu0 it is fine, else with the logic that you suggest,
hot-plugging out the dedicated stand by cpu would mean moving the work
of broadcasting to another cpu and modifying the cpuidle state table for
it. Even with cpu0, if support to hotplug it out is enabled (maybe it is
already), we will face the same issue and this gets very messy.

> Wouldn't make sense and be less intrusive than the patchset you proposed ?

Actually this patchset brings in a solution that is as less intrusive as

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Thomas Gleixner]

On Tue, 11 Feb 2014, Daniel Lezcano wrote:
> On 02/07/2014 09:06 AM, Preeti U Murthy wrote:
> Setting the smp affinity on the earliest timer should be handled automatically
> with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using this flag ?

How should this flag help? Not at all, because the hrtimer based
broadcast device cannot assign affinities.
 
> Another comment is the overall approach. We enter the cpuidle idle framework
> with a specific state to go to and it is the tick framework telling us we
> mustn't go to this state. IMO the logic is wrong, the decision to not enter
> this state should be moved somewhere else.
> 
> Why don't you create a cpuidle driver with the shallow idle states assigned to
> a cpu (let's say cpu0) and another one with all the deeper idle states for the
> rest of the cpus ? Using the multiple cpuidle driver support makes it
> possible. The timer won't be moving around and a cpu will be dedicated to act
> as the broadcast timer.
> 
> Wouldn't make sense and be less intrusive than the patchset you proposed ?

How do you arm the broadcast timer on CPU0 from CPU1? You can't!

You cannot access the cpu local timer on a different cpu. So you would
have to send an IPI over to CPU0 so that it can reevaluate and
schedule the broadcast. That's even more backwards than telling the
cpuidle code that the CPU is not in a state to go deep.

Thanks,

tglx


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Daniel Lezcano]

On 02/07/2014 09:06 AM, Preeti U Murthy wrote:

From: Thomas Gleixner 

On some architectures, in certain CPU deep idle states the local timers stop.
An external clock device is used to wakeup these CPUs. The kernel support for 
the
wakeup of these CPUs is provided by the tick broadcast framework by using the
external clock device as the wakeup source.

However not all implementations of architectures provide such an external
clock device. This patch includes support in the broadcast framework to handle
the wakeup of the CPUs in deep idle states on such systems by queuing a hrtimer
on one of the CPUs, which is meant to handle the wakeup of CPUs in deep idle 
states.

This patchset introduces a pseudo clock device which can be registered by the
archs as tick_broadcast_device in the absence of a real external clock
device. Once registered, the broadcast framework will work as is for these
architectures as long as the archs take care of the BROADCAST_ENTER
notification failing for one of the CPUs. This CPU is made the stand by CPU to
handle wakeup of the CPUs in deep idle and it *must not enter deep idle states*.

The CPU with the earliest wakeup is chosen to be this CPU. Hence this way the
stand by CPU dynamically moves around and so does the hrtimer which is queued
to trigger at the next earliest wakeup time. This is consistent with the case 
where
an external clock device is present. The smp affinity of this clock device is
set to the CPU with the earliest wakeup.


Hi Preeti,

jumping a bit late in the thread...

Setting the smp affinity on the earliest timer should be handled 
automatically with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using 
this flag ?


Another comment is the overall approach. We enter the cpuidle idle 
framework with a specific state to go to and it is the tick framework 
telling us we mustn't go to this state. IMO the logic is wrong, the 
decision to not enter this state should be moved somewhere else.


Why don't you create a cpuidle driver with the shallow idle states 
assigned to a cpu (let's say cpu0) and another one with all the deeper 
idle states for the rest of the cpus ? Using the multiple cpuidle driver 
support makes it possible. The timer won't be moving around and a cpu 
will be dedicated to act as the broadcast timer.


Wouldn't make sense and be less intrusive than the patchset you proposed ?



This patchset handles the hotplug of
the stand by CPU as well by moving the hrtimer on to the CPU handling the 
CPU_DEAD
notification.

Signed-off-by: Preeti U Murthy 
[Added Changelog and code to handle reprogramming of hrtimer]
---

  include/linux/clockchips.h   |9 +++
  kernel/time/Makefile |2 -
  kernel/time/tick-broadcast-hrtimer.c |  105 ++
  kernel/time/tick-broadcast.c |   54 +
  4 files changed, 166 insertions(+), 4 deletions(-)
  create mode 100644 kernel/time/tick-broadcast-hrtimer.c

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index e0c5a6c..dbe9e14 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -62,6 +62,11 @@ enum clock_event_mode {
  #define CLOCK_EVT_FEAT_DYNIRQ 0x20
  #define CLOCK_EVT_FEAT_PERCPU 0x40

+/*
+ * Clockevent device is based on a hrtimer for broadcast
+ */
+#define CLOCK_EVT_FEAT_HRTIMER 0x80
+
  /**
   * struct clock_event_device - clock event device descriptor
   * @event_handler:Assigned by the framework to be called by the low
@@ -83,6 +88,7 @@ enum clock_event_mode {
   * @name: ptr to clock event name
   * @rating:   variable to rate clock event devices
   * @irq:  IRQ number (only for non CPU local devices)
+ * @bound_on:  Bound on CPU
   * @cpumask:  cpumask to indicate for which CPUs this device works
   * @list: list head for the management code
   * @owner:module reference
@@ -113,6 +119,7 @@ struct clock_event_device {
const char  *name;
int rating;
int irq;
+   int bound_on;
const struct cpumask*cpumask;
struct list_headlist;
struct module   *owner;
@@ -180,9 +187,11 @@ extern int tick_receive_broadcast(void);
  #endif

  #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && 
defined(CONFIG_TICK_ONESHOT)
+extern void tick_setup_hrtimer_broadcast(void);
  extern int tick_check_broadcast_expired(void);
  #else
  static inline int tick_check_broadcast_expired(void) { return 0; }
+static void tick_setup_hrtimer_broadcast(void) {};
  #endif

  #ifdef CONFIG_GENERIC_CLOCKEVENTS
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 9250130..06151ef 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -3,7 +3,7 @@ obj-y += timeconv.o posix-clock.o alarmtimer.o

  obj-$(CONFIG_GENERIC_CLOCKEVENTS_BUILD)   += 

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Daniel Lezcano]

On 02/07/2014 09:06 AM, Preeti U Murthy wrote:

From: Thomas Gleixner t...@linutronix.de

On some architectures, in certain CPU deep idle states the local timers stop.
An external clock device is used to wakeup these CPUs. The kernel support for 
the
wakeup of these CPUs is provided by the tick broadcast framework by using the
external clock device as the wakeup source.

However not all implementations of architectures provide such an external
clock device. This patch includes support in the broadcast framework to handle
the wakeup of the CPUs in deep idle states on such systems by queuing a hrtimer
on one of the CPUs, which is meant to handle the wakeup of CPUs in deep idle 
states.

This patchset introduces a pseudo clock device which can be registered by the
archs as tick_broadcast_device in the absence of a real external clock
device. Once registered, the broadcast framework will work as is for these
architectures as long as the archs take care of the BROADCAST_ENTER
notification failing for one of the CPUs. This CPU is made the stand by CPU to
handle wakeup of the CPUs in deep idle and it *must not enter deep idle states*.

The CPU with the earliest wakeup is chosen to be this CPU. Hence this way the
stand by CPU dynamically moves around and so does the hrtimer which is queued
to trigger at the next earliest wakeup time. This is consistent with the case 
where
an external clock device is present. The smp affinity of this clock device is
set to the CPU with the earliest wakeup.


Hi Preeti,

jumping a bit late in the thread...

Setting the smp affinity on the earliest timer should be handled 
automatically with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using 
this flag ?


Another comment is the overall approach. We enter the cpuidle idle 
framework with a specific state to go to and it is the tick framework 
telling us we mustn't go to this state. IMO the logic is wrong, the 
decision to not enter this state should be moved somewhere else.


Why don't you create a cpuidle driver with the shallow idle states 
assigned to a cpu (let's say cpu0) and another one with all the deeper 
idle states for the rest of the cpus ? Using the multiple cpuidle driver 
support makes it possible. The timer won't be moving around and a cpu 
will be dedicated to act as the broadcast timer.


Wouldn't make sense and be less intrusive than the patchset you proposed ?



This patchset handles the hotplug of
the stand by CPU as well by moving the hrtimer on to the CPU handling the 
CPU_DEAD
notification.

Signed-off-by: Preeti U Murthy pre...@linux.vnet.ibm.com
[Added Changelog and code to handle reprogramming of hrtimer]
---

  include/linux/clockchips.h   |9 +++
  kernel/time/Makefile |2 -
  kernel/time/tick-broadcast-hrtimer.c |  105 ++
  kernel/time/tick-broadcast.c |   54 +
  4 files changed, 166 insertions(+), 4 deletions(-)
  create mode 100644 kernel/time/tick-broadcast-hrtimer.c

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index e0c5a6c..dbe9e14 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -62,6 +62,11 @@ enum clock_event_mode {
  #define CLOCK_EVT_FEAT_DYNIRQ 0x20
  #define CLOCK_EVT_FEAT_PERCPU 0x40

+/*
+ * Clockevent device is based on a hrtimer for broadcast
+ */
+#define CLOCK_EVT_FEAT_HRTIMER 0x80
+
  /**
   * struct clock_event_device - clock event device descriptor
   * @event_handler:Assigned by the framework to be called by the low
@@ -83,6 +88,7 @@ enum clock_event_mode {
   * @name: ptr to clock event name
   * @rating:   variable to rate clock event devices
   * @irq:  IRQ number (only for non CPU local devices)
+ * @bound_on:  Bound on CPU
   * @cpumask:  cpumask to indicate for which CPUs this device works
   * @list: list head for the management code
   * @owner:module reference
@@ -113,6 +119,7 @@ struct clock_event_device {
const char  *name;
int rating;
int irq;
+   int bound_on;
const struct cpumask*cpumask;
struct list_headlist;
struct module   *owner;
@@ -180,9 +187,11 @@ extern int tick_receive_broadcast(void);
  #endif

  #if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST)  
defined(CONFIG_TICK_ONESHOT)
+extern void tick_setup_hrtimer_broadcast(void);
  extern int tick_check_broadcast_expired(void);
  #else
  static inline int tick_check_broadcast_expired(void) { return 0; }
+static void tick_setup_hrtimer_broadcast(void) {};
  #endif

  #ifdef CONFIG_GENERIC_CLOCKEVENTS
diff --git a/kernel/time/Makefile b/kernel/time/Makefile
index 9250130..06151ef 100644
--- a/kernel/time/Makefile
+++ b/kernel/time/Makefile
@@ -3,7 +3,7 @@ obj-y += timeconv.o posix-clock.o alarmtimer.o

  

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Thomas Gleixner]

On Tue, 11 Feb 2014, Daniel Lezcano wrote:
 On 02/07/2014 09:06 AM, Preeti U Murthy wrote:
 Setting the smp affinity on the earliest timer should be handled automatically
 with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using this flag ?

How should this flag help? Not at all, because the hrtimer based
broadcast device cannot assign affinities.
 
 Another comment is the overall approach. We enter the cpuidle idle framework
 with a specific state to go to and it is the tick framework telling us we
 mustn't go to this state. IMO the logic is wrong, the decision to not enter
 this state should be moved somewhere else.
 
 Why don't you create a cpuidle driver with the shallow idle states assigned to
 a cpu (let's say cpu0) and another one with all the deeper idle states for the
 rest of the cpus ? Using the multiple cpuidle driver support makes it
 possible. The timer won't be moving around and a cpu will be dedicated to act
 as the broadcast timer.
 
 Wouldn't make sense and be less intrusive than the patchset you proposed ?

How do you arm the broadcast timer on CPU0 from CPU1? You can't!

You cannot access the cpu local timer on a different cpu. So you would
have to send an IPI over to CPU0 so that it can reevaluate and
schedule the broadcast. That's even more backwards than telling the
cpuidle code that the CPU is not in a state to go deep.

Thanks,

tglx


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Preeti U Murthy]

Hi Daniel,

Thank you very much for the review.

On 02/11/2014 03:46 PM, Daniel Lezcano wrote:
 On 02/07/2014 09:06 AM, Preeti U Murthy wrote:
 From: Thomas Gleixner t...@linutronix.de

 On some architectures, in certain CPU deep idle states the local
 timers stop.
 An external clock device is used to wakeup these CPUs. The kernel
 support for the
 wakeup of these CPUs is provided by the tick broadcast framework by
 using the
 external clock device as the wakeup source.

 However not all implementations of architectures provide such an external
 clock device. This patch includes support in the broadcast framework
 to handle
 the wakeup of the CPUs in deep idle states on such systems by queuing
 a hrtimer
 on one of the CPUs, which is meant to handle the wakeup of CPUs in
 deep idle states.

 This patchset introduces a pseudo clock device which can be registered
 by the
 archs as tick_broadcast_device in the absence of a real external clock
 device. Once registered, the broadcast framework will work as is for
 these
 architectures as long as the archs take care of the BROADCAST_ENTER
 notification failing for one of the CPUs. This CPU is made the stand
 by CPU to
 handle wakeup of the CPUs in deep idle and it *must not enter deep
 idle states*.

 The CPU with the earliest wakeup is chosen to be this CPU. Hence this
 way the
 stand by CPU dynamically moves around and so does the hrtimer which is
 queued
 to trigger at the next earliest wakeup time. This is consistent with
 the case where
 an external clock device is present. The smp affinity of this clock
 device is
 set to the CPU with the earliest wakeup.
 
 Hi Preeti,
 
 jumping a bit late in the thread...
 
 Setting the smp affinity on the earliest timer should be handled
 automatically with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using
 this flag ?

This patch is not setting the smp affinity of the pseudo clock device at
all. Its not required to for the reason that it does not exist.

I mentioned this point because we assign a CPU with the earliest wakeup
as standby. I compared this logic to the one used by the tick broadcast
framework for archs which have an external clock device to set the smp
affinity of the device.

If these archs do not have the flag CLOCK_EVT_FEAT_DYNIRQ set for the
external clock device, the tick broadcast framework sets the smp
affinity of this device to the CPU with the earliest wakeup. We are
using the same logic in this patchset as well to assign the stand by CPU.

 
 Another comment is the overall approach. We enter the cpuidle idle
 framework with a specific state to go to and it is the tick framework
 telling us we mustn't go to this state. IMO the logic is wrong, the
 decision to not enter this state should be moved somewhere else.

Its not the tick framework which tells us that we cannot enter deep idle
state, its the *tick broadcast* framework specifically. The tick
broadcast framework was introduced with the primary intention of
handling wakeup of CPUs in deep idle states when the local timers become
non-functional. Therefore there is a co-operation between this tick
broadcast framework and cpuidle. This has always been the case.

That is why just before cpus go into deep idle, they call into the
broadcast framework. Till now it was assumed that the tick broadcast
framework would find no problems with the cpus entering deep idle.
Therefore cpuidle would simply assume that all is well and go ahead and
enter deep idle state.
  But today there is a scenario when there could be problems if all cpus
enter deep idle states and the tick broadcast framework now notifies the
cpuidle framework to hold back one cpu. This is just a simple extension
of the current interaction between cpuidle and tick broadcast framework.

 
 Why don't you create a cpuidle driver with the shallow idle states
 assigned to a cpu (let's say cpu0) and another one with all the deeper
 idle states for the rest of the cpus ? Using the multiple cpuidle driver
 support makes it possible. The timer won't be moving around and a cpu
 will be dedicated to act as the broadcast timer.
 

Having a dedicated stand by cpu for broadcasting has some issues which
were pointed to when I posted the initial versions of this patchset.
https://lkml.org/lkml/2013/7/27/14

1. This could create power/thermal imbalance on the chip since only the
standby cpu cannot enter deep idle state at all times.

2. If it is cpu0 it is fine, else with the logic that you suggest,
hot-plugging out the dedicated stand by cpu would mean moving the work
of broadcasting to another cpu and modifying the cpuidle state table for
it. Even with cpu0, if support to hotplug it out is enabled (maybe it is
already), we will face the same issue and this gets very messy.

 Wouldn't make sense and be less intrusive than the patchset you proposed ?

Actually this patchset brings in a solution that is as less intrusive as
possible. It makes the problem nearly invisible except for a failed
return from a 

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Daniel Lezcano]

On 02/11/2014 04:58 PM, Thomas Gleixner wrote:

On Tue, 11 Feb 2014, Daniel Lezcano wrote:

On 02/07/2014 09:06 AM, Preeti U Murthy wrote:
Setting the smp affinity on the earliest timer should be handled automatically
with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using this flag ?


How should this flag help? Not at all, because the hrtimer based
broadcast device cannot assign affinities.


Another comment is the overall approach. We enter the cpuidle idle framework
with a specific state to go to and it is the tick framework telling us we
mustn't go to this state. IMO the logic is wrong, the decision to not enter
this state should be moved somewhere else.

Why don't you create a cpuidle driver with the shallow idle states assigned to
a cpu (let's say cpu0) and another one with all the deeper idle states for the
rest of the cpus ? Using the multiple cpuidle driver support makes it
possible. The timer won't be moving around and a cpu will be dedicated to act
as the broadcast timer.

Wouldn't make sense and be less intrusive than the patchset you proposed ?


How do you arm the broadcast timer on CPU0 from CPU1? You can't!

You cannot access the cpu local timer on a different cpu. So you would
have to send an IPI over to CPU0 so that it can reevaluate and
schedule the broadcast. That's even more backwards than telling the
cpuidle code that the CPU is not in a state to go deep.


Indeed :)

Thanks for the clarification.

  -- Daniel

--
 http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  http://www.facebook.com/pages/Linaro Facebook |
http://twitter.com/#!/linaroorg Twitter |
http://www.linaro.org/linaro-blog/ Blog

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V4 2/3] tick/cpuidle: Initialize hrtimer mode of broadcast

[Daniel Lezcano]

On 02/11/2014 05:09 PM, Preeti U Murthy wrote:

Hi Daniel,

Thank you very much for the review.

On 02/11/2014 03:46 PM, Daniel Lezcano wrote:

On 02/07/2014 09:06 AM, Preeti U Murthy wrote:

From: Thomas Gleixner t...@linutronix.de

On some architectures, in certain CPU deep idle states the local
timers stop.
An external clock device is used to wakeup these CPUs. The kernel
support for the
wakeup of these CPUs is provided by the tick broadcast framework by
using the
external clock device as the wakeup source.

However not all implementations of architectures provide such an external
clock device. This patch includes support in the broadcast framework
to handle
the wakeup of the CPUs in deep idle states on such systems by queuing
a hrtimer
on one of the CPUs, which is meant to handle the wakeup of CPUs in
deep idle states.

This patchset introduces a pseudo clock device which can be registered
by the
archs as tick_broadcast_device in the absence of a real external clock
device. Once registered, the broadcast framework will work as is for
these
architectures as long as the archs take care of the BROADCAST_ENTER
notification failing for one of the CPUs. This CPU is made the stand
by CPU to
handle wakeup of the CPUs in deep idle and it *must not enter deep
idle states*.

The CPU with the earliest wakeup is chosen to be this CPU. Hence this
way the
stand by CPU dynamically moves around and so does the hrtimer which is
queued
to trigger at the next earliest wakeup time. This is consistent with
the case where
an external clock device is present. The smp affinity of this clock
device is
set to the CPU with the earliest wakeup.


Hi Preeti,

jumping a bit late in the thread...

Setting the smp affinity on the earliest timer should be handled
automatically with the CLOCK_EVT_FEAT_DYNIRQ flag. Did you look at using
this flag ?


This patch is not setting the smp affinity of the pseudo clock device at
all. Its not required to for the reason that it does not exist.

I mentioned this point because we assign a CPU with the earliest wakeup
as standby. I compared this logic to the one used by the tick broadcast
framework for archs which have an external clock device to set the smp
affinity of the device.

If these archs do not have the flag CLOCK_EVT_FEAT_DYNIRQ set for the
external clock device, the tick broadcast framework sets the smp
affinity of this device to the CPU with the earliest wakeup. We are
using the same logic in this patchset as well to assign the stand by CPU.



Another comment is the overall approach. We enter the cpuidle idle
framework with a specific state to go to and it is the tick framework
telling us we mustn't go to this state. IMO the logic is wrong, the
decision to not enter this state should be moved somewhere else.


Its not the tick framework which tells us that we cannot enter deep idle
state, its the *tick broadcast* framework specifically. The tick
broadcast framework was introduced with the primary intention of
handling wakeup of CPUs in deep idle states when the local timers become
non-functional. Therefore there is a co-operation between this tick
broadcast framework and cpuidle. This has always been the case.

That is why just before cpus go into deep idle, they call into the
broadcast framework. Till now it was assumed that the tick broadcast
framework would find no problems with the cpus entering deep idle.
Therefore cpuidle would simply assume that all is well and go ahead and
enter deep idle state.
   But today there is a scenario when there could be problems if all cpus
enter deep idle states and the tick broadcast framework now notifies the
cpuidle framework to hold back one cpu. This is just a simple extension
of the current interaction between cpuidle and tick broadcast framework.



Why don't you create a cpuidle driver with the shallow idle states
assigned to a cpu (let's say cpu0) and another one with all the deeper
idle states for the rest of the cpus ? Using the multiple cpuidle driver
support makes it possible. The timer won't be moving around and a cpu
will be dedicated to act as the broadcast timer.



Having a dedicated stand by cpu for broadcasting has some issues which
were pointed to when I posted the initial versions of this patchset.
https://lkml.org/lkml/2013/7/27/14

1. This could create power/thermal imbalance on the chip since only the
standby cpu cannot enter deep idle state at all times.

2. If it is cpu0 it is fine, else with the logic that you suggest,
hot-plugging out the dedicated stand by cpu would mean moving the work
of broadcasting to another cpu and modifying the cpuidle state table for
it. Even with cpu0, if support to hotplug it out is enabled (maybe it is
already), we will face the same issue and this gets very messy.


Wouldn't make sense and be less intrusive than the patchset you proposed ?


Actually this patchset brings in a solution that is as less intrusive as
possible. It makes the problem nearly invisible except for a failed
return