clock event device’s next_event

clock event device’s next_event

[Sodagudi Prasad]

Hi Viresh and Thomas,

In the functions tick_nohz_stop_sched_tick(), when expires = KTIME_MAX 
we are canceling the tick_sched_timer timer but we are not updating the 
clock event device’s next_event to KTIME_MAX.
Due to that broadcast device’s next_event is not programmed properly and 
resulting unnecessary wakeups for this cpu.

         /*
          * If the expiration time == KTIME_MAX, then we simply stop
          * the tick timer.
          */
         if (unlikely(expires == KTIME_MAX)) {
                 if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
                         hrtimer_cancel(&ts->sched_timer);
                 goto out;
         }


After digging further, I see that following call flow is updating 
tick_cpu_device state to shutdown state but clock event device  
next_event is not updated to KTIME_MAX.
hrtimer_cancel -> __remove_hrtimer -> hrtimer_force_reprogram -> 
tick_program_event.

int tick_program_event(ktime_t expires, int force)
{
         struct clock_event_device *dev = 
__this_cpu_read(tick_cpu_device.evtdev);

         if (unlikely(expires == KTIME_MAX)) {
                 /*
                  * We don't need the clock event device any more, stop 
it.
                  */
                 clockevents_switch_state(dev, 
CLOCK_EVT_STATE_ONESHOT_STOPPED);
                 return 0;
         }
In the above tick_program_event() function clock event device’s 
next_event is not getting updated as clockevents_program_event() 
function not called after state update.

-thanks, Prasad


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: clock event device’s next_event

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

On Mon, 23 Oct 2017, Sodagudi Prasad wrote:

> Hi Viresh and Thomas,
> 
> In the functions tick_nohz_stop_sched_tick(), when expires = KTIME_MAX we are
> canceling the tick_sched_timer timer but we are not updating the clock event
> device’s next_event to KTIME_MAX.
> Due to that broadcast device’s next_event is not programmed properly and
> resulting unnecessary wakeups for this cpu.
> 
>         /*
>          * If the expiration time == KTIME_MAX, then we simply stop
>          * the tick timer.
>          */
>         if (unlikely(expires == KTIME_MAX)) {
>                 if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
>                         hrtimer_cancel(&ts->sched_timer);
>                 goto out;
>         }

Right, because this code does not have access to the broadcast device at
all. It doesn't even know and care about it.

> After digging further, I see that following call flow is updating
> tick_cpu_device state to shutdown state but clock event device  next_event is
> not updated to KTIME_MAX.
> hrtimer_cancel -> __remove_hrtimer -> hrtimer_force_reprogram ->
> tick_program_event.
> 
> int tick_program_event(ktime_t expires, int force)
> {
>         struct clock_event_device *dev =
> __this_cpu_read(tick_cpu_device.evtdev);
> 
>         if (unlikely(expires == KTIME_MAX)) {
>                 /*
>                  * We don't need the clock event device any more, stop it.
>                  */
>                 clockevents_switch_state(dev,
> CLOCK_EVT_STATE_ONESHOT_STOPPED);
>                 return 0;
>         }
> In the above tick_program_event() function clock event device’s next_event is
> not getting updated as clockevents_program_event() function not called after
> state update.

If the device is shutdown, then next_event does not matter. But yes, for
consistency reasons we could set it to KTIME_MAX.

Thanks,

	tglx

Re: clock event device’s next_event

[Sodagudi Prasad]

On 2017-10-24 00:23, Thomas Gleixner wrote:
> On Mon, 23 Oct 2017, Sodagudi Prasad wrote:
> 
>> Hi Viresh and Thomas,
>> 
>> In the functions tick_nohz_stop_sched_tick(), when expires = KTIME_MAX 
>> we are
>> canceling the tick_sched_timer timer but we are not updating the clock 
>> event
>> device’s next_event to KTIME_MAX.
>> Due to that broadcast device’s next_event is not programmed properly 
>> and
>> resulting unnecessary wakeups for this cpu.
>> 
>>         /*
>>          * If the expiration time == KTIME_MAX, then we simply stop
>>          * the tick timer.
>>          */
>>         if (unlikely(expires == KTIME_MAX)) {
>>                 if (ts->nohz_mode == NOHZ_MODE_HIGHRES)
>>                         hrtimer_cancel(&ts->sched_timer);
>>                 goto out;
>>         }
> 
> Right, because this code does not have access to the broadcast device 
> at
> all. It doesn't even know and care about it.
> 

Some of the drivers in cpuidle frame works are using 
tick_nohz_get_sleep_length API
to find maximum idle/sleep time from ts->sleep_length.

And also the estimated sleep length until the next timer in 
tick_nohz_stop_sched_tick
not getting updated at the end.

>> After digging further, I see that following call flow is updating
>> tick_cpu_device state to shutdown state but clock event device  
>> next_event is
>> not updated to KTIME_MAX.
>> hrtimer_cancel -> __remove_hrtimer -> hrtimer_force_reprogram ->
>> tick_program_event.
>> 
>> int tick_program_event(ktime_t expires, int force)
>> {
>>         struct clock_event_device *dev =
>> __this_cpu_read(tick_cpu_device.evtdev);
>> 
>>         if (unlikely(expires == KTIME_MAX)) {
>>                 /*
>>                  * We don't need the clock event device any more, stop 
>> it.
>>                  */
>>                 clockevents_switch_state(dev,
>> CLOCK_EVT_STATE_ONESHOT_STOPPED);
>>                 return 0;
>>         }
>> In the above tick_program_event() function clock event device’s 
>> next_event is
>> not getting updated as clockevents_program_event() function not called 
>> after
>> state update.
> 
> If the device is shutdown, then next_event does not matter. But yes, 
> for
> consistency reasons we could set it to KTIME_MAX.
> 
Thanks tglx for your quick reply on this and I will send patch for the 
same.

> Thanks,
> 
> 	tglx

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

[PATCH] Clockevents: Always call clockevents_program_event

[Prasad Sodagudi]

Currently tick_program_event function is not calling
clockevents_program_event when  'expires == KTIME_MAX',
it is just updating clockevent state to CLOCK_EVT_STATE_ONESHOT_STOPPED.
clockevents_program_event function updates clockevent
device next_event by checking clockevent device state,
so always call clockevents_program_event() from tick_program_event.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
---
 kernel/time/tick-oneshot.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 6b009c2..650b071 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -28,15 +28,12 @@ int tick_program_event(ktime_t expires, int force)
 {
 	struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
 
-	if (unlikely(expires == KTIME_MAX)) {
+	if (expires == KTIME_MAX) {
 		/*
 		 * We don't need the clock event device any more, stop it.
 		 */
 		clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT_STOPPED);
-		return 0;
-	}
-
-	if (unlikely(clockevent_state_oneshot_stopped(dev))) {
+	} else if (clockevent_state_oneshot_stopped(dev)) {
 		/*
 		 * We need the clock event again, configure it in ONESHOT mode
 		 * before using it.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] Clockevents: Always call clockevents_program_event

[Thomas Gleixner]

On Tue, 24 Oct 2017, Prasad Sodagudi wrote:

> Currently tick_program_event function is not calling
> clockevents_program_event when  'expires == KTIME_MAX',
> it is just updating clockevent state to CLOCK_EVT_STATE_ONESHOT_STOPPED.
> clockevents_program_event function updates clockevent
> device next_event by checking clockevent device state,
> so always call clockevents_program_event() from tick_program_event.

No. This is fundmentally wrong. If the clockevent is in oneshot stopped
mode then you cannot call clockevents_program_event(). There is even a
warning in that code which will trigger.

Thanks,

	tglx

Re: [PATCH] Clockevents: Always call clockevents_program_event

[Sodagudi Prasad]

On 2017-10-24 01:37, Thomas Gleixner wrote:
> On Tue, 24 Oct 2017, Prasad Sodagudi wrote:
> 
>> Currently tick_program_event function is not calling
>> clockevents_program_event when  'expires == KTIME_MAX',
>> it is just updating clockevent state to 
>> CLOCK_EVT_STATE_ONESHOT_STOPPED.
>> clockevents_program_event function updates clockevent
>> device next_event by checking clockevent device state,
>> so always call clockevents_program_event() from tick_program_event.
> 
> No. This is fundmentally wrong. If the clockevent is in oneshot stopped
> mode then you cannot call clockevents_program_event(). There is even a
> warning in that code which will trigger.

<Prasad> Yes. There is warning and I overlooked at that part of the code 
and thought
it would return from the clockevents_program_event function after 
next_event update.
   dev->next_event = expires;

         if (clockevent_state_shutdown(dev))
                 return 0;

Thanks tglx for reviewing patch.

How to clean next next_event from clockevent device in the 
ONESHOT_STOPPED state from tick_program_event()?

Shall I update the next patch set with following condition?  Or Any 
other suggestions to fix this path?

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 4237e07..21104b6 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -317,7 +317,8 @@ int clockevents_program_event(struct 
clock_event_device *dev, ktime_t expires,

         dev->next_event = expires;

-       if (clockevent_state_shutdown(dev))
+       if (clockevent_state_shutdown(dev) ||
+               clockevent_state_oneshot_stopped(dev))
                 return 0;

> 
> Thanks,
> 
> 	tglx

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
Linux Foundation Collaborative Project

Re: [PATCH] Clockevents: Always call clockevents_program_event

[Thomas Gleixner]

On Tue, 24 Oct 2017, Sodagudi Prasad wrote:

> On 2017-10-24 01:37, Thomas Gleixner wrote:
> > On Tue, 24 Oct 2017, Prasad Sodagudi wrote:
> > 
> > > Currently tick_program_event function is not calling
> > > clockevents_program_event when  'expires == KTIME_MAX',
> > > it is just updating clockevent state to CLOCK_EVT_STATE_ONESHOT_STOPPED.
> > > clockevents_program_event function updates clockevent
> > > device next_event by checking clockevent device state,
> > > so always call clockevents_program_event() from tick_program_event.
> > 
> > No. This is fundmentally wrong. If the clockevent is in oneshot stopped
> > mode then you cannot call clockevents_program_event(). There is even a
> > warning in that code which will trigger.
> 
> <Prasad> Yes. There is warning and I overlooked at that part of the code and
> thought
> it would return from the clockevents_program_event function after next_event
> update.
>   dev->next_event = expires;
> 
>         if (clockevent_state_shutdown(dev))
>                 return 0;
> 
> Thanks tglx for reviewing patch.
> 
> How to clean next next_event from clockevent device in the ONESHOT_STOPPED
> state from tick_program_event()?
> 
> Shall I update the next patch set with following condition?  Or Any other
> suggestions to fix this path?
> 
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 4237e07..21104b6 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -317,7 +317,8 @@ int clockevents_program_event(struct clock_event_device
> *dev, ktime_t expires,
> 
>         dev->next_event = expires;
> 
> -       if (clockevent_state_shutdown(dev))
> +       if (clockevent_state_shutdown(dev) ||
> +               clockevent_state_oneshot_stopped(dev))
>                 return 0;

This needs more thought than that.

Thanks,

	tglx

[PATCH] clockevents: Update clockevents device next_event

[Prasad Sodagudi]

Usually clockevent device's next_event is updated in
clockevents_program_event() and next_event indicates
the next timer event on that cpu. Whenever there are
no timers on a CPU, corresponding clockevent device
is going into ONESHOT_STOPPED state but clockevent
device next_event is not updated, because
clockevents_program_event() not called.

As next_event is not updated properly, resulting in spurious
wakeups, so update the clockevent device next_event
with proper value(KTIME_MAX) to avoid spurious wakeups.

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
---
 kernel/time/tick-oneshot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 6b009c2..c1f518e 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -33,6 +33,7 @@ int tick_program_event(ktime_t expires, int force)
 		 * We don't need the clock event device any more, stop it.
 		 */
 		clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT_STOPPED);
+		dev->next_event = KTIME_MAX;
 		return 0;
 	}
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[tip:timers/core] clockevents: Update clockevents device next_event on stop

[tip-bot for Prasad Sodagudi]

Commit-ID:  39c82caff8610d57ffe32157cb3130dfabe12fbe
Gitweb:     https://git.kernel.org/tip/39c82caff8610d57ffe32157cb3130dfabe12fbe
Author:     Prasad Sodagudi <psodagud@codeaurora.org>
AuthorDate: Thu, 26 Oct 2017 11:37:22 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Wed, 1 Nov 2017 18:20:17 +0100

clockevents: Update clockevents device next_event on stop

clockevent_device::next_event holds the next timer event of a clock event
device. The value is updated in clockevents_program_event(), i.e. when the
hardware timer is armed for the next expiry.

When there are no software timers armed on a CPU, the corresponding per CPU
clockevent device is brought into ONESHOT_STOPPED state, but
clockevent_device::next_event is not updated, because
clockevents_program_event() is not called.

So the content of clockevent_device::next_event is stale, which is not an
issue when real hardware is used. But the hrtimer broadcast device relies
on that information and the stale value causes spurious wakeups.

Update clockevent_device::next_event to KTIME_MAX when it has been brought
into ONESHOT_STOPPED state to avoid spurious wakeups. This reflects the
proper expiry time of the stopped timer: infinity.

[ tglx: Massaged changelog ]

Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: viresh.kumar@linaro.org
Link: https://lkml.kernel.org/r/1509043042-32486-1-git-send-email-psodagud@codeaurora.org

---
 kernel/time/tick-oneshot.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 6b009c2..c1f518e 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -33,6 +33,7 @@ int tick_program_event(ktime_t expires, int force)
 		 * We don't need the clock event device any more, stop it.
 		 */
 		clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT_STOPPED);
+		dev->next_event = KTIME_MAX;
 		return 0;
 	}