Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Preeti U Murthy]

On 04/29/2015 06:34 AM, Rafael J. Wysocki wrote:
> On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:
>> On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:
>>>
>>> On 28/04/15 15:14, Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
> On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
>> On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
>>> On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  
>>> wrote:
 Sudeep:
> At-least I observed issue only when I am using hardware broadcast 
> timer.
> It doesn't hang when I am using hrtimer as broadcast timer in which 
> case
> one of the cpu will be not enter deeper idle states that lose timer.
> I will rerun on v4.1-rc1 and post the complete log.

 So the bug here is that cpuidle_enter() enables interrupts, so the
 assumption about them being not enabled made by
 tick_broadcast_oneshot_control() is actually not valid.

 It looks like we need to acquire the clockevents_lock at least in this
 particular case.  Let me see where to put it and I'll send a patch for
 testing.
>>>
>>> Aha that looks very much like it. Put me on the patch and I'll
>>> take it for a spin.
>>
>> OK, so something like the below for starters (the _irqsave variant is 
>> used to
>> avoid adding one more WARN_ON(irqs_disabled()) in there).
>>
>> I haven't tested it, but then I can't reproduce the original issue in the
>> first place.
>
> Of course, the whole "broadcast" thing could be done from cpuidle_enter()
> in the first place, but then we could not avoid the problem with the 
> cpuidle
> *callback* enabling interrupts possibly in there anyway (not to mention 
> the
> "coupled" stuff).

 That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I 
 really
 wouldn't expect it to re-enable interrupts on exit and the "coupled" thing
 seems to be fundamentally at odds with that flag either.

 So it should be possible to move the "broadcast" logic into the cpuidle 
 layer,
 which I'm going to try to do.

>>>
>>> Makes sense.
>>>
 Please test the patch I've sent, though, as it should bring the code back 
 to
 where it was before the clockevents_notify() removal and it'd be good to 
 verify
 that.

>>>
>>> I tested your patch and it works now. Anyways I am continuing to run
>>> stress tests on my board. I will report if I find any issues.
>>
>> Great, thanks!
>>
>> Below is the patch I came up with in the meantime.
>>
>> This moves the "switch to broadcast" timer logic into
>> cpuidle_enter_state() which allows tick_broadcast_exit() to be
>> called directly with interrupts disabled (as required), but
>> it also adds a fallback branch reflecting the 4.0 and earlier
>> behavior for idle states that enable interrupts on exit
>> from their ->enter callbacks.
>>
>> I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
>> set for such states, but people may try to add stuff like that in the
>> future, so it's better to catch that (hence the WARN_ON_ONCE) and do
>> our best to handle it gracefully anyway, IMO.
>>
>> The "if (entered_state == -EBUSY)" check is conservative.  It may
>> be better to do "if (entered_state < 0)" and fall back to the default
>> on all errors, but that's not what we do today (I guess the concern
>> would be "what if the state ->enter returns an error after entering
>> and exiting the idle state, in which case we may miss a wakeup event
>> if we fall back to the default").
> 
> Actually, if my understanding of things is correct (the local clock event
> device cannot go away from under code executed with interrupts disabled
> on the local CPU), the simplified one below should be sufficient.
> 
> ---
>  drivers/cpuidle/cpuidle.c |   16 
>  kernel/sched/idle.c   |   16 ++--
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> Index: linux-pm/kernel/sched/idle.c
> ===
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
>   struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>   struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>   int next_state, entered_state;
> - unsigned int broadcast;
>   bool reflect;
> 
>   /*
> @@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
>   goto exit_idle;
>   }
> 
> - broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
> -
> - /*
> -  * Tell the time framework to switch to a broadcast timer
> -  * because our local timer will be shutdown. If a local timer
> -  * is used from another cpu as a 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

Hi Rafael,

On 29/04/15 02:04, Rafael J. Wysocki wrote:

On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:


[...]



Below is the patch I came up with in the meantime.

This moves the "switch to broadcast" timer logic into
cpuidle_enter_state() which allows tick_broadcast_exit() to be
called directly with interrupts disabled (as required), but
it also adds a fallback branch reflecting the 4.0 and earlier
behavior for idle states that enable interrupts on exit
from their ->enter callbacks.

I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
set for such states, but people may try to add stuff like that in the
future, so it's better to catch that (hence the WARN_ON_ONCE) and do
our best to handle it gracefully anyway, IMO.

The "if (entered_state == -EBUSY)" check is conservative.  It may
be better to do "if (entered_state < 0)" and fall back to the default
on all errors, but that's not what we do today (I guess the concern
would be "what if the state ->enter returns an error after entering
and exiting the idle state, in which case we may miss a wakeup event
if we fall back to the default").


Actually, if my understanding of things is correct (the local clock event
device cannot go away from under code executed with interrupts disabled
on the local CPU), the simplified one below should be sufficient.



Tested-by: Sudeep Holla 

Regards,
Sudeep
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Daniel Lezcano]

On 04/29/2015 03:04 AM, Rafael J. Wysocki wrote:

On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:


On 28/04/15 15:14, Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:

On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  wrote:

Sudeep:

At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.


So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.


Aha that looks very much like it. Put me on the patch and I'll
take it for a spin.


OK, so something like the below for starters (the _irqsave variant is used to
avoid adding one more WARN_ON(irqs_disabled()) in there).

I haven't tested it, but then I can't reproduce the original issue in the
first place.


Of course, the whole "broadcast" thing could be done from cpuidle_enter()
in the first place, but then we could not avoid the problem with the cpuidle
*callback* enabling interrupts possibly in there anyway (not to mention the
"coupled" stuff).


That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I really
wouldn't expect it to re-enable interrupts on exit and the "coupled" thing
seems to be fundamentally at odds with that flag either.

So it should be possible to move the "broadcast" logic into the cpuidle layer,
which I'm going to try to do.



Makes sense.


Please test the patch I've sent, though, as it should bring the code back to
where it was before the clockevents_notify() removal and it'd be good to verify
that.



I tested your patch and it works now. Anyways I am continuing to run
stress tests on my board. I will report if I find any issues.


Great, thanks!

Below is the patch I came up with in the meantime.

This moves the "switch to broadcast" timer logic into
cpuidle_enter_state() which allows tick_broadcast_exit() to be
called directly with interrupts disabled (as required), but
it also adds a fallback branch reflecting the 4.0 and earlier
behavior for idle states that enable interrupts on exit
from their ->enter callbacks.

I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
set for such states, but people may try to add stuff like that in the
future, so it's better to catch that (hence the WARN_ON_ONCE) and do
our best to handle it gracefully anyway, IMO.

The "if (entered_state == -EBUSY)" check is conservative.  It may
be better to do "if (entered_state < 0)" and fall back to the default
on all errors, but that's not what we do today (I guess the concern
would be "what if the state ->enter returns an error after entering
and exiting the idle state, in which case we may miss a wakeup event
if we fall back to the default").


Actually, if my understanding of things is correct (the local clock event
device cannot go away from under code executed with interrupts disabled
on the local CPU), the simplified one below should be sufficient.


Acked-by: Daniel Lezcano 


---
  drivers/cpuidle/cpuidle.c |   16 
  kernel/sched/idle.c   |   16 ++--
  2 files changed, 18 insertions(+), 14 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int next_state, entered_state;
-   unsigned int broadcast;
bool reflect;

/*
@@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
goto exit_idle;
}

-   broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
-
-   /*
-* Tell the time framework to switch to a broadcast timer
-* because our local timer will be shutdown. If a local timer
-* is used from another cpu as a broadcast timer, this call may
-* fail if it is not available
-*/
-   if (broadcast && tick_broadcast_enter())
-   goto use_default;
-
/* Take note of the planned idle state. */
idle_set_state(this_rq(), >states[next_state]);

@@ -174,8 +162,8 @@ static void cpuidle_idle_call(void)
/* The cpu is no longer idle or about to enter idle. */
idle_set_state(this_rq(), NULL);

-   if 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Peter Zijlstra]

On Wed, Apr 29, 2015 at 03:04:47AM +0200, Rafael J. Wysocki wrote:
> > Below is the patch I came up with in the meantime.
> > 
> > This moves the "switch to broadcast" timer logic into
> > cpuidle_enter_state() which allows tick_broadcast_exit() to be
> > called directly with interrupts disabled (as required), but
> > it also adds a fallback branch reflecting the 4.0 and earlier
> > behavior for idle states that enable interrupts on exit
> > from their ->enter callbacks.
> > 
> > I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
> > set for such states, but people may try to add stuff like that in the
> > future, so it's better to catch that (hence the WARN_ON_ONCE) and do
> > our best to handle it gracefully anyway, IMO.
> > 
> > The "if (entered_state == -EBUSY)" check is conservative.  It may
> > be better to do "if (entered_state < 0)" and fall back to the default
> > on all errors, but that's not what we do today (I guess the concern
> > would be "what if the state ->enter returns an error after entering
> > and exiting the idle state, in which case we may miss a wakeup event
> > if we fall back to the default").
> 
> Actually, if my understanding of things is correct (the local clock event
> device cannot go away from under code executed with interrupts disabled
> on the local CPU), the simplified one below should be sufficient.

Afaict both tick_broadcast_{enter,exit}() end up calling
tick_broadcast_oneshot_control() which is serialized with the
tick_broadcast_lock.

But yes, the local device is strictly managed from the local cpu, so any
other CPU wanting to muck with it would have to IPI and therefore
disabling the local IRQs would make it safe.

Acked-by: Peter Zijlstra (Intel) 

> ---
>  drivers/cpuidle/cpuidle.c |   16 
>  kernel/sched/idle.c   |   16 ++--
>  2 files changed, 18 insertions(+), 14 deletions(-)
> 
> Index: linux-pm/kernel/sched/idle.c
> ===
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
>   struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>   struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>   int next_state, entered_state;
> - unsigned int broadcast;
>   bool reflect;
>  
>   /*
> @@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
>   goto exit_idle;
>   }
>  
> - broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
> -
> - /*
> -  * Tell the time framework to switch to a broadcast timer
> -  * because our local timer will be shutdown. If a local timer
> -  * is used from another cpu as a broadcast timer, this call may
> -  * fail if it is not available
> -  */
> - if (broadcast && tick_broadcast_enter())
> - goto use_default;
> -
>   /* Take note of the planned idle state. */
>   idle_set_state(this_rq(), >states[next_state]);
>  
> @@ -174,8 +162,8 @@ static void cpuidle_idle_call(void)
>   /* The cpu is no longer idle or about to enter idle. */
>   idle_set_state(this_rq(), NULL);
>  
> - if (broadcast)
> - tick_broadcast_exit();
> + if (entered_state == -EBUSY)
> + goto use_default;
>  
>   /*
>* Give the governor an opportunity to reflect on the outcome
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -158,9 +158,18 @@ int cpuidle_enter_state(struct cpuidle_d
>   int entered_state;
>  
>   struct cpuidle_state *target_state = >states[index];
> + bool broadcast = !!(target_state->flags & CPUIDLE_FLAG_TIMER_STOP);
>   ktime_t time_start, time_end;
>   s64 diff;
>  
> + /*
> +  * Tell the time framework to switch to a broadcast timer because our
> +  * local timer will be shut down.  If a local timer is used from another
> +  * CPU as a broadcast timer, this call may fail if it is not available.
> +  */
> + if (broadcast && tick_broadcast_enter())
> + return -EBUSY;
> +
>   trace_cpu_idle_rcuidle(index, dev->cpu);
>   time_start = ktime_get();
>  
> @@ -169,6 +178,13 @@ int cpuidle_enter_state(struct cpuidle_d
>   time_end = ktime_get();
>   trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
>  
> + if (broadcast) {
> + if (WARN_ON_ONCE(!irqs_disabled()))
> + local_irq_disable();
> +
> + tick_broadcast_exit();
> + }
> +
>   if (!cpuidle_state_is_coupled(dev, drv, entered_state))
>   local_irq_enable();
>  
> 
--
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

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Linus Walleij]

On Wed, Apr 29, 2015 at 3:04 AM, Rafael J. Wysocki  wrote:
> On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:

>> Below is the patch I came up with in the meantime.
>>
>> This moves the "switch to broadcast" timer logic into
>> cpuidle_enter_state() which allows tick_broadcast_exit() to be
>> called directly with interrupts disabled (as required), but
>> it also adds a fallback branch reflecting the 4.0 and earlier
>> behavior for idle states that enable interrupts on exit
>> from their ->enter callbacks.
>>
>> I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
>> set for such states, but people may try to add stuff like that in the
>> future, so it's better to catch that (hence the WARN_ON_ONCE) and do
>> our best to handle it gracefully anyway, IMO.
>>
>> The "if (entered_state == -EBUSY)" check is conservative.  It may
>> be better to do "if (entered_state < 0)" and fall back to the default
>> on all errors, but that's not what we do today (I guess the concern
>> would be "what if the state ->enter returns an error after entering
>> and exiting the idle state, in which case we may miss a wakeup event
>> if we fall back to the default").
>
> Actually, if my understanding of things is correct (the local clock event
> device cannot go away from under code executed with interrupts disabled
> on the local CPU), the simplified one below should be sufficient.

Rock solid, after this patch the issues are gone and system stable.
Using it as stabilizer base for all v4.2 work now.

Excellent work Rafael!

Tested-by: Linus Walleij 

Yours,
Linus Walleij
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

Hi Rafael,

On 29/04/15 02:04, Rafael J. Wysocki wrote:

On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:


[...]



Below is the patch I came up with in the meantime.

This moves the switch to broadcast timer logic into
cpuidle_enter_state() which allows tick_broadcast_exit() to be
called directly with interrupts disabled (as required), but
it also adds a fallback branch reflecting the 4.0 and earlier
behavior for idle states that enable interrupts on exit
from their -enter callbacks.

I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
set for such states, but people may try to add stuff like that in the
future, so it's better to catch that (hence the WARN_ON_ONCE) and do
our best to handle it gracefully anyway, IMO.

The if (entered_state == -EBUSY) check is conservative.  It may
be better to do if (entered_state  0) and fall back to the default
on all errors, but that's not what we do today (I guess the concern
would be what if the state -enter returns an error after entering
and exiting the idle state, in which case we may miss a wakeup event
if we fall back to the default).


Actually, if my understanding of things is correct (the local clock event
device cannot go away from under code executed with interrupts disabled
on the local CPU), the simplified one below should be sufficient.



Tested-by: Sudeep Holla sudeep.ho...@arm.com

Regards,
Sudeep
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Linus Walleij]

On Wed, Apr 29, 2015 at 3:04 AM, Rafael J. Wysocki r...@rjwysocki.net wrote:
 On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:

 Below is the patch I came up with in the meantime.

 This moves the switch to broadcast timer logic into
 cpuidle_enter_state() which allows tick_broadcast_exit() to be
 called directly with interrupts disabled (as required), but
 it also adds a fallback branch reflecting the 4.0 and earlier
 behavior for idle states that enable interrupts on exit
 from their -enter callbacks.

 I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
 set for such states, but people may try to add stuff like that in the
 future, so it's better to catch that (hence the WARN_ON_ONCE) and do
 our best to handle it gracefully anyway, IMO.

 The if (entered_state == -EBUSY) check is conservative.  It may
 be better to do if (entered_state  0) and fall back to the default
 on all errors, but that's not what we do today (I guess the concern
 would be what if the state -enter returns an error after entering
 and exiting the idle state, in which case we may miss a wakeup event
 if we fall back to the default).

 Actually, if my understanding of things is correct (the local clock event
 device cannot go away from under code executed with interrupts disabled
 on the local CPU), the simplified one below should be sufficient.

Rock solid, after this patch the issues are gone and system stable.
Using it as stabilizer base for all v4.2 work now.

Excellent work Rafael!

Tested-by: Linus Walleij linus.wall...@linaro.org

Yours,
Linus Walleij
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Peter Zijlstra]

On Wed, Apr 29, 2015 at 03:04:47AM +0200, Rafael J. Wysocki wrote:
  Below is the patch I came up with in the meantime.
  
  This moves the switch to broadcast timer logic into
  cpuidle_enter_state() which allows tick_broadcast_exit() to be
  called directly with interrupts disabled (as required), but
  it also adds a fallback branch reflecting the 4.0 and earlier
  behavior for idle states that enable interrupts on exit
  from their -enter callbacks.
  
  I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
  set for such states, but people may try to add stuff like that in the
  future, so it's better to catch that (hence the WARN_ON_ONCE) and do
  our best to handle it gracefully anyway, IMO.
  
  The if (entered_state == -EBUSY) check is conservative.  It may
  be better to do if (entered_state  0) and fall back to the default
  on all errors, but that's not what we do today (I guess the concern
  would be what if the state -enter returns an error after entering
  and exiting the idle state, in which case we may miss a wakeup event
  if we fall back to the default).
 
 Actually, if my understanding of things is correct (the local clock event
 device cannot go away from under code executed with interrupts disabled
 on the local CPU), the simplified one below should be sufficient.

Afaict both tick_broadcast_{enter,exit}() end up calling
tick_broadcast_oneshot_control() which is serialized with the
tick_broadcast_lock.

But yes, the local device is strictly managed from the local cpu, so any
other CPU wanting to muck with it would have to IPI and therefore
disabling the local IRQs would make it safe.

Acked-by: Peter Zijlstra (Intel) pet...@infradead.org

 ---
  drivers/cpuidle/cpuidle.c |   16 
  kernel/sched/idle.c   |   16 ++--
  2 files changed, 18 insertions(+), 14 deletions(-)
 
 Index: linux-pm/kernel/sched/idle.c
 ===
 --- linux-pm.orig/kernel/sched/idle.c
 +++ linux-pm/kernel/sched/idle.c
 @@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
   struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
   struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
   int next_state, entered_state;
 - unsigned int broadcast;
   bool reflect;
  
   /*
 @@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
   goto exit_idle;
   }
  
 - broadcast = drv-states[next_state].flags  CPUIDLE_FLAG_TIMER_STOP;
 -
 - /*
 -  * Tell the time framework to switch to a broadcast timer
 -  * because our local timer will be shutdown. If a local timer
 -  * is used from another cpu as a broadcast timer, this call may
 -  * fail if it is not available
 -  */
 - if (broadcast  tick_broadcast_enter())
 - goto use_default;
 -
   /* Take note of the planned idle state. */
   idle_set_state(this_rq(), drv-states[next_state]);
  
 @@ -174,8 +162,8 @@ static void cpuidle_idle_call(void)
   /* The cpu is no longer idle or about to enter idle. */
   idle_set_state(this_rq(), NULL);
  
 - if (broadcast)
 - tick_broadcast_exit();
 + if (entered_state == -EBUSY)
 + goto use_default;
  
   /*
* Give the governor an opportunity to reflect on the outcome
 Index: linux-pm/drivers/cpuidle/cpuidle.c
 ===
 --- linux-pm.orig/drivers/cpuidle/cpuidle.c
 +++ linux-pm/drivers/cpuidle/cpuidle.c
 @@ -158,9 +158,18 @@ int cpuidle_enter_state(struct cpuidle_d
   int entered_state;
  
   struct cpuidle_state *target_state = drv-states[index];
 + bool broadcast = !!(target_state-flags  CPUIDLE_FLAG_TIMER_STOP);
   ktime_t time_start, time_end;
   s64 diff;
  
 + /*
 +  * Tell the time framework to switch to a broadcast timer because our
 +  * local timer will be shut down.  If a local timer is used from another
 +  * CPU as a broadcast timer, this call may fail if it is not available.
 +  */
 + if (broadcast  tick_broadcast_enter())
 + return -EBUSY;
 +
   trace_cpu_idle_rcuidle(index, dev-cpu);
   time_start = ktime_get();
  
 @@ -169,6 +178,13 @@ int cpuidle_enter_state(struct cpuidle_d
   time_end = ktime_get();
   trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev-cpu);
  
 + if (broadcast) {
 + if (WARN_ON_ONCE(!irqs_disabled()))
 + local_irq_disable();
 +
 + tick_broadcast_exit();
 + }
 +
   if (!cpuidle_state_is_coupled(dev, drv, entered_state))
   local_irq_enable();
  
 
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Daniel Lezcano]

On 04/29/2015 03:04 AM, Rafael J. Wysocki wrote:

On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:


On 28/04/15 15:14, Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:

On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org wrote:

Sudeep:

At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.


So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.


Aha that looks very much like it. Put me on the patch and I'll
take it for a spin.


OK, so something like the below for starters (the _irqsave variant is used to
avoid adding one more WARN_ON(irqs_disabled()) in there).

I haven't tested it, but then I can't reproduce the original issue in the
first place.


Of course, the whole broadcast thing could be done from cpuidle_enter()
in the first place, but then we could not avoid the problem with the cpuidle
*callback* enabling interrupts possibly in there anyway (not to mention the
coupled stuff).


That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I really
wouldn't expect it to re-enable interrupts on exit and the coupled thing
seems to be fundamentally at odds with that flag either.

So it should be possible to move the broadcast logic into the cpuidle layer,
which I'm going to try to do.



Makes sense.


Please test the patch I've sent, though, as it should bring the code back to
where it was before the clockevents_notify() removal and it'd be good to verify
that.



I tested your patch and it works now. Anyways I am continuing to run
stress tests on my board. I will report if I find any issues.


Great, thanks!

Below is the patch I came up with in the meantime.

This moves the switch to broadcast timer logic into
cpuidle_enter_state() which allows tick_broadcast_exit() to be
called directly with interrupts disabled (as required), but
it also adds a fallback branch reflecting the 4.0 and earlier
behavior for idle states that enable interrupts on exit
from their -enter callbacks.

I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
set for such states, but people may try to add stuff like that in the
future, so it's better to catch that (hence the WARN_ON_ONCE) and do
our best to handle it gracefully anyway, IMO.

The if (entered_state == -EBUSY) check is conservative.  It may
be better to do if (entered_state  0) and fall back to the default
on all errors, but that's not what we do today (I guess the concern
would be what if the state -enter returns an error after entering
and exiting the idle state, in which case we may miss a wakeup event
if we fall back to the default).


Actually, if my understanding of things is correct (the local clock event
device cannot go away from under code executed with interrupts disabled
on the local CPU), the simplified one below should be sufficient.


Acked-by: Daniel Lezcano daniel.lezc...@linaro.org


---
  drivers/cpuidle/cpuidle.c |   16 
  kernel/sched/idle.c   |   16 ++--
  2 files changed, 18 insertions(+), 14 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int next_state, entered_state;
-   unsigned int broadcast;
bool reflect;

/*
@@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
goto exit_idle;
}

-   broadcast = drv-states[next_state].flags  CPUIDLE_FLAG_TIMER_STOP;
-
-   /*
-* Tell the time framework to switch to a broadcast timer
-* because our local timer will be shutdown. If a local timer
-* is used from another cpu as a broadcast timer, this call may
-* fail if it is not available
-*/
-   if (broadcast  tick_broadcast_enter())
-   goto use_default;
-
/* Take note of the planned idle state. */
idle_set_state(this_rq(), drv-states[next_state]);

@@ -174,8 +162,8 @@ static void cpuidle_idle_call(void)
/* The cpu is no longer idle or about to enter idle. */
idle_set_state(this_rq(), 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Preeti U Murthy]

On 04/29/2015 06:34 AM, Rafael J. Wysocki wrote:
 On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:

 On 28/04/15 15:14, Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
 On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org 
 wrote:
 Sudeep:
 At-least I observed issue only when I am using hardware broadcast 
 timer.
 It doesn't hang when I am using hrtimer as broadcast timer in which 
 case
 one of the cpu will be not enter deeper idle states that lose timer.
 I will rerun on v4.1-rc1 and post the complete log.

 So the bug here is that cpuidle_enter() enables interrupts, so the
 assumption about them being not enabled made by
 tick_broadcast_oneshot_control() is actually not valid.

 It looks like we need to acquire the clockevents_lock at least in this
 particular case.  Let me see where to put it and I'll send a patch for
 testing.

 Aha that looks very much like it. Put me on the patch and I'll
 take it for a spin.

 OK, so something like the below for starters (the _irqsave variant is 
 used to
 avoid adding one more WARN_ON(irqs_disabled()) in there).

 I haven't tested it, but then I can't reproduce the original issue in the
 first place.

 Of course, the whole broadcast thing could be done from cpuidle_enter()
 in the first place, but then we could not avoid the problem with the 
 cpuidle
 *callback* enabling interrupts possibly in there anyway (not to mention 
 the
 coupled stuff).

 That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I 
 really
 wouldn't expect it to re-enable interrupts on exit and the coupled thing
 seems to be fundamentally at odds with that flag either.

 So it should be possible to move the broadcast logic into the cpuidle 
 layer,
 which I'm going to try to do.


 Makes sense.

 Please test the patch I've sent, though, as it should bring the code back 
 to
 where it was before the clockevents_notify() removal and it'd be good to 
 verify
 that.


 I tested your patch and it works now. Anyways I am continuing to run
 stress tests on my board. I will report if I find any issues.

 Great, thanks!

 Below is the patch I came up with in the meantime.

 This moves the switch to broadcast timer logic into
 cpuidle_enter_state() which allows tick_broadcast_exit() to be
 called directly with interrupts disabled (as required), but
 it also adds a fallback branch reflecting the 4.0 and earlier
 behavior for idle states that enable interrupts on exit
 from their -enter callbacks.

 I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
 set for such states, but people may try to add stuff like that in the
 future, so it's better to catch that (hence the WARN_ON_ONCE) and do
 our best to handle it gracefully anyway, IMO.

 The if (entered_state == -EBUSY) check is conservative.  It may
 be better to do if (entered_state  0) and fall back to the default
 on all errors, but that's not what we do today (I guess the concern
 would be what if the state -enter returns an error after entering
 and exiting the idle state, in which case we may miss a wakeup event
 if we fall back to the default).
 
 Actually, if my understanding of things is correct (the local clock event
 device cannot go away from under code executed with interrupts disabled
 on the local CPU), the simplified one below should be sufficient.
 
 ---
  drivers/cpuidle/cpuidle.c |   16 
  kernel/sched/idle.c   |   16 ++--
  2 files changed, 18 insertions(+), 14 deletions(-)
 
 Index: linux-pm/kernel/sched/idle.c
 ===
 --- linux-pm.orig/kernel/sched/idle.c
 +++ linux-pm/kernel/sched/idle.c
 @@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
   struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
   struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
   int next_state, entered_state;
 - unsigned int broadcast;
   bool reflect;
 
   /*
 @@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
   goto exit_idle;
   }
 
 - broadcast = drv-states[next_state].flags  CPUIDLE_FLAG_TIMER_STOP;
 -
 - /*
 -  * Tell the time framework to switch to a broadcast timer
 -  * because our local timer will be shutdown. If a local timer
 -  * is used from another cpu as a broadcast timer, this call may
 -  * fail if it is not available
 -  */
 - if (broadcast  tick_broadcast_enter())
 - goto use_default;
 -
   /* Take note of the planned idle state. */
   idle_set_state(this_rq(), drv-states[next_state]);
 
 @@ -174,8 +162,8 @@ static void cpuidle_idle_call(void)
   /* The cpu is no longer idle or about to enter idle. */
   idle_set_state(this_rq(), 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:
> On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:
> > 
> > On 28/04/15 15:14, Rafael J. Wysocki wrote:
> > > On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
> > >> On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
> > >>> On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
> >  On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  
> >  wrote:
> > > Sudeep:
> > >> At-least I observed issue only when I am using hardware broadcast 
> > >> timer.
> > >> It doesn't hang when I am using hrtimer as broadcast timer in which 
> > >> case
> > >> one of the cpu will be not enter deeper idle states that lose timer.
> > >> I will rerun on v4.1-rc1 and post the complete log.
> > >
> > > So the bug here is that cpuidle_enter() enables interrupts, so the
> > > assumption about them being not enabled made by
> > > tick_broadcast_oneshot_control() is actually not valid.
> > >
> > > It looks like we need to acquire the clockevents_lock at least in this
> > > particular case.  Let me see where to put it and I'll send a patch for
> > > testing.
> > 
> >  Aha that looks very much like it. Put me on the patch and I'll
> >  take it for a spin.
> > >>>
> > >>> OK, so something like the below for starters (the _irqsave variant is 
> > >>> used to
> > >>> avoid adding one more WARN_ON(irqs_disabled()) in there).
> > >>>
> > >>> I haven't tested it, but then I can't reproduce the original issue in 
> > >>> the
> > >>> first place.
> > >>
> > >> Of course, the whole "broadcast" thing could be done from cpuidle_enter()
> > >> in the first place, but then we could not avoid the problem with the 
> > >> cpuidle
> > >> *callback* enabling interrupts possibly in there anyway (not to mention 
> > >> the
> > >> "coupled" stuff).
> > >
> > > That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I 
> > > really
> > > wouldn't expect it to re-enable interrupts on exit and the "coupled" thing
> > > seems to be fundamentally at odds with that flag either.
> > >
> > > So it should be possible to move the "broadcast" logic into the cpuidle 
> > > layer,
> > > which I'm going to try to do.
> > >
> > 
> > Makes sense.
> > 
> > > Please test the patch I've sent, though, as it should bring the code back 
> > > to
> > > where it was before the clockevents_notify() removal and it'd be good to 
> > > verify
> > > that.
> > >
> > 
> > I tested your patch and it works now. Anyways I am continuing to run
> > stress tests on my board. I will report if I find any issues.
> 
> Great, thanks!
> 
> Below is the patch I came up with in the meantime.
> 
> This moves the "switch to broadcast" timer logic into
> cpuidle_enter_state() which allows tick_broadcast_exit() to be
> called directly with interrupts disabled (as required), but
> it also adds a fallback branch reflecting the 4.0 and earlier
> behavior for idle states that enable interrupts on exit
> from their ->enter callbacks.
> 
> I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
> set for such states, but people may try to add stuff like that in the
> future, so it's better to catch that (hence the WARN_ON_ONCE) and do
> our best to handle it gracefully anyway, IMO.
> 
> The "if (entered_state == -EBUSY)" check is conservative.  It may
> be better to do "if (entered_state < 0)" and fall back to the default
> on all errors, but that's not what we do today (I guess the concern
> would be "what if the state ->enter returns an error after entering
> and exiting the idle state, in which case we may miss a wakeup event
> if we fall back to the default").

Actually, if my understanding of things is correct (the local clock event
device cannot go away from under code executed with interrupts disabled
on the local CPU), the simplified one below should be sufficient.

---
 drivers/cpuidle/cpuidle.c |   16 
 kernel/sched/idle.c   |   16 ++--
 2 files changed, 18 insertions(+), 14 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int next_state, entered_state;
-   unsigned int broadcast;
bool reflect;
 
/*
@@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
goto exit_idle;
}
 
-   broadcast = drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP;
-
-   /*
-* Tell the time framework to switch to a broadcast timer
-* because our local timer will be shutdown. If a local timer
-* is used from another cpu as a broadcast timer, this call may
-* 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:
> 
> On 28/04/15 15:14, Rafael J. Wysocki wrote:
> > On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
> >> On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
> >>> On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
>  On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  
>  wrote:
> > Sudeep:
> >> At-least I observed issue only when I am using hardware broadcast 
> >> timer.
> >> It doesn't hang when I am using hrtimer as broadcast timer in which 
> >> case
> >> one of the cpu will be not enter deeper idle states that lose timer.
> >> I will rerun on v4.1-rc1 and post the complete log.
> >
> > So the bug here is that cpuidle_enter() enables interrupts, so the
> > assumption about them being not enabled made by
> > tick_broadcast_oneshot_control() is actually not valid.
> >
> > It looks like we need to acquire the clockevents_lock at least in this
> > particular case.  Let me see where to put it and I'll send a patch for
> > testing.
> 
>  Aha that looks very much like it. Put me on the patch and I'll
>  take it for a spin.
> >>>
> >>> OK, so something like the below for starters (the _irqsave variant is 
> >>> used to
> >>> avoid adding one more WARN_ON(irqs_disabled()) in there).
> >>>
> >>> I haven't tested it, but then I can't reproduce the original issue in the
> >>> first place.
> >>
> >> Of course, the whole "broadcast" thing could be done from cpuidle_enter()
> >> in the first place, but then we could not avoid the problem with the 
> >> cpuidle
> >> *callback* enabling interrupts possibly in there anyway (not to mention the
> >> "coupled" stuff).
> >
> > That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I 
> > really
> > wouldn't expect it to re-enable interrupts on exit and the "coupled" thing
> > seems to be fundamentally at odds with that flag either.
> >
> > So it should be possible to move the "broadcast" logic into the cpuidle 
> > layer,
> > which I'm going to try to do.
> >
> 
> Makes sense.
> 
> > Please test the patch I've sent, though, as it should bring the code back to
> > where it was before the clockevents_notify() removal and it'd be good to 
> > verify
> > that.
> >
> 
> I tested your patch and it works now. Anyways I am continuing to run
> stress tests on my board. I will report if I find any issues.

Great, thanks!

Below is the patch I came up with in the meantime.

This moves the "switch to broadcast" timer logic into
cpuidle_enter_state() which allows tick_broadcast_exit() to be
called directly with interrupts disabled (as required), but
it also adds a fallback branch reflecting the 4.0 and earlier
behavior for idle states that enable interrupts on exit
from their ->enter callbacks.

I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
set for such states, but people may try to add stuff like that in the
future, so it's better to catch that (hence the WARN_ON_ONCE) and do
our best to handle it gracefully anyway, IMO.

The "if (entered_state == -EBUSY)" check is conservative.  It may
be better to do "if (entered_state < 0)" and fall back to the default
on all errors, but that's not what we do today (I guess the concern
would be "what if the state ->enter returns an error after entering
and exiting the idle state, in which case we may miss a wakeup event
if we fall back to the default").

---
 drivers/cpuidle/cpuidle.c  |   16 
 include/linux/clockchips.h |2 ++
 kernel/sched/idle.c|   16 ++--
 kernel/time/clockevents.c  |   13 +
 4 files changed, 33 insertions(+), 14 deletions(-)

Index: linux-pm/include/linux/clockchips.h
===
--- linux-pm.orig/include/linux/clockchips.h
+++ linux-pm/include/linux/clockchips.h
@@ -198,9 +198,11 @@ extern int tick_receive_broadcast(void);
 # if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && 
defined(CONFIG_TICK_ONESHOT)
 extern void tick_setup_hrtimer_broadcast(void);
 extern int tick_check_broadcast_expired(void);
+extern void tick_broadcast_exit_idle_fallback(void);
 # else
 static inline int tick_check_broadcast_expired(void) { return 0; }
 static inline void tick_setup_hrtimer_broadcast(void) { }
+static inline void tick_broadcast_exit_idle_fallback(void) { }
 # endif
 
 extern int clockevents_notify(unsigned long reason, void *arg);
Index: linux-pm/kernel/time/clockevents.c
===
--- linux-pm.orig/kernel/time/clockevents.c
+++ linux-pm/kernel/time/clockevents.c
@@ -735,6 +735,19 @@ static ssize_t sysfs_unbind_tick_dev(str
 static DEVICE_ATTR(unbind_device, 0200, NULL, sysfs_unbind_tick_dev);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_broadcast_exit_idle_fallback - Fallback broadcast oneshot mode exit.
+ *
+ * Called from within the CPU 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

On 28/04/15 15:14, Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:

On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  wrote:

Sudeep:

At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.


So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.


Aha that looks very much like it. Put me on the patch and I'll
take it for a spin.


OK, so something like the below for starters (the _irqsave variant is used to
avoid adding one more WARN_ON(irqs_disabled()) in there).

I haven't tested it, but then I can't reproduce the original issue in the
first place.


Of course, the whole "broadcast" thing could be done from cpuidle_enter()
in the first place, but then we could not avoid the problem with the cpuidle
*callback* enabling interrupts possibly in there anyway (not to mention the
"coupled" stuff).


That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I really
wouldn't expect it to re-enable interrupts on exit and the "coupled" thing
seems to be fundamentally at odds with that flag either.

So it should be possible to move the "broadcast" logic into the cpuidle layer,
which I'm going to try to do.



Makes sense.


Please test the patch I've sent, though, as it should bring the code back to
where it was before the clockevents_notify() removal and it'd be good to verify
that.



I tested your patch and it works now. Anyways I am continuing to run
stress tests on my board. I will report if I find any issues.

Regards,
Sudeep
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
> On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
> > On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
> > > On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  
> > > wrote:
> > > > Sudeep:
> > > >> At-least I observed issue only when I am using hardware broadcast 
> > > >> timer.
> > > >> It doesn't hang when I am using hrtimer as broadcast timer in which 
> > > >> case
> > > >> one of the cpu will be not enter deeper idle states that lose timer.
> > > >> I will rerun on v4.1-rc1 and post the complete log.
> > > >
> > > > So the bug here is that cpuidle_enter() enables interrupts, so the
> > > > assumption about them being not enabled made by
> > > > tick_broadcast_oneshot_control() is actually not valid.
> > > >
> > > > It looks like we need to acquire the clockevents_lock at least in this
> > > > particular case.  Let me see where to put it and I'll send a patch for
> > > > testing.
> > > 
> > > Aha that looks very much like it. Put me on the patch and I'll
> > > take it for a spin.
> > 
> > OK, so something like the below for starters (the _irqsave variant is used 
> > to
> > avoid adding one more WARN_ON(irqs_disabled()) in there).
> > 
> > I haven't tested it, but then I can't reproduce the original issue in the
> > first place.
> 
> Of course, the whole "broadcast" thing could be done from cpuidle_enter()
> in the first place, but then we could not avoid the problem with the cpuidle
> *callback* enabling interrupts possibly in there anyway (not to mention the
> "coupled" stuff).

That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I really
wouldn't expect it to re-enable interrupts on exit and the "coupled" thing
seems to be fundamentally at odds with that flag either.

So it should be possible to move the "broadcast" logic into the cpuidle layer,
which I'm going to try to do.

Please test the patch I've sent, though, as it should bring the code back to
where it was before the clockevents_notify() removal and it'd be good to verify
that.

Rafael

--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
> On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
> > On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  
> > wrote:
> > > Sudeep:
> > >> At-least I observed issue only when I am using hardware broadcast timer.
> > >> It doesn't hang when I am using hrtimer as broadcast timer in which case
> > >> one of the cpu will be not enter deeper idle states that lose timer.
> > >> I will rerun on v4.1-rc1 and post the complete log.
> > >
> > > So the bug here is that cpuidle_enter() enables interrupts, so the
> > > assumption about them being not enabled made by
> > > tick_broadcast_oneshot_control() is actually not valid.
> > >
> > > It looks like we need to acquire the clockevents_lock at least in this
> > > particular case.  Let me see where to put it and I'll send a patch for
> > > testing.
> > 
> > Aha that looks very much like it. Put me on the patch and I'll
> > take it for a spin.
> 
> OK, so something like the below for starters (the _irqsave variant is used to
> avoid adding one more WARN_ON(irqs_disabled()) in there).
> 
> I haven't tested it, but then I can't reproduce the original issue in the
> first place.

Of course, the whole "broadcast" thing could be done from cpuidle_enter()
in the first place, but then we could not avoid the problem with the cpuidle
*callback* enabling interrupts possibly in there anyway (not to mention the
"coupled" stuff).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
> On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  wrote:
> > Sudeep:
> >> At-least I observed issue only when I am using hardware broadcast timer.
> >> It doesn't hang when I am using hrtimer as broadcast timer in which case
> >> one of the cpu will be not enter deeper idle states that lose timer.
> >> I will rerun on v4.1-rc1 and post the complete log.
> >
> > So the bug here is that cpuidle_enter() enables interrupts, so the
> > assumption about them being not enabled made by
> > tick_broadcast_oneshot_control() is actually not valid.
> >
> > It looks like we need to acquire the clockevents_lock at least in this
> > particular case.  Let me see where to put it and I'll send a patch for
> > testing.
> 
> Aha that looks very much like it. Put me on the patch and I'll
> take it for a spin.

OK, so something like the below for starters (the _irqsave variant is used to
avoid adding one more WARN_ON(irqs_disabled()) in there).

I haven't tested it, but then I can't reproduce the original issue in the
first place.

---
 include/linux/clockchips.h |2 ++
 kernel/sched/idle.c|2 +-
 kernel/time/clockevents.c  |   15 +++
 3 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-pm/include/linux/clockchips.h
===
--- linux-pm.orig/include/linux/clockchips.h
+++ linux-pm/include/linux/clockchips.h
@@ -198,9 +198,11 @@ extern int tick_receive_broadcast(void);
 # if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST) && 
defined(CONFIG_TICK_ONESHOT)
 extern void tick_setup_hrtimer_broadcast(void);
 extern int tick_check_broadcast_expired(void);
+extern void tick_broadcast_exit_idle(void);
 # else
 static inline int tick_check_broadcast_expired(void) { return 0; }
 static inline void tick_setup_hrtimer_broadcast(void) { }
+static inline void tick_broadcast_exit_idle(void) { }
 # endif
 
 extern int clockevents_notify(unsigned long reason, void *arg);
Index: linux-pm/kernel/time/clockevents.c
===
--- linux-pm.orig/kernel/time/clockevents.c
+++ linux-pm/kernel/time/clockevents.c
@@ -735,6 +735,21 @@ static ssize_t sysfs_unbind_tick_dev(str
 static DEVICE_ATTR(unbind_device, 0200, NULL, sysfs_unbind_tick_dev);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_broadcast_exit_idle - Broadcast oneshot mode exit for the idle loop.
+ *
+ * The idle loop exits the broadcast oneshot mode with interrupts enabled, so
+ * clockevents_lock needs to be acquired around that.
+ */
+void tick_broadcast_exit_idle(void)
+{
+   unsigned long flags;
+
+   raw_spin_lock_irqsave(_lock, flags);
+   tick_broadcast_exit();
+   raw_spin_unlock_irqrestore(_lock, flags);
+}
+
 static struct device tick_bc_dev = {
.init_name  = "broadcast",
.id = 0,
Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -175,7 +175,7 @@ static void cpuidle_idle_call(void)
idle_set_state(this_rq(), NULL);
 
if (broadcast)
-   tick_broadcast_exit();
+   tick_broadcast_exit_idle();
 
/*
 * Give the governor an opportunity to reflect on the outcome

--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

Hi Rafael,

On 28/04/15 13:19, Rafael J. Wysocki wrote:

On Tue, Apr 28, 2015 at 12:42 PM, Sudeep Holla  wrote:


[...]



At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.


So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.



Thanks for looking at this, I can also help in testing. Sending the 
complete log again with v4.1-rc1 if it's any useful.


Regards,
Sudeep

--->8

BUG: spinlock lockup suspected on CPU#0, swapper/0/0
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0

CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[] (unwind_backtrace) from [] (show_stack+0x11/0x14)
[] (show_stack) from [] (dump_stack+0x6d/0x78)
[] (dump_stack) from [] (do_raw_spin_lock+0xc3/0x144)
[] (do_raw_spin_lock) from [] 
(tick_handle_oneshot_broadcast+0x25/0x164)
[] (tick_handle_oneshot_broadcast) from [] 
(sp804_timer_interrupt+0x31/0x34)
[] (sp804_timer_interrupt) from [] 
(handle_irq_event_percpu+0x45/0x154)
[] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x2f/0x44)
[] (handle_irq_event) from [] 
(handle_fasteoi_irq+0x6f/0xf0)
[] (handle_fasteoi_irq) from [] 
(generic_handle_irq+0x23/0x2c)
[] (generic_handle_irq) from [] 
(__handle_domain_irq+0x45/0x84)
[] (__handle_domain_irq) from [] 
(gic_handle_irq+0x27/0x50)

[] (gic_handle_irq) from [] (__irq_svc+0x3f/0x64)
Exception stack(0xc07ebf08 to 0xc07ebf50)
bf00:    c1038814 0001 0001 c07f3f80 
ee78dec0
bf20:  c1038810  0001 c07ebf88 c0520fc8  
c07ebf50

bf40: c008151d c02e104e 4173 
[] (__irq_svc) from [] (_test_and_clear_bit+0x26/0x48)
[] (_test_and_clear_bit) from [] 
(tick_broadcast_oneshot_control+0x159/0x1ec)
[] (tick_broadcast_oneshot_control) from [] 
(cpu_startup_entry+0x2c3/0x2f8)
[] (cpu_startup_entry) from [] 
(start_kernel+0x327/0x330)

BUG: spinlock lockup suspected on CPU#3, swapper/3/0
BUG: spinlock lockup suspected on CPU#4, swapper/4/0
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0

CPU: 4 PID: 0 Comm: swapper/4 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[] (unwind_backtrace) from [] (show_stack+0x11/0x14)
[] (show_stack) from [] (dump_stack+0x6d/0x78)
BUG: spinlock lockup suspected on CPU#2, swapper/2/0
[] (dump_stack) from [] (do_raw_spin_lock+0xc3/0x144)
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0
[] (do_raw_spin_lock) from [] 
(tick_broadcast_oneshot_control+0x45/0x1ec)
[] (tick_broadcast_oneshot_control) from [] 
(cpu_startup_entry+0x2c3/0x2f8)

[] (cpu_startup_entry) from [<800093d1>] (0x800093d1)
CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[] (unwind_backtrace) from [] (show_stack+0x11/0x14)
[] (show_stack) from [] (dump_stack+0x6d/0x78)
[] (dump_stack) from [] (do_raw_spin_lock+0xc3/0x144)
[] (do_raw_spin_lock) from [] 
(tick_broadcast_oneshot_control+0x45/0x1ec)
[] (tick_broadcast_oneshot_control) from [] 
(cpu_startup_entry+0x2c3/0x2f8)

[] (cpu_startup_entry) from [<800093d1>] (0x800093d1)
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0

CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[] (unwind_backtrace) from [] (show_stack+0x11/0x14)
[] (show_stack) from [] (dump_stack+0x6d/0x78)
[] (dump_stack) from [] (do_raw_spin_lock+0xc3/0x144)
[] (do_raw_spin_lock) from [] 
(tick_broadcast_oneshot_control+0x45/0x1ec)
[] (tick_broadcast_oneshot_control) from [] 
(cpu_startup_entry+0x2c3/0x2f8)

[] (cpu_startup_entry) from [<800093d1>] (0x800093d1)





--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Linus Walleij]

On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki  wrote:
> Sudeep:
>> At-least I observed issue only when I am using hardware broadcast timer.
>> It doesn't hang when I am using hrtimer as broadcast timer in which case
>> one of the cpu will be not enter deeper idle states that lose timer.
>> I will rerun on v4.1-rc1 and post the complete log.
>
> So the bug here is that cpuidle_enter() enables interrupts, so the
> assumption about them being not enabled made by
> tick_broadcast_oneshot_control() is actually not valid.
>
> It looks like we need to acquire the clockevents_lock at least in this
> particular case.  Let me see where to put it and I'll send a patch for
> testing.

Aha that looks very much like it. Put me on the patch and I'll
take it for a spin.

Yours,
Linus Walleij
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tue, Apr 28, 2015 at 12:42 PM, Sudeep Holla  wrote:
>
>
> On 28/04/15 11:34, Daniel Lezcano wrote:
>>
>> On 04/28/2015 12:11 PM, Linus Walleij wrote:
>>>
>>> On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki
>>>  wrote:
>>>
 From: Thomas Gleixner 

 Replace the clockevents_notify() call with an explicit function
 call.

 Signed-off-by: Thomas Gleixner 
 Signed-off-by: Rafael J. Wysocki 
>>>
>>>
>>> For some reason this makes my Ux500 system arbitrarily hang,
>>> especially during boot. Bisected down to this commit. Since the
>>> entire changeset is removing the notifications altogether I can't
>>> just revert it.
>>>
>>> Disabling CONFIG_CPU_IDLE removes the problem.
>>>
>>> Tried registering a stub driver (I just #if 0 all the code in
>>> drivers/cpuidle/cpuidle-ux500.c) it still crashes.
>>>
>>> That makes me think something inside the cpuidle subsystem is
>>> locking up after this, but my other idea is that the timer may be
>>> involved in some way, like this is stressing the timer in some new
>>> yet untested way.
>>>
>>> Has anyone else seen problems with this or is it only ux500?
>>>
>>> I'm looking closer at it but feel a bit clueless...
>>
>>
>> If you keep the #if 0 and remove the CPUIDLE_FLAG_TIMER_STOP flag,
>> does it still crash ?
>>
>
> At-least I observed issue only when I am using hardware broadcast timer.
> It doesn't hang when I am using hrtimer as broadcast timer in which case
> one of the cpu will be not enter deeper idle states that lose timer.
> I will rerun on v4.1-rc1 and post the complete log.

So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.

Rafael
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

On 28/04/15 11:34, Daniel Lezcano wrote:

On 04/28/2015 12:11 PM, Linus Walleij wrote:

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki
 wrote:


From: Thomas Gleixner 

Replace the clockevents_notify() call with an explicit function
call.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Rafael J. Wysocki 


For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit. Since the
entire changeset is removing the notifications altogether I can't
just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem is
locking up after this, but my other idea is that the timer may be
involved in some way, like this is stressing the timer in some new
yet untested way.

Has anyone else seen problems with this or is it only ux500?

I'm looking closer at it but feel a bit clueless...


If you keep the #if 0 and remove the CPUIDLE_FLAG_TIMER_STOP flag,
does it still crash ?



At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.

Regards,
Sudeep
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Daniel Lezcano]

On 04/28/2015 12:11 PM, Linus Walleij wrote:

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki  wrote:


From: Thomas Gleixner 

Replace the clockevents_notify() call with an explicit function call.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Rafael J. Wysocki 


For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit.
Since the entire changeset is removing the notifications
altogether I can't just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem
is locking up after this, but my other idea is that the timer
may be involved in some way, like this is stressing the timer
in some new yet untested way.

Has anyone else seen problems with this or is it only
ux500?

I'm looking closer at it but feel a bit clueless...


If you keep the #if 0 and remove the CPUIDLE_FLAG_TIMER_STOP flag, does 
it still crash ?



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




--
  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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

On 28/04/15 11:11, Linus Walleij wrote:

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki  wrote:


From: Thomas Gleixner 

Replace the clockevents_notify() call with an explicit function call.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Rafael J. Wysocki 


For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit.
Since the entire changeset is removing the notifications
altogether I can't just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem
is locking up after this, but my other idea is that the timer
may be involved in some way, like this is stressing the timer
in some new yet untested way.

Has anyone else seen problems with this or is it only
ux500?

I'm looking closer at it but feel a bit clueless...



Yes I am too in similar situation with my Vexpress platform.
I did report with the lockdep logs[1] before v4.1-rc1.
I still see the issue with v4.1-rc1.

Regards,
Sudeep

[1] https://lkml.org/lkml/2015/4/23/329
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Linus Walleij]

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki  wrote:

> From: Thomas Gleixner 
>
> Replace the clockevents_notify() call with an explicit function call.
>
> Signed-off-by: Thomas Gleixner 
> Signed-off-by: Rafael J. Wysocki 

For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit.
Since the entire changeset is removing the notifications
altogether I can't just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem
is locking up after this, but my other idea is that the timer
may be involved in some way, like this is stressing the timer
in some new yet untested way.

Has anyone else seen problems with this or is it only
ux500?

I'm looking closer at it but feel a bit clueless...

Yours,
Linus Walleij
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

Hi Rafael,

On 28/04/15 13:19, Rafael J. Wysocki wrote:

On Tue, Apr 28, 2015 at 12:42 PM, Sudeep Holla sudeep.ho...@arm.com wrote:


[...]



At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.


So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.



Thanks for looking at this, I can also help in testing. Sending the 
complete log again with v4.1-rc1 if it's any useful.


Regards,
Sudeep

---8

BUG: spinlock lockup suspected on CPU#0, swapper/0/0
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0

CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[c0014b1d] (unwind_backtrace) from [c0011659] (show_stack+0x11/0x14)
[c0011659] (show_stack) from [c05192ed] (dump_stack+0x6d/0x78)
[c05192ed] (dump_stack) from [c005c897] (do_raw_spin_lock+0xc3/0x144)
[c005c897] (do_raw_spin_lock) from [c00811f9] 
(tick_handle_oneshot_broadcast+0x25/0x164)
[c00811f9] (tick_handle_oneshot_broadcast) from [c001dd21] 
(sp804_timer_interrupt+0x31/0x34)
[c001dd21] (sp804_timer_interrupt) from [c0067ee1] 
(handle_irq_event_percpu+0x45/0x154)
[c0067ee1] (handle_irq_event_percpu) from [c006801f] 
(handle_irq_event+0x2f/0x44)
[c006801f] (handle_irq_event) from [c0069edb] 
(handle_fasteoi_irq+0x6f/0xf0)
[c0069edb] (handle_fasteoi_irq) from [c00677c7] 
(generic_handle_irq+0x23/0x2c)
[c00677c7] (generic_handle_irq) from [c0067a0d] 
(__handle_domain_irq+0x45/0x84)
[c0067a0d] (__handle_domain_irq) from [c0009303] 
(gic_handle_irq+0x27/0x50)

[c0009303] (gic_handle_irq) from [c0011eff] (__irq_svc+0x3f/0x64)
Exception stack(0xc07ebf08 to 0xc07ebf50)
bf00:    c1038814 0001 0001 c07f3f80 
ee78dec0
bf20:  c1038810  0001 c07ebf88 c0520fc8  
c07ebf50

bf40: c008151d c02e104e 4173 
[c0011eff] (__irq_svc) from [c02e104e] (_test_and_clear_bit+0x26/0x48)
[c02e104e] (_test_and_clear_bit) from [c008151d] 
(tick_broadcast_oneshot_control+0x159/0x1ec)
[c008151d] (tick_broadcast_oneshot_control) from [c0050273] 
(cpu_startup_entry+0x2c3/0x2f8)
[c0050273] (cpu_startup_entry) from [c0782a47] 
(start_kernel+0x327/0x330)

BUG: spinlock lockup suspected on CPU#3, swapper/3/0
BUG: spinlock lockup suspected on CPU#4, swapper/4/0
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0

CPU: 4 PID: 0 Comm: swapper/4 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[c0014b1d] (unwind_backtrace) from [c0011659] (show_stack+0x11/0x14)
[c0011659] (show_stack) from [c05192ed] (dump_stack+0x6d/0x78)
BUG: spinlock lockup suspected on CPU#2, swapper/2/0
[c05192ed] (dump_stack) from [c005c897] (do_raw_spin_lock+0xc3/0x144)
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0
[c005c897] (do_raw_spin_lock) from [c0081409] 
(tick_broadcast_oneshot_control+0x45/0x1ec)
[c0081409] (tick_broadcast_oneshot_control) from [c0050273] 
(cpu_startup_entry+0x2c3/0x2f8)

[c0050273] (cpu_startup_entry) from [800093d1] (0x800093d1)
CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[c0014b1d] (unwind_backtrace) from [c0011659] (show_stack+0x11/0x14)
[c0011659] (show_stack) from [c05192ed] (dump_stack+0x6d/0x78)
[c05192ed] (dump_stack) from [c005c897] (do_raw_spin_lock+0xc3/0x144)
[c005c897] (do_raw_spin_lock) from [c0081409] 
(tick_broadcast_oneshot_control+0x45/0x1ec)
[c0081409] (tick_broadcast_oneshot_control) from [c0050273] 
(cpu_startup_entry+0x2c3/0x2f8)

[c0050273] (cpu_startup_entry) from [800093d1] (0x800093d1)
 lock: tick_broadcast_lock+0x0/0x40, .magic: dead4ead, .owner: 
swapper/0/0, .owner_cpu: 0

CPU: 3 PID: 0 Comm: swapper/3 Not tainted 4.1.0-rc1 #16
Hardware name: ARM-Versatile Express
[c0014b1d] (unwind_backtrace) from [c0011659] (show_stack+0x11/0x14)
[c0011659] (show_stack) from [c05192ed] (dump_stack+0x6d/0x78)
[c05192ed] (dump_stack) from [c005c897] (do_raw_spin_lock+0xc3/0x144)
[c005c897] (do_raw_spin_lock) from [c0081409] 
(tick_broadcast_oneshot_control+0x45/0x1ec)
[c0081409] (tick_broadcast_oneshot_control) from [c0050273] 
(cpu_startup_entry+0x2c3/0x2f8)

[c0050273] (cpu_startup_entry) from [800093d1] (0x800093d1)





--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
 On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org wrote:
  Sudeep:
  At-least I observed issue only when I am using hardware broadcast timer.
  It doesn't hang when I am using hrtimer as broadcast timer in which case
  one of the cpu will be not enter deeper idle states that lose timer.
  I will rerun on v4.1-rc1 and post the complete log.
 
  So the bug here is that cpuidle_enter() enables interrupts, so the
  assumption about them being not enabled made by
  tick_broadcast_oneshot_control() is actually not valid.
 
  It looks like we need to acquire the clockevents_lock at least in this
  particular case.  Let me see where to put it and I'll send a patch for
  testing.
 
 Aha that looks very much like it. Put me on the patch and I'll
 take it for a spin.

OK, so something like the below for starters (the _irqsave variant is used to
avoid adding one more WARN_ON(irqs_disabled()) in there).

I haven't tested it, but then I can't reproduce the original issue in the
first place.

---
 include/linux/clockchips.h |2 ++
 kernel/sched/idle.c|2 +-
 kernel/time/clockevents.c  |   15 +++
 3 files changed, 18 insertions(+), 1 deletion(-)

Index: linux-pm/include/linux/clockchips.h
===
--- linux-pm.orig/include/linux/clockchips.h
+++ linux-pm/include/linux/clockchips.h
@@ -198,9 +198,11 @@ extern int tick_receive_broadcast(void);
 # if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST)  
defined(CONFIG_TICK_ONESHOT)
 extern void tick_setup_hrtimer_broadcast(void);
 extern int tick_check_broadcast_expired(void);
+extern void tick_broadcast_exit_idle(void);
 # else
 static inline int tick_check_broadcast_expired(void) { return 0; }
 static inline void tick_setup_hrtimer_broadcast(void) { }
+static inline void tick_broadcast_exit_idle(void) { }
 # endif
 
 extern int clockevents_notify(unsigned long reason, void *arg);
Index: linux-pm/kernel/time/clockevents.c
===
--- linux-pm.orig/kernel/time/clockevents.c
+++ linux-pm/kernel/time/clockevents.c
@@ -735,6 +735,21 @@ static ssize_t sysfs_unbind_tick_dev(str
 static DEVICE_ATTR(unbind_device, 0200, NULL, sysfs_unbind_tick_dev);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_broadcast_exit_idle - Broadcast oneshot mode exit for the idle loop.
+ *
+ * The idle loop exits the broadcast oneshot mode with interrupts enabled, so
+ * clockevents_lock needs to be acquired around that.
+ */
+void tick_broadcast_exit_idle(void)
+{
+   unsigned long flags;
+
+   raw_spin_lock_irqsave(clockevents_lock, flags);
+   tick_broadcast_exit();
+   raw_spin_unlock_irqrestore(clockevents_lock, flags);
+}
+
 static struct device tick_bc_dev = {
.init_name  = broadcast,
.id = 0,
Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -175,7 +175,7 @@ static void cpuidle_idle_call(void)
idle_set_state(this_rq(), NULL);
 
if (broadcast)
-   tick_broadcast_exit();
+   tick_broadcast_exit_idle();
 
/*
 * Give the governor an opportunity to reflect on the outcome

--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
  On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
   On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org 
   wrote:
Sudeep:
At-least I observed issue only when I am using hardware broadcast 
timer.
It doesn't hang when I am using hrtimer as broadcast timer in which 
case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.
   
So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.
   
It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.
   
   Aha that looks very much like it. Put me on the patch and I'll
   take it for a spin.
  
  OK, so something like the below for starters (the _irqsave variant is used 
  to
  avoid adding one more WARN_ON(irqs_disabled()) in there).
  
  I haven't tested it, but then I can't reproduce the original issue in the
  first place.
 
 Of course, the whole broadcast thing could be done from cpuidle_enter()
 in the first place, but then we could not avoid the problem with the cpuidle
 *callback* enabling interrupts possibly in there anyway (not to mention the
 coupled stuff).

That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I really
wouldn't expect it to re-enable interrupts on exit and the coupled thing
seems to be fundamentally at odds with that flag either.

So it should be possible to move the broadcast logic into the cpuidle layer,
which I'm going to try to do.

Please test the patch I've sent, though, as it should bring the code back to
where it was before the clockevents_notify() removal and it'd be good to verify
that.

Rafael

--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

On 28/04/15 15:14, Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:

On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:

On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org wrote:

Sudeep:

At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.


So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.


Aha that looks very much like it. Put me on the patch and I'll
take it for a spin.


OK, so something like the below for starters (the _irqsave variant is used to
avoid adding one more WARN_ON(irqs_disabled()) in there).

I haven't tested it, but then I can't reproduce the original issue in the
first place.


Of course, the whole broadcast thing could be done from cpuidle_enter()
in the first place, but then we could not avoid the problem with the cpuidle
*callback* enabling interrupts possibly in there anyway (not to mention the
coupled stuff).


That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I really
wouldn't expect it to re-enable interrupts on exit and the coupled thing
seems to be fundamentally at odds with that flag either.

So it should be possible to move the broadcast logic into the cpuidle layer,
which I'm going to try to do.



Makes sense.


Please test the patch I've sent, though, as it should bring the code back to
where it was before the clockevents_notify() removal and it'd be good to verify
that.



I tested your patch and it works now. Anyways I am continuing to run
stress tests on my board. I will report if I find any issues.

Regards,
Sudeep
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:
 
 On 28/04/15 15:14, Rafael J. Wysocki wrote:
  On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
  On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
  On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
  On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org 
  wrote:
  Sudeep:
  At-least I observed issue only when I am using hardware broadcast 
  timer.
  It doesn't hang when I am using hrtimer as broadcast timer in which 
  case
  one of the cpu will be not enter deeper idle states that lose timer.
  I will rerun on v4.1-rc1 and post the complete log.
 
  So the bug here is that cpuidle_enter() enables interrupts, so the
  assumption about them being not enabled made by
  tick_broadcast_oneshot_control() is actually not valid.
 
  It looks like we need to acquire the clockevents_lock at least in this
  particular case.  Let me see where to put it and I'll send a patch for
  testing.
 
  Aha that looks very much like it. Put me on the patch and I'll
  take it for a spin.
 
  OK, so something like the below for starters (the _irqsave variant is 
  used to
  avoid adding one more WARN_ON(irqs_disabled()) in there).
 
  I haven't tested it, but then I can't reproduce the original issue in the
  first place.
 
  Of course, the whole broadcast thing could be done from cpuidle_enter()
  in the first place, but then we could not avoid the problem with the 
  cpuidle
  *callback* enabling interrupts possibly in there anyway (not to mention the
  coupled stuff).
 
  That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I 
  really
  wouldn't expect it to re-enable interrupts on exit and the coupled thing
  seems to be fundamentally at odds with that flag either.
 
  So it should be possible to move the broadcast logic into the cpuidle 
  layer,
  which I'm going to try to do.
 
 
 Makes sense.
 
  Please test the patch I've sent, though, as it should bring the code back to
  where it was before the clockevents_notify() removal and it'd be good to 
  verify
  that.
 
 
 I tested your patch and it works now. Anyways I am continuing to run
 stress tests on my board. I will report if I find any issues.

Great, thanks!

Below is the patch I came up with in the meantime.

This moves the switch to broadcast timer logic into
cpuidle_enter_state() which allows tick_broadcast_exit() to be
called directly with interrupts disabled (as required), but
it also adds a fallback branch reflecting the 4.0 and earlier
behavior for idle states that enable interrupts on exit
from their -enter callbacks.

I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
set for such states, but people may try to add stuff like that in the
future, so it's better to catch that (hence the WARN_ON_ONCE) and do
our best to handle it gracefully anyway, IMO.

The if (entered_state == -EBUSY) check is conservative.  It may
be better to do if (entered_state  0) and fall back to the default
on all errors, but that's not what we do today (I guess the concern
would be what if the state -enter returns an error after entering
and exiting the idle state, in which case we may miss a wakeup event
if we fall back to the default).

---
 drivers/cpuidle/cpuidle.c  |   16 
 include/linux/clockchips.h |2 ++
 kernel/sched/idle.c|   16 ++--
 kernel/time/clockevents.c  |   13 +
 4 files changed, 33 insertions(+), 14 deletions(-)

Index: linux-pm/include/linux/clockchips.h
===
--- linux-pm.orig/include/linux/clockchips.h
+++ linux-pm/include/linux/clockchips.h
@@ -198,9 +198,11 @@ extern int tick_receive_broadcast(void);
 # if defined(CONFIG_GENERIC_CLOCKEVENTS_BROADCAST)  
defined(CONFIG_TICK_ONESHOT)
 extern void tick_setup_hrtimer_broadcast(void);
 extern int tick_check_broadcast_expired(void);
+extern void tick_broadcast_exit_idle_fallback(void);
 # else
 static inline int tick_check_broadcast_expired(void) { return 0; }
 static inline void tick_setup_hrtimer_broadcast(void) { }
+static inline void tick_broadcast_exit_idle_fallback(void) { }
 # endif
 
 extern int clockevents_notify(unsigned long reason, void *arg);
Index: linux-pm/kernel/time/clockevents.c
===
--- linux-pm.orig/kernel/time/clockevents.c
+++ linux-pm/kernel/time/clockevents.c
@@ -735,6 +735,19 @@ static ssize_t sysfs_unbind_tick_dev(str
 static DEVICE_ATTR(unbind_device, 0200, NULL, sysfs_unbind_tick_dev);
 
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
+/**
+ * tick_broadcast_exit_idle_fallback - Fallback broadcast oneshot mode exit.
+ *
+ * Called from within the CPU idle subsystem when exiting the broadcast oneshot
+ * mode with interrupts enabled (fallback case only).
+ */
+void tick_broadcast_exit_idle_fallback(void)
+{
+   raw_spin_lock_irq(clockevents_lock);
+   

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Wednesday, April 29, 2015 02:50:22 AM Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 02:58:37 PM Sudeep Holla wrote:
  
  On 28/04/15 15:14, Rafael J. Wysocki wrote:
   On Tuesday, April 28, 2015 03:37:44 PM Rafael J. Wysocki wrote:
   On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
   On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
   On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org 
   wrote:
   Sudeep:
   At-least I observed issue only when I am using hardware broadcast 
   timer.
   It doesn't hang when I am using hrtimer as broadcast timer in which 
   case
   one of the cpu will be not enter deeper idle states that lose timer.
   I will rerun on v4.1-rc1 and post the complete log.
  
   So the bug here is that cpuidle_enter() enables interrupts, so the
   assumption about them being not enabled made by
   tick_broadcast_oneshot_control() is actually not valid.
  
   It looks like we need to acquire the clockevents_lock at least in this
   particular case.  Let me see where to put it and I'll send a patch for
   testing.
  
   Aha that looks very much like it. Put me on the patch and I'll
   take it for a spin.
  
   OK, so something like the below for starters (the _irqsave variant is 
   used to
   avoid adding one more WARN_ON(irqs_disabled()) in there).
  
   I haven't tested it, but then I can't reproduce the original issue in 
   the
   first place.
  
   Of course, the whole broadcast thing could be done from cpuidle_enter()
   in the first place, but then we could not avoid the problem with the 
   cpuidle
   *callback* enabling interrupts possibly in there anyway (not to mention 
   the
   coupled stuff).
  
   That said, if the given state is marked with CPUIDLE_FLAG_TIMER_STOP, I 
   really
   wouldn't expect it to re-enable interrupts on exit and the coupled thing
   seems to be fundamentally at odds with that flag either.
  
   So it should be possible to move the broadcast logic into the cpuidle 
   layer,
   which I'm going to try to do.
  
  
  Makes sense.
  
   Please test the patch I've sent, though, as it should bring the code back 
   to
   where it was before the clockevents_notify() removal and it'd be good to 
   verify
   that.
  
  
  I tested your patch and it works now. Anyways I am continuing to run
  stress tests on my board. I will report if I find any issues.
 
 Great, thanks!
 
 Below is the patch I came up with in the meantime.
 
 This moves the switch to broadcast timer logic into
 cpuidle_enter_state() which allows tick_broadcast_exit() to be
 called directly with interrupts disabled (as required), but
 it also adds a fallback branch reflecting the 4.0 and earlier
 behavior for idle states that enable interrupts on exit
 from their -enter callbacks.
 
 I'm not aware of any valid cases when CPUIDLE_FLAG_TIMER_STOP can be
 set for such states, but people may try to add stuff like that in the
 future, so it's better to catch that (hence the WARN_ON_ONCE) and do
 our best to handle it gracefully anyway, IMO.
 
 The if (entered_state == -EBUSY) check is conservative.  It may
 be better to do if (entered_state  0) and fall back to the default
 on all errors, but that's not what we do today (I guess the concern
 would be what if the state -enter returns an error after entering
 and exiting the idle state, in which case we may miss a wakeup event
 if we fall back to the default).

Actually, if my understanding of things is correct (the local clock event
device cannot go away from under code executed with interrupts disabled
on the local CPU), the simplified one below should be sufficient.

---
 drivers/cpuidle/cpuidle.c |   16 
 kernel/sched/idle.c   |   16 ++--
 2 files changed, 18 insertions(+), 14 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -81,7 +81,6 @@ static void cpuidle_idle_call(void)
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int next_state, entered_state;
-   unsigned int broadcast;
bool reflect;
 
/*
@@ -150,17 +149,6 @@ static void cpuidle_idle_call(void)
goto exit_idle;
}
 
-   broadcast = drv-states[next_state].flags  CPUIDLE_FLAG_TIMER_STOP;
-
-   /*
-* Tell the time framework to switch to a broadcast timer
-* because our local timer will be shutdown. If a local timer
-* is used from another cpu as a broadcast timer, this call may
-* fail if it is not available
-*/
-   if (broadcast  tick_broadcast_enter())
-   goto use_default;
-
/* Take note of the planned idle state. */
idle_set_state(this_rq(), drv-states[next_state]);
 
@@ -174,8 +162,8 @@ static void cpuidle_idle_call(void)
/* The cpu is 

Re: [PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Linus Walleij]

On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org wrote:
 Sudeep:
 At-least I observed issue only when I am using hardware broadcast timer.
 It doesn't hang when I am using hrtimer as broadcast timer in which case
 one of the cpu will be not enter deeper idle states that lose timer.
 I will rerun on v4.1-rc1 and post the complete log.

 So the bug here is that cpuidle_enter() enables interrupts, so the
 assumption about them being not enabled made by
 tick_broadcast_oneshot_control() is actually not valid.

 It looks like we need to acquire the clockevents_lock at least in this
 particular case.  Let me see where to put it and I'll send a patch for
 testing.

Aha that looks very much like it. Put me on the patch and I'll
take it for a spin.

Yours,
Linus Walleij
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tue, Apr 28, 2015 at 12:42 PM, Sudeep Holla sudeep.ho...@arm.com wrote:


 On 28/04/15 11:34, Daniel Lezcano wrote:

 On 04/28/2015 12:11 PM, Linus Walleij wrote:

 On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki
 r...@rjwysocki.net wrote:

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

 Replace the clockevents_notify() call with an explicit function
 call.

 Signed-off-by: Thomas Gleixner t...@linutronix.de
 Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com


 For some reason this makes my Ux500 system arbitrarily hang,
 especially during boot. Bisected down to this commit. Since the
 entire changeset is removing the notifications altogether I can't
 just revert it.

 Disabling CONFIG_CPU_IDLE removes the problem.

 Tried registering a stub driver (I just #if 0 all the code in
 drivers/cpuidle/cpuidle-ux500.c) it still crashes.

 That makes me think something inside the cpuidle subsystem is
 locking up after this, but my other idea is that the timer may be
 involved in some way, like this is stressing the timer in some new
 yet untested way.

 Has anyone else seen problems with this or is it only ux500?

 I'm looking closer at it but feel a bit clueless...


 If you keep the #if 0 and remove the CPUIDLE_FLAG_TIMER_STOP flag,
 does it still crash ?


 At-least I observed issue only when I am using hardware broadcast timer.
 It doesn't hang when I am using hrtimer as broadcast timer in which case
 one of the cpu will be not enter deeper idle states that lose timer.
 I will rerun on v4.1-rc1 and post the complete log.

So the bug here is that cpuidle_enter() enables interrupts, so the
assumption about them being not enabled made by
tick_broadcast_oneshot_control() is actually not valid.

It looks like we need to acquire the clockevents_lock at least in this
particular case.  Let me see where to put it and I'll send a patch for
testing.

Rafael
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

On Tuesday, April 28, 2015 03:31:54 PM Rafael J. Wysocki wrote:
 On Tuesday, April 28, 2015 02:37:10 PM Linus Walleij wrote:
  On Tue, Apr 28, 2015 at 2:19 PM, Rafael J. Wysocki raf...@kernel.org 
  wrote:
   Sudeep:
   At-least I observed issue only when I am using hardware broadcast timer.
   It doesn't hang when I am using hrtimer as broadcast timer in which case
   one of the cpu will be not enter deeper idle states that lose timer.
   I will rerun on v4.1-rc1 and post the complete log.
  
   So the bug here is that cpuidle_enter() enables interrupts, so the
   assumption about them being not enabled made by
   tick_broadcast_oneshot_control() is actually not valid.
  
   It looks like we need to acquire the clockevents_lock at least in this
   particular case.  Let me see where to put it and I'll send a patch for
   testing.
  
  Aha that looks very much like it. Put me on the patch and I'll
  take it for a spin.
 
 OK, so something like the below for starters (the _irqsave variant is used to
 avoid adding one more WARN_ON(irqs_disabled()) in there).
 
 I haven't tested it, but then I can't reproduce the original issue in the
 first place.

Of course, the whole broadcast thing could be done from cpuidle_enter()
in the first place, but then we could not avoid the problem with the cpuidle
*callback* enabling interrupts possibly in there anyway (not to mention the
coupled stuff).


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Linus Walleij]

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki r...@rjwysocki.net wrote:

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

 Replace the clockevents_notify() call with an explicit function call.

 Signed-off-by: Thomas Gleixner t...@linutronix.de
 Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com

For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit.
Since the entire changeset is removing the notifications
altogether I can't just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem
is locking up after this, but my other idea is that the timer
may be involved in some way, like this is stressing the timer
in some new yet untested way.

Has anyone else seen problems with this or is it only
ux500?

I'm looking closer at it but feel a bit clueless...

Yours,
Linus Walleij
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

On 28/04/15 11:11, Linus Walleij wrote:

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki r...@rjwysocki.net wrote:


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

Replace the clockevents_notify() call with an explicit function call.

Signed-off-by: Thomas Gleixner t...@linutronix.de
Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com


For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit.
Since the entire changeset is removing the notifications
altogether I can't just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem
is locking up after this, but my other idea is that the timer
may be involved in some way, like this is stressing the timer
in some new yet untested way.

Has anyone else seen problems with this or is it only
ux500?

I'm looking closer at it but feel a bit clueless...



Yes I am too in similar situation with my Vexpress platform.
I did report with the lockdep logs[1] before v4.1-rc1.
I still see the issue with v4.1-rc1.

Regards,
Sudeep

[1] https://lkml.org/lkml/2015/4/23/329
--
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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Daniel Lezcano]

On 04/28/2015 12:11 PM, Linus Walleij wrote:

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki r...@rjwysocki.net wrote:


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

Replace the clockevents_notify() call with an explicit function call.

Signed-off-by: Thomas Gleixner t...@linutronix.de
Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com


For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit.
Since the entire changeset is removing the notifications
altogether I can't just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem
is locking up after this, but my other idea is that the timer
may be involved in some way, like this is stressing the timer
in some new yet untested way.

Has anyone else seen problems with this or is it only
ux500?

I'm looking closer at it but feel a bit clueless...


If you keep the #if 0 and remove the CPUIDLE_FLAG_TIMER_STOP flag, does 
it still crash ?



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




--
 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 16/20] sched/idle: Use explicit broadcast oneshot control function

[Sudeep Holla]

On 28/04/15 11:34, Daniel Lezcano wrote:

On 04/28/2015 12:11 PM, Linus Walleij wrote:

On Thu, Apr 2, 2015 at 12:22 AM, Rafael J. Wysocki
r...@rjwysocki.net wrote:


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

Replace the clockevents_notify() call with an explicit function
call.

Signed-off-by: Thomas Gleixner t...@linutronix.de
Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com


For some reason this makes my Ux500 system arbitrarily hang,
especially during boot. Bisected down to this commit. Since the
entire changeset is removing the notifications altogether I can't
just revert it.

Disabling CONFIG_CPU_IDLE removes the problem.

Tried registering a stub driver (I just #if 0 all the code in
drivers/cpuidle/cpuidle-ux500.c) it still crashes.

That makes me think something inside the cpuidle subsystem is
locking up after this, but my other idea is that the timer may be
involved in some way, like this is stressing the timer in some new
yet untested way.

Has anyone else seen problems with this or is it only ux500?

I'm looking closer at it but feel a bit clueless...


If you keep the #if 0 and remove the CPUIDLE_FLAG_TIMER_STOP flag,
does it still crash ?



At-least I observed issue only when I am using hardware broadcast timer.
It doesn't hang when I am using hrtimer as broadcast timer in which case
one of the cpu will be not enter deeper idle states that lose timer.
I will rerun on v4.1-rc1 and post the complete log.

Regards,
Sudeep
--
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/

[PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

From: Thomas Gleixner 

Replace the clockevents_notify() call with an explicit function call.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Rafael J. Wysocki 
---
 kernel/sched/idle.c |5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -158,8 +158,7 @@ static void cpuidle_idle_call(void)
 * is used from another cpu as a broadcast timer, this call may
 * fail if it is not available
 */
-   if (broadcast &&
-   clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, >cpu))
+   if (broadcast && tick_broadcast_enter())
goto use_default;
 
/* Take note of the planned idle state. */
@@ -176,7 +175,7 @@ static void cpuidle_idle_call(void)
idle_set_state(this_rq(), NULL);
 
if (broadcast)
-   clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, >cpu);
+   tick_broadcast_exit();
 
/*
 * Give the governor an opportunity to reflect on the outcome

--
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/

[PATCH 16/20] sched/idle: Use explicit broadcast oneshot control function

[Rafael J. Wysocki]

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

Replace the clockevents_notify() call with an explicit function call.

Signed-off-by: Thomas Gleixner t...@linutronix.de
Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
 kernel/sched/idle.c |5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Index: linux-pm/kernel/sched/idle.c
===
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -158,8 +158,7 @@ static void cpuidle_idle_call(void)
 * is used from another cpu as a broadcast timer, this call may
 * fail if it is not available
 */
-   if (broadcast 
-   clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, dev-cpu))
+   if (broadcast  tick_broadcast_enter())
goto use_default;
 
/* Take note of the planned idle state. */
@@ -176,7 +175,7 @@ static void cpuidle_idle_call(void)
idle_set_state(this_rq(), NULL);
 
if (broadcast)
-   clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, dev-cpu);
+   tick_broadcast_exit();
 
/*
 * Give the governor an opportunity to reflect on the outcome

--
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/