[PATCH v2 0/4] Stop the idle tick on polling idle on nohz_full CPUs

[PATCH v2 0/4] Stop the idle tick on polling idle on nohz_full CPUs

[Marco Crivellari]

Hi,

On nohz_full CPUs booted with idle=poll, the tick interrupts the task once
after every wakeup from idle. do_idle() restarts the tick before polling,
and on wakeup tick_nohz_idle_exit() only re-evaluates the tick if it is
stopped, so it keeps running and fires in userspace before
tick_nohz_irq_exit() stops it. Without idle=poll this doesn't happen:
since commit a5183862e76f ("tick/nohz: Conditionally restart tick on idle
exit") a nohz_full CPU leaves idle with the tick still stopped.

Marcelo Tosatti proposed stopping the tick on polling idle entry:
  https://lore.kernel.org/all/aQiWfnnSzxsnwa2o@tpad/

Following Frederic's review, this series does it only on nohz_full CPUs,
and handles on the target CPU the cases where stopping the tick in polling
idle would leave a timer unprogrammed.

[ P1 ] tick/nohz: Re-evaluate the tick on return to user space
  On nohz_full CPUs the tick is re-evaluated on IRQ exit and on idle exit.

  This is not enough on return to user space. If the tick is left running in
  the kernel, for example because a tick dependency was cleared
  (tick_nohz_dep_clear*() don't kick the CPU) or a timer that kept it
  running was cancelled, it fires at least once in userspace before
  tick_nohz_irq_exit() stops it.

  Stop it from context tracking right before resuming userspace. When the
  tick is already stopped, the common case, this costs a few flag tests on
  each return to userspace.

[ P2 ] Don't handle polling idle as dynticks idle

  TS_FLAG_INIDLE tells nohz whether the tick is managed by the idle loop or
  on IRQ exit. With idle=poll it is set while polling, but cpu_idle_poll()
  doesn't run the idle loop again on interrupts: a nohz_full kick or the
  expiry of a stopped tick isn't re-evaluated until something sets
  TIF_NEED_RESCHED. The tick is therefore restarted before polling, and on
  nohz_full CPUs it then keeps running after the wake-up and interrupts
  userspace.

  Enter dynticks idle only on the cpuidle path, from cpuidle_idle_call(),
  and leave it before cpu_idle_poll(). While polling, the tick is then
  managed on IRQ exit as for a running task: on nohz_full CPUs it stays
  stopped if possible, and nothing is left to do on wake-up, so the task
  doesn't pay for a tick stop after waking up. On other CPUs, leaving
  dynticks idle restarts the tick as before.

[ P3 ] tick/nohz: Remove tick_nohz_idle_restart_tick()

  Polling idle now leaves dynticks idle through tick_nohz_idle_exit()
  before cpu_idle_poll(), which restarts the tick where needed.
  tick_nohz_idle_restart_tick() has no users left: remove it.

[ P4 ] Documentation: no_hz: Add tick stop in polling idle

  Document the above in the idle=poll item of "Known Issues".

With big effort on the Claude side, I made a test suite to help testing
the work:

  https://github.com/DispatchCode/nohz-full-polling-idle-tick-test-suite

The following is a short summary of the results.

On x86_64 guest, HZ=1000, nohz_full=2-3 idle=poll), a task alternating
100us sleeps with 10ms of userspace work, 200 times:
  - ticks in userspace:              200/200 -> 0/200
  - ticks while polling:             35 -> 0
  - housekeeping CPUs:               unchanged (tick runs while polling)
  - P2 without the kick change:      15 of 20 timers enqueued from an
                                     interrupt on the polling CPU lost
  - P2 without the stopped tick fix: the second of two queued timers up
                                     to 1.9s late
Tick dependencies (POSIX CPU timers, two tasks on the CPU) and CPU hotplug
were also checked.

Thanks,

  Marco

---
Changes in v2:
 - P1: The only change part of this patch is re-evaluate the tick on
   return to user space.
 
 - P2: Changed as suggested by Frederic: TS_FLAG_INIDLE is only set when
   the CPU goes idle and not in polling idle; tick_nohz_idle_enter() is
   called on every idle loop and runs with interrupt disabled. The tick
   is not restarted before polling idle, so
   tick_nohz_idle_restart_tick() has been removed.

 - P3: Remove tick_nohz_idle_restart_tick(), now unused.

Marco Crivellari (4):
  tick/nohz: Re-evaluate the tick on return to user space
  sched/idle: Don't handle polling idle as dynticks idle
  tick/nohz: Remove tick_nohz_idle_restart_tick()
  Documentation: no_hz: Add tick stop in polling idle

 Documentation/timers/no_hz.rst |  2 +
 include/linux/tick.h           | 10 ++++-
 kernel/context_tracking.c      |  8 ++++
 kernel/sched/idle.c            | 13 ++++++-
 kernel/time/tick-sched.c       | 68 ++++++++++++++++++++--------------
 5 files changed, 69 insertions(+), 32 deletions(-)

-- 
2.56.0

[PATCH v2 1/4] tick/nohz: Re-evaluate the tick on return to user space

[Marco Crivellari]

On nohz_full CPUs the tick is re-evaluated on IRQ exit and on idle exit.

This is not enough on return to user space. If the tick is left running in
the kernel, for example because a tick dependency was cleared
(tick_nohz_dep_clear*() don't kick the CPU) or a timer that kept it
running was cancelled, it fires at least once in userspace before
tick_nohz_irq_exit() stops it.

Stop it from context tracking right before resuming userspace. When the
tick is already stopped, the common case, this costs a few flag tests on
each return to userspace.

Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Thomas Gleixner <tglx@kernel.org>
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 include/linux/tick.h      |  8 ++++++++
 kernel/context_tracking.c |  8 ++++++++
 kernel/time/tick-sched.c  | 24 ++++++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index b121c5d53e30..8bfbb053f4fd 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -269,6 +269,7 @@ static inline void tick_dep_clear_signal(struct signal_struct *signal,
 extern void tick_nohz_full_kick_cpu(int cpu);
 extern void __tick_nohz_task_switch(void);
 extern void __init tick_nohz_full_setup(cpumask_var_t cpumask);
+extern void __tick_nohz_user_enter(void);
 #else
 static inline bool tick_nohz_full_enabled(void) { return false; }
 static inline bool tick_nohz_full_cpu(int cpu) { return false; }
@@ -294,6 +295,7 @@ static inline void tick_dep_clear_signal(struct signal_struct *signal,
 static inline void tick_nohz_full_kick_cpu(int cpu) { }
 static inline void __tick_nohz_task_switch(void) { }
 static inline void tick_nohz_full_setup(cpumask_var_t cpumask) { }
+static inline void __tick_nohz_user_enter(void) { }
 #endif
 
 static inline void tick_nohz_task_switch(void)
@@ -302,6 +304,12 @@ static inline void tick_nohz_task_switch(void)
 		__tick_nohz_task_switch();
 }
 
+static inline void tick_nohz_user_enter(void)
+{
+	if (tick_nohz_full_enabled())
+		__tick_nohz_user_enter();
+}
+
 static inline void tick_nohz_user_enter_prepare(void)
 {
 	if (tick_nohz_full_cpu(smp_processor_id()))
diff --git a/kernel/context_tracking.c b/kernel/context_tracking.c
index a743e7ffa6c0..347f99e1880f 100644
--- a/kernel/context_tracking.c
+++ b/kernel/context_tracking.c
@@ -24,6 +24,7 @@
 #include <linux/export.h>
 #include <linux/kprobes.h>
 #include <trace/events/rcu.h>
+#include <linux/tick.h>
 
 
 DEFINE_PER_CPU(struct context_tracking, context_tracking) = {
@@ -466,6 +467,13 @@ void noinstr __ct_user_enter(enum ctx_state state)
 			 */
 			if (state == CT_STATE_USER) {
 				instrumentation_begin();
+				/*
+				 * On nohz_full CPUs the tick may have been left running in the
+				 * kernel, e.g. kept by a tick dependency that is now gone.
+				 * Stop it now rather than let it interrupt userspace once
+				 * more before the next IRQ exit notices.
+				 */
+				tick_nohz_user_enter();
 				trace_user_enter(0);
 				vtime_user_enter(current);
 				instrumentation_end();
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index c6d86e4da8a9..645dba919386 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1071,6 +1071,30 @@ static void tick_nohz_full_update_tick(struct tick_sched *ts)
 	__tick_nohz_full_update_tick(ts, ktime_get());
 }
 
+#ifdef CONFIG_NO_HZ_FULL
+void __tick_nohz_user_enter(void)
+{
+	struct tick_sched *ts;
+	int cpu;
+
+	lockdep_assert_irqs_disabled();
+
+	cpu = smp_processor_id();
+	if (!tick_nohz_full_cpu(cpu))
+		return;
+
+	ts = this_cpu_ptr(&tick_cpu_sched);
+	if (!tick_sched_flag_test(ts, TS_FLAG_NOHZ))
+		return;
+
+	if (tick_sched_flag_test(ts, TS_FLAG_STOPPED))
+		return;
+
+	if (can_stop_full_tick(cpu, ts))
+		tick_nohz_full_stop_tick(ts, cpu);
+}
+#endif
+
 /*
  * A pending softirq outside an IRQ (or softirq disabled section) context
  * should be waiting for ksoftirqd to handle it. Therefore we shouldn't
-- 
2.56.0

[PATCH v2 2/4] sched/idle: Don't handle polling idle as dynticks idle

[Marco Crivellari]

TS_FLAG_INIDLE tells nohz whether the tick is managed by the idle loop or
on IRQ exit. With idle=poll it is set while polling, but cpu_idle_poll()
doesn't run the idle loop again on interrupts: a nohz_full kick or the
expiry of a stopped tick isn't re-evaluated until something sets
TIF_NEED_RESCHED. The tick is therefore restarted before polling, and on
nohz_full CPUs it then keeps running after the wake-up and interrupts
userspace.

Enter dynticks idle only on the cpuidle path, from cpuidle_idle_call(),
and leave it before cpu_idle_poll(). While polling, the tick is then
managed on IRQ exit as for a running task: on nohz_full CPUs it stays
stopped if possible, and nothing is left to do on wake-up, so the task
doesn't pay for a tick stop after waking up. On other CPUs, leaving
dynticks idle restarts the tick as before.

tick_nohz_idle_enter() may now be called on each idle loop iteration and
always runs with interrupts disabled; tick_nohz_idle_exit() may be called
outside dynticks idle and with interrupts disabled.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Juri Lelli <juri.lelli@redhat.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Ben Segall <bsegall@google.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Valentin Schneider <vschneid@redhat.com>
Cc: K Prateek Nayak <kprateek.nayak@amd.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Thomas Gleixner <tglx@kernel.org>
Link: https://lore.kernel.org/all/aQiWfnnSzxsnwa2o@tpad/
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 kernel/sched/idle.c      | 13 +++++++++++--
 kernel/time/tick-sched.c | 27 ++++++++++++++++-----------
 2 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index e0c10a5dbda2..e0e3c06da528 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -193,6 +193,8 @@ static void cpuidle_idle_call(bool stop_tick)
 		return;
 	}
 
+	tick_nohz_idle_enter();
+
 	if (cpuidle_not_available(drv, dev)) {
 		idle_call_stop_or_retain_tick(stop_tick);
 
@@ -303,7 +305,6 @@ static void do_idle(void)
 	 */
 
 	__current_set_polling();
-	tick_nohz_idle_enter();
 
 	while (!need_resched()) {
 
@@ -349,7 +350,15 @@ static void do_idle(void)
 		 * idle as we know that the IPI is going to arrive right away.
 		 */
 		if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
-			tick_nohz_idle_restart_tick();
+			/*
+			 * Polling idle is not dynticks idle: leave it before
+			 * polling so that the tick is managed on IRQ exit as for
+			 * a running task. On nohz_full CPUs it stays stopped and
+			 * nothing is left to do on wake-up that would delay the
+			 * task. On other CPUs, leaving dynticks idle restarts it
+			 * as before.
+			 */
+			tick_nohz_idle_exit();
 			cpu_idle_poll();
 		} else {
 			cpuidle_idle_call(got_tick);
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 645dba919386..0b798dc39511 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1227,23 +1227,22 @@ static void tick_nohz_clock_wakeup(struct tick_sched *ts)
 /**
  * tick_nohz_idle_enter - prepare for entering idle on the current CPU
  *
- * Called when we start the idle loop.
+ * Called with IRQs disabled on each idle loop iteration that enters
+ * cpuidle. Only the first call enters dynticks idle.
  */
 void tick_nohz_idle_enter(void)
 {
-	struct tick_sched *ts;
+	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
 
-	lockdep_assert_irqs_enabled();
+	lockdep_assert_irqs_disabled();
 
-	local_irq_disable();
+	if (tick_sched_flag_test(ts, TS_FLAG_INIDLE))
+		return;
 
-	ts = this_cpu_ptr(&tick_cpu_sched);
 	WARN_ON_ONCE(ts->timer_expires_base);
 	tick_sched_flag_set(ts, TS_FLAG_INIDLE);
 	ts->idle_entrytime = ktime_get();
 	tick_nohz_clock_sleep(ts);
-
-	local_irq_enable();
 }
 
 /**
@@ -1415,15 +1414,21 @@ static void tick_nohz_idle_update_tick(struct tick_sched *ts, ktime_t now)
  *        as long as possible.
  *   2.2) If the tick has dependencies, restart the tick.
  *
+ * Also called before polling idle, which is not dynticks idle: it may
+ * therefore be called outside dynticks idle, with or without IRQs
+ * disabled.
  */
 void tick_nohz_idle_exit(void)
 {
 	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
+	unsigned long flags;
 	ktime_t now;
 
-	local_irq_disable();
+	local_irq_save(flags);
+
+	if (!tick_sched_flag_test(ts, TS_FLAG_INIDLE))
+		goto out;
 
-	WARN_ON_ONCE(!tick_sched_flag_test(ts, TS_FLAG_INIDLE));
 	WARN_ON_ONCE(ts->timer_expires_base);
 
 	tick_sched_flag_clear(ts, TS_FLAG_INIDLE);
@@ -1434,8 +1439,8 @@ void tick_nohz_idle_exit(void)
 		tick_nohz_dyntick_stop(ts, now);
 		tick_nohz_idle_update_tick(ts, now);
 	}
-
-	local_irq_enable();
+out:
+	local_irq_restore(flags);
 }
 
 /*
-- 
2.56.0

[PATCH v2 3/4] tick/nohz: Remove tick_nohz_idle_restart_tick()

[Marco Crivellari]

Polling idle now leaves dynticks idle through tick_nohz_idle_exit()
before cpu_idle_poll(), which restarts the tick where needed.
tick_nohz_idle_restart_tick() has no users left: remove it.

Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Thomas Gleixner <tglx@kernel.org>
Suggested-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 include/linux/tick.h     |  2 --
 kernel/time/tick-sched.c | 17 -----------------
 2 files changed, 19 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 8bfbb053f4fd..686f7e58738b 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -130,7 +130,6 @@ extern bool tick_nohz_tick_stopped(void);
 extern bool tick_nohz_tick_stopped_cpu(int cpu);
 extern void tick_nohz_idle_stop_tick(void);
 extern void tick_nohz_idle_retain_tick(void);
-extern void tick_nohz_idle_restart_tick(void);
 extern void tick_nohz_idle_enter(void);
 extern void tick_nohz_idle_exit(void);
 extern void tick_nohz_irq_exit(void);
@@ -145,7 +144,6 @@ static inline int tick_nohz_tick_stopped(void) { return 0; }
 static inline int tick_nohz_tick_stopped_cpu(int cpu) { return 0; }
 static inline void tick_nohz_idle_stop_tick(void) { }
 static inline void tick_nohz_idle_retain_tick(void) { }
-static inline void tick_nohz_idle_restart_tick(void) { }
 static inline void tick_nohz_idle_enter(void) { }
 static inline void tick_nohz_idle_exit(void) { }
 static inline bool tick_nohz_idle_got_tick(void) { return false; }
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 0b798dc39511..ed4a02881653 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1374,23 +1374,6 @@ static void tick_nohz_dyntick_stop(struct tick_sched *ts, ktime_t now)
 	kcpustat_dyntick_stop(now, ts->last_tick - TICK_NSEC);
 }
 
-void tick_nohz_idle_restart_tick(void)
-{
-	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
-
-	if (tick_sched_flag_test(ts, TS_FLAG_STOPPED)) {
-		/*
-		 * Update entrytime here in case the tick restart is due to temporary
-		 * polling on forced broadcast. The tick may be stopped again later within
-		 * the same idle trip. The idle_entrytime was updated recently but make sure
-		 * no tiny amount of idle time is accounted twice.
-		 */
-		ts->idle_entrytime = ktime_get();
-		tick_nohz_dyntick_stop(ts, ts->idle_entrytime);
-		tick_nohz_restart_sched_tick(ts, ts->idle_entrytime);
-	}
-}
-
 static void tick_nohz_idle_update_tick(struct tick_sched *ts, ktime_t now)
 {
 	if (tick_nohz_full_cpu(smp_processor_id()))
-- 
2.56.0

[PATCH v2 4/4] Documentation: no_hz: Add tick stop in polling idle

[Marco Crivellari]

On nohz_full CPUs the scheduling-clock tick is stopped while polling.
Update the documentation accordingly.

Cc: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: Thomas Gleixner <tglx@kernel.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Marco Crivellari <marco.crivellari@suse.com>
---
 Documentation/timers/no_hz.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/timers/no_hz.rst b/Documentation/timers/no_hz.rst
index 4006cab64079..86cea04d2a49 100644
--- a/Documentation/timers/no_hz.rst
+++ b/Documentation/timers/no_hz.rst
@@ -246,6 +246,8 @@ Known Issues
 		be even worse than that of dyntick-idle.  Furthermore,
 		this parameter effectively disables Turbo Mode on Intel
 		CPUs, which can significantly reduce maximum performance.
+		Note also that on nohz_full CPUs the scheduling-clock tick
+		is stopped while polling, as in the other idle states.
 
 *	Adaptive-ticks slows user/kernel transitions slightly.
 	This is not expected to be a problem for computationally intensive
-- 
2.56.0