Re: [PATCH v9 06/10] time: tick-sched: Split tick_nohz_stop_sched_tick()

[Rafael J. Wysocki]

On Saturday, April 7, 2018 4:36:25 AM CEST Frederic Weisbecker wrote:
> On Wed, Apr 04, 2018 at 10:41:13AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > 
> > In order to address the issue with short idle duration predictions
> > by the idle governor after the scheduler tick has been stopped, split
> > tick_nohz_stop_sched_tick() into two separate routines, one computing
> > the time to the next timer event and the other simply stopping the
> > tick when the time to the next timer event is known.
> > 
> > Prepare these two routines to be called separately, as one of them
> > will be called by the idle governor in the cpuidle_select() code
> > path after subsequent changes.
> > 
> > Update the former callers of tick_nohz_stop_sched_tick() to use
> > the new routines, tick_nohz_next_event() and tick_nohz_stop_tick(),
> > instead of it and move the updates of the sleep_length field in
> > struct tick_sched into __tick_nohz_idle_stop_tick() as it doesn't
> > need to be updated anywhere else.
> > 
> > There should be no intentional visible changes in functionality
> > resulting from this change.
> > 
> > Signed-off-by: Rafael J. Wysocki 
> 
> Reviewed-by: Frederic Weisbecker 
> 
> Thanks! And sorry for the slow reviews, the changes are sensitive

Right, very much so.

> and I want to make sure we are not breaking some subtlety.

Thanks a lot for doing that!

Re: [PATCH v9 06/10] time: tick-sched: Split tick_nohz_stop_sched_tick()

[Rafael J. Wysocki]

On Saturday, April 7, 2018 4:36:25 AM CEST Frederic Weisbecker wrote:
> On Wed, Apr 04, 2018 at 10:41:13AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki 
> > 
> > In order to address the issue with short idle duration predictions
> > by the idle governor after the scheduler tick has been stopped, split
> > tick_nohz_stop_sched_tick() into two separate routines, one computing
> > the time to the next timer event and the other simply stopping the
> > tick when the time to the next timer event is known.
> > 
> > Prepare these two routines to be called separately, as one of them
> > will be called by the idle governor in the cpuidle_select() code
> > path after subsequent changes.
> > 
> > Update the former callers of tick_nohz_stop_sched_tick() to use
> > the new routines, tick_nohz_next_event() and tick_nohz_stop_tick(),
> > instead of it and move the updates of the sleep_length field in
> > struct tick_sched into __tick_nohz_idle_stop_tick() as it doesn't
> > need to be updated anywhere else.
> > 
> > There should be no intentional visible changes in functionality
> > resulting from this change.
> > 
> > Signed-off-by: Rafael J. Wysocki 
> 
> Reviewed-by: Frederic Weisbecker 
> 
> Thanks! And sorry for the slow reviews, the changes are sensitive

Right, very much so.

> and I want to make sure we are not breaking some subtlety.

Thanks a lot for doing that!

Re: [PATCH v9 06/10] time: tick-sched: Split tick_nohz_stop_sched_tick()

[Frederic Weisbecker]

On Wed, Apr 04, 2018 at 10:41:13AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> In order to address the issue with short idle duration predictions
> by the idle governor after the scheduler tick has been stopped, split
> tick_nohz_stop_sched_tick() into two separate routines, one computing
> the time to the next timer event and the other simply stopping the
> tick when the time to the next timer event is known.
> 
> Prepare these two routines to be called separately, as one of them
> will be called by the idle governor in the cpuidle_select() code
> path after subsequent changes.
> 
> Update the former callers of tick_nohz_stop_sched_tick() to use
> the new routines, tick_nohz_next_event() and tick_nohz_stop_tick(),
> instead of it and move the updates of the sleep_length field in
> struct tick_sched into __tick_nohz_idle_stop_tick() as it doesn't
> need to be updated anywhere else.
> 
> There should be no intentional visible changes in functionality
> resulting from this change.
> 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Frederic Weisbecker 

Thanks! And sorry for the slow reviews, the changes are sensitive
and I want to make sure we are not breaking some subtlety.

Re: [PATCH v9 06/10] time: tick-sched: Split tick_nohz_stop_sched_tick()

[Frederic Weisbecker]

On Wed, Apr 04, 2018 at 10:41:13AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> In order to address the issue with short idle duration predictions
> by the idle governor after the scheduler tick has been stopped, split
> tick_nohz_stop_sched_tick() into two separate routines, one computing
> the time to the next timer event and the other simply stopping the
> tick when the time to the next timer event is known.
> 
> Prepare these two routines to be called separately, as one of them
> will be called by the idle governor in the cpuidle_select() code
> path after subsequent changes.
> 
> Update the former callers of tick_nohz_stop_sched_tick() to use
> the new routines, tick_nohz_next_event() and tick_nohz_stop_tick(),
> instead of it and move the updates of the sleep_length field in
> struct tick_sched into __tick_nohz_idle_stop_tick() as it doesn't
> need to be updated anywhere else.
> 
> There should be no intentional visible changes in functionality
> resulting from this change.
> 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Frederic Weisbecker 

Thanks! And sorry for the slow reviews, the changes are sensitive
and I want to make sure we are not breaking some subtlety.

[PATCH v9 06/10] time: tick-sched: Split tick_nohz_stop_sched_tick()

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

In order to address the issue with short idle duration predictions
by the idle governor after the scheduler tick has been stopped, split
tick_nohz_stop_sched_tick() into two separate routines, one computing
the time to the next timer event and the other simply stopping the
tick when the time to the next timer event is known.

Prepare these two routines to be called separately, as one of them
will be called by the idle governor in the cpuidle_select() code
path after subsequent changes.

Update the former callers of tick_nohz_stop_sched_tick() to use
the new routines, tick_nohz_next_event() and tick_nohz_stop_tick(),
instead of it and move the updates of the sleep_length field in
struct tick_sched into __tick_nohz_idle_stop_tick() as it doesn't
need to be updated anywhere else.

There should be no intentional visible changes in functionality
resulting from this change.

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

v8 -> v9: No changes.

---
 kernel/time/tick-sched.c |  128 +--
 kernel/time/tick-sched.h |4 +
 2 files changed, 84 insertions(+), 48 deletions(-)

Index: linux-pm/kernel/time/tick-sched.h
===
--- linux-pm.orig/kernel/time/tick-sched.h
+++ linux-pm/kernel/time/tick-sched.h
@@ -39,6 +39,8 @@ enum tick_nohz_mode {
  * @idle_sleeptime:Sum of the time slept in idle with sched tick stopped
  * @iowait_sleeptime:  Sum of the time slept in idle with sched tick stopped, 
with IO outstanding
  * @sleep_length:  Duration of the current idle sleep
+ * @timer_expires: Anticipated timer expiration time (in case sched tick 
is stopped)
+ * @timer_expires_base:Base time clock monotonic for @timer_expires
  * @do_timer_lst:  CPU was the last one doing do_timer before going idle
  */
 struct tick_sched {
@@ -60,6 +62,8 @@ struct tick_sched {
ktime_t iowait_sleeptime;
ktime_t sleep_length;
unsigned long   last_jiffies;
+   u64 timer_expires;
+   u64 timer_expires_base;
u64 next_timer;
ktime_t idle_expires;
int do_timer_last;
Index: linux-pm/kernel/time/tick-sched.c
===
--- linux-pm.orig/kernel/time/tick-sched.c
+++ linux-pm/kernel/time/tick-sched.c
@@ -652,13 +652,10 @@ static inline bool local_timer_softirq_p
return local_softirq_pending() & TIMER_SOFTIRQ;
 }
 
-static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
-ktime_t now, int cpu)
+static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
 {
-   struct clock_event_device *dev = 
__this_cpu_read(tick_cpu_device.evtdev);
u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
unsigned long seq, basejiff;
-   ktime_t tick;
 
/* Read jiffies and the time when jiffies were updated last */
do {
@@ -667,6 +664,7 @@ static ktime_t tick_nohz_stop_sched_tick
basejiff = jiffies;
} while (read_seqretry(_lock, seq));
ts->last_jiffies = basejiff;
+   ts->timer_expires_base = basemono;
 
/*
 * Keep the periodic tick, when RCU, architecture or irq_work
@@ -711,32 +709,20 @@ static ktime_t tick_nohz_stop_sched_tick
 * next period, so no point in stopping it either, bail.
 */
if (!ts->tick_stopped) {
-   tick = 0;
+   ts->timer_expires = 0;
goto out;
}
}
 
/*
-* If this CPU is the one which updates jiffies, then give up
-* the assignment and let it be taken by the CPU which runs
-* the tick timer next, which might be this CPU as well. If we
-* don't drop this here the jiffies might be stale and
-* do_timer() never invoked. Keep track of the fact that it
-* was the one which had the do_timer() duty last. If this CPU
-* is the one which had the do_timer() duty last, we limit the
-* sleep time to the timekeeping max_deferment value.
+* If this CPU is the one which had the do_timer() duty last, we limit
+* the sleep time to the timekeeping max_deferment value.
 * Otherwise we can sleep as long as we want.
 */
delta = timekeeping_max_deferment();
-   if (cpu == tick_do_timer_cpu) {
-   tick_do_timer_cpu = TICK_DO_TIMER_NONE;
-   ts->do_timer_last = 1;
-   } else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
-   delta = KTIME_MAX;
-   ts->do_timer_last = 0;
-   } else if (!ts->do_timer_last) {

[PATCH v9 06/10] time: tick-sched: Split tick_nohz_stop_sched_tick()

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

In order to address the issue with short idle duration predictions
by the idle governor after the scheduler tick has been stopped, split
tick_nohz_stop_sched_tick() into two separate routines, one computing
the time to the next timer event and the other simply stopping the
tick when the time to the next timer event is known.

Prepare these two routines to be called separately, as one of them
will be called by the idle governor in the cpuidle_select() code
path after subsequent changes.

Update the former callers of tick_nohz_stop_sched_tick() to use
the new routines, tick_nohz_next_event() and tick_nohz_stop_tick(),
instead of it and move the updates of the sleep_length field in
struct tick_sched into __tick_nohz_idle_stop_tick() as it doesn't
need to be updated anywhere else.

There should be no intentional visible changes in functionality
resulting from this change.

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

v8 -> v9: No changes.

---
 kernel/time/tick-sched.c |  128 +--
 kernel/time/tick-sched.h |4 +
 2 files changed, 84 insertions(+), 48 deletions(-)

Index: linux-pm/kernel/time/tick-sched.h
===
--- linux-pm.orig/kernel/time/tick-sched.h
+++ linux-pm/kernel/time/tick-sched.h
@@ -39,6 +39,8 @@ enum tick_nohz_mode {
  * @idle_sleeptime:Sum of the time slept in idle with sched tick stopped
  * @iowait_sleeptime:  Sum of the time slept in idle with sched tick stopped, 
with IO outstanding
  * @sleep_length:  Duration of the current idle sleep
+ * @timer_expires: Anticipated timer expiration time (in case sched tick 
is stopped)
+ * @timer_expires_base:Base time clock monotonic for @timer_expires
  * @do_timer_lst:  CPU was the last one doing do_timer before going idle
  */
 struct tick_sched {
@@ -60,6 +62,8 @@ struct tick_sched {
ktime_t iowait_sleeptime;
ktime_t sleep_length;
unsigned long   last_jiffies;
+   u64 timer_expires;
+   u64 timer_expires_base;
u64 next_timer;
ktime_t idle_expires;
int do_timer_last;
Index: linux-pm/kernel/time/tick-sched.c
===
--- linux-pm.orig/kernel/time/tick-sched.c
+++ linux-pm/kernel/time/tick-sched.c
@@ -652,13 +652,10 @@ static inline bool local_timer_softirq_p
return local_softirq_pending() & TIMER_SOFTIRQ;
 }
 
-static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
-ktime_t now, int cpu)
+static ktime_t tick_nohz_next_event(struct tick_sched *ts, int cpu)
 {
-   struct clock_event_device *dev = 
__this_cpu_read(tick_cpu_device.evtdev);
u64 basemono, next_tick, next_tmr, next_rcu, delta, expires;
unsigned long seq, basejiff;
-   ktime_t tick;
 
/* Read jiffies and the time when jiffies were updated last */
do {
@@ -667,6 +664,7 @@ static ktime_t tick_nohz_stop_sched_tick
basejiff = jiffies;
} while (read_seqretry(_lock, seq));
ts->last_jiffies = basejiff;
+   ts->timer_expires_base = basemono;
 
/*
 * Keep the periodic tick, when RCU, architecture or irq_work
@@ -711,32 +709,20 @@ static ktime_t tick_nohz_stop_sched_tick
 * next period, so no point in stopping it either, bail.
 */
if (!ts->tick_stopped) {
-   tick = 0;
+   ts->timer_expires = 0;
goto out;
}
}
 
/*
-* If this CPU is the one which updates jiffies, then give up
-* the assignment and let it be taken by the CPU which runs
-* the tick timer next, which might be this CPU as well. If we
-* don't drop this here the jiffies might be stale and
-* do_timer() never invoked. Keep track of the fact that it
-* was the one which had the do_timer() duty last. If this CPU
-* is the one which had the do_timer() duty last, we limit the
-* sleep time to the timekeeping max_deferment value.
+* If this CPU is the one which had the do_timer() duty last, we limit
+* the sleep time to the timekeeping max_deferment value.
 * Otherwise we can sleep as long as we want.
 */
delta = timekeeping_max_deferment();
-   if (cpu == tick_do_timer_cpu) {
-   tick_do_timer_cpu = TICK_DO_TIMER_NONE;
-   ts->do_timer_last = 1;
-   } else if (tick_do_timer_cpu != TICK_DO_TIMER_NONE) {
-   delta = KTIME_MAX;
-   ts->do_timer_last = 0;
-   } else if (!ts->do_timer_last) {
+   if (cpu != tick_do_timer_cpu &&
+