sched: Cleanup and improve polling idle loops

Remote wakeups of polling CPUs are a valuable performance improvement; add a tracepoint to make it much easier to verify that they're working.

Signed-off-by Andy Lutomirski <[email protected]>

---
  include/trace/events/sched.h | 20 ++++++++++++++++++++
  kernel/sched/core.c          |  4 ++++
  2 files changed, 24 insertions(+)

include/trace/events/sched.h +20 -0

index 67e1bbf..08f8632 100644
@@ -530,6 +530,26 @@ TRACE_EVENT(sched_swap_numa,
  			__entry->dst_pid, __entry->dst_tgid, __entry->dst_ngid,
  			__entry->dst_cpu, __entry->dst_nid)
  );
+
+/*
+ * Tracepoint for waking a polling cpu without an IPI.
+ */
+TRACE_EVENT(sched_wake_polling_cpu,
+
+	TP_PROTO(int cpu),
+
+	TP_ARGS(cpu),
+
+	TP_STRUCT__entry(
+		__field(	int,	cpu	)
+	),
+
+	TP_fast_assign(
+		__entry->cpu	= cpu;
+	),
+
+	TP_printk("cpu=%d", __entry->cpu)
+);
  #endif /* _TRACE_SCHED_H */
 
  /* This part must be outside protection */

kernel/sched/core.c +4 -0

index 321d800..18bebfc 100644
@@ -564,6 +564,8 @@ void resched_task(struct task_struct *p)
 
  	if (set_nr_and_not_polling(p))
  		smp_send_reschedule(cpu);
+	else
+		trace_sched_wake_polling_cpu(cpu);
  }
 
  void resched_cpu(int cpu)

@@ -647,6 +649,8 @@ static void wake_up_idle_cpu(int cpu)
  	smp_mb();
  	if (!tsk_is_polling(rq->idle))
  		smp_send_reschedule(cpu);
+	else
+		trace_sched_wake_polling_cpu(cpu);
  }
 
  static bool wake_up_full_nohz_cpu(int cpu)

Quoted 5 lines · Andy Lutomirski

Remote wakeups of polling CPUs are a valuable performance improvement; add a tracepoint to make it much easier to verify that they're working.

Signed-off-by Andy Lutomirski <[email protected]>

I don't think this trace makes sense. The polling state is x86 only and this trace is in the generic code.

Quoted 6 lines · Andy Lutomirski

Remote wakeups of polling CPUs are a valuable performance improvement; add a tracepoint to make it much easier to verify that they're working.

Signed-off-by Andy Lutomirski <[email protected]>

I don't think this trace makes sense. The polling state is x86 only and this trace is in the generic code.

Furthermore, you may be not in polling state but in the idle mainloop before or after the idle state, so the trace will be wrong for the purpose you are aiming.

IMO, the right place would be in 'poll_idle' but why add such trace ?

If, on x86, we exit poll_idle, we have the idle state exit trace. The missing information would be the origin of the 'wakeup' (irq or ipi or nothing). The missing informations are the IPI traces [1].

And as a sidenote, the polling state could be rare on a system with a cpuidle driver, it should be much more easy to restrict the idle states to 'poll' only and check there are no IPI_WAKEUP, no ?

This doesn't change functionality at all, but I've misread this code so many times that I want to make it a bit more obvious.

Signed-off-by Andy Lutomirski <[email protected]>

---
  kernel/sched/idle.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)

kernel/sched/idle.c +4 -0

index 25b9423..2ec9f47 100644
@@ -67,6 +67,10 @@ void __weak arch_cpu_idle(void)
   * cpuidle_idle_call - the main idle function
   *
   * NOTE: no locks or semaphores should be used here
+ *
+ * On archs that support TIF_POLLING_NRFLAG, is called with polling
+ * set, and it returns with polling set.  If it ever stops polling, it
+ * must clear the polling bit.
   */
  static void cpuidle_idle_call(void)
  {

@@ -178,7 +182,14 @@ exit_idle:
   */

kernel/sched/idle.c +0 -0

  	arch_cpu_idle_prepare();
  	cpu_idle_loop();
  }

Quoted 31 lines · Andy Lutomirski

This doesn't change functionality at all, but I've misread this code so many times that I want to make it a bit more obvious.

Signed-off-by Andy Lutomirski <[email protected]>

---
  kernel/sched/idle.c | 12 +++++++++++-
  1 file changed, 11 insertions(+), 1 deletion(-)

kernel/sched/idle.c +6 -0

index 25b9423..2ec9f47 100644
@@ -67,6 +67,10 @@ void __weak arch_cpu_idle(void)
   * cpuidle_idle_call - the main idle function
   *
   * NOTE: no locks or semaphores should be used here
+ *
+ * On archs that support TIF_POLLING_NRFLAG, is called with polling
+ * set, and it returns with polling set.  If it ever stops polling, it
+ * must clear the polling bit.
   */
  static void cpuidle_idle_call(void)
  {

@@ -178,7 +182,14 @@ exit_idle:
   */
  static void cpu_idle_loop(void)
  {
+       __current_set_polling();
+
        while (1) {

kernel/sched/idle.c exit_idle: · line 192

Quoted 5 lines

kernel/sched/idle.c +5 -0

+               /*
+                * Invariant: polling is set here (assuming that the arch
+                * has a polling bit.
+                */
+

nit : extra line

kernel/sched/idle.c void cpu_startup_entry(enum cpuhp_state state) · line 242

Quoted 9 lines

kernel/sched/idle.c +0 -1

                tick_nohz_idle_enter();
 
                while (!need_resched()) {

@@ -239,7 +250,6 @@ void cpu_startup_entry(enum cpuhp_state state)
         */
        boot_init_stack_canary();
  #endif
-       __current_set_polling();

I don't get the connection with the patch description.

Currently, the only real guarantee provided by the polling bit is that, if you hold rq->lock and the polling bit is set, then you can set need_resched to force a reschedule.

The only reason the lock is needed is that the idle thread might not be running at all when setting its need_resched bit, and rq->lock keeps it pinned.

This is easy to fix: just clear the polling bit before scheduling. Now the polling bit is only ever set when rq->curr == rq->idle.

Quoted 8 lines · Andy Lutomirski

Currently, the only real guarantee provided by the polling bit is that, if you hold rq->lock and the polling bit is set, then you can set need_resched to force a reschedule.

The only reason the lock is needed is that the idle thread might not be running at all when setting its need_resched bit, and rq->lock keeps it pinned.

Quoted 2 lines

This is easy to fix: just clear the polling bit before scheduling. Now the polling bit is only ever set when rq->curr == rq->idle.

Yah, except of course:

lkml.kernel.org/r/[email protected]

which I really need to rebase and post again :/

In any case, this is useful even with that, although then we really must do something like:

  rcu_read_lock();
  if (!set_nr_if_polling(rq->curr))
        smp_send_reschedule(rq->cpu);
  rcu_read_unlock();

Because there's other tasks than rq->idle which might be 'idle', joy!

Quoted 14 lines · Peter Zijlstra

Quoted 10 lines · Andy Lutomirski

Currently, the only real guarantee provided by the polling bit is that, if you hold rq->lock and the polling bit is set, then you can set need_resched to force a reschedule.

The only reason the lock is needed is that the idle thread might not be running at all when setting its need_resched bit, and rq->lock keeps it pinned.

This is easy to fix: just clear the polling bit before scheduling. Now the polling bit is only ever set when rq->curr == rq->idle.

Yah, except of course:

Quoted 1 line

lkml.kernel.org/r/[email protected]

Wow, that code was ugly.

Can you be persuaded to hold off on that patch until after this series is done? I think the cpu_startup_entry change will just muddy the waters for now.

Quoted 11 lines

which I really need to rebase and post again :/

In any case, this is useful even with that, although then we really must do something like:

  rcu_read_lock();
  if (!set_nr_if_polling(rq->curr))
        smp_send_reschedule(rq->cpu);
  rcu_read_unlock();

Because there's other tasks than rq->idle which might be 'idle', joy!

Is this really a problem?

It's certainly the case that a non-idle (in the sense of != rq->idle) task can have the polling bit set, but so what? I'm perfectly happy to wake these ersatz idle things via IPI instead of via TIF_NEED_RESCHED. I think that all of the code that plays with the polling bit after all these patches are applied either holds rq->lock (which prevents the task from going away) or acts directly on rq->idle, which really has no business being deallocated.

I think that the RCU solution is actually racy, too. Consider:

kernel/sched/idle.c +0 -0

 		schedule_preempt_disabled();
 	}
 }

Signature · 1 line

1.9.3

kernel/sched/idle.c static void cpu_idle_loop(void) · line 238

Quoted 6 lines · Andy Lutomirski

kernel/sched/idle.c +2 -0

@@ -229,6 +234,8 @@ static void cpu_idle_loop(void)
               */
              preempt_set_need_resched();
              tick_nohz_idle_exit();
+             __current_clr_polling();
+             smp_mb__after_clear_bit();

barriers always need a comment, and I'm not entirely sure why you put this one here.