[PATCH 00/10] timers/cpuidle: Fixes and cleanups

Frederic Weisbecker posted 10 patches 2 years, 4 months ago
arch/loongarch/kernel/genex.S      | 32 ++++++++++++++++++--------------
arch/loongarch/kernel/idle.c       |  1 -
arch/x86/include/asm/mwait.h       | 21 ++++++++++++++++++---
arch/x86/kernel/process.c          |  1 -
drivers/acpi/acpi_pad.c            |  1 +
drivers/cpuidle/cpuidle-haltpoll.c |  5 +----
drivers/cpuidle/poll_state.c       | 10 ++++++++--
drivers/idle/intel_idle.c          | 19 +++++++------------
kernel/sched/core.c                | 22 ++++++++++++++++++++++
kernel/sched/idle.c                | 30 ++++++++++++++++++++++++++++++
10 files changed, 105 insertions(+), 37 deletions(-)
[PATCH 00/10] timers/cpuidle: Fixes and cleanups
Posted by Frederic Weisbecker 2 years, 4 months ago
Hi,

The first part of the series is cpuidle callback fixes against timers,
some of which haven't been Signed by Peter yet.

Another part is removing the overhead of useless TIF_NR_POLLING clearing.

The rest is comments.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	timers/idle

HEAD: b66dd12bb29cca558b9323f2b270a7dae8f56c48

Thanks,
	Frederic
---

Frederic Weisbecker (8):
      x86: Add a comment about the "magic" behind shadow sti before mwait
      cpuidle: Report illegal tick stopped while polling
      cpuidle: Comment about timers requirements VS idle handler
      cpuidle: Remove unnecessary current_clr_polling_and_test() from haltpoll
      cpuidle: Remove unnecessary current_clr_polling() on poll_idle()
      x86: Remove __current_clr_polling() from mwait_idle()
      x86: Remove the current_clr_polling() call upon mwait exit
      sched/timers: Explain why idle task schedules out on remote timer enqueue

Peter Zijlstra (2):
      cpuidle: Fix CPUIDLE_FLAG_IRQ_ENABLE leaking timer reprogram
      loongson: Fix idle VS timer enqueue


 arch/loongarch/kernel/genex.S      | 32 ++++++++++++++++++--------------
 arch/loongarch/kernel/idle.c       |  1 -
 arch/x86/include/asm/mwait.h       | 21 ++++++++++++++++++---
 arch/x86/kernel/process.c          |  1 -
 drivers/acpi/acpi_pad.c            |  1 +
 drivers/cpuidle/cpuidle-haltpoll.c |  5 +----
 drivers/cpuidle/poll_state.c       | 10 ++++++++--
 drivers/idle/intel_idle.c          | 19 +++++++------------
 kernel/sched/core.c                | 22 ++++++++++++++++++++++
 kernel/sched/idle.c                | 30 ++++++++++++++++++++++++++++++
 10 files changed, 105 insertions(+), 37 deletions(-)
Re: [PATCH 00/10] timers/cpuidle: Fixes and cleanups
Posted by Peter Zijlstra 2 years, 4 months ago
On Fri, Aug 11, 2023 at 07:00:39PM +0200, Frederic Weisbecker wrote:
> Hi,
> 
> The first part of the series is cpuidle callback fixes against timers,
> some of which haven't been Signed by Peter yet.
> 
> Another part is removing the overhead of useless TIF_NR_POLLING clearing.

So I've again forgotten why we don't simply set TIF_NEED_RESCHED if we
need the timer re-programmed. That is by far the simplest fix.

I'm sure we talked about it, but this was a long time ago and I can't
remember :/

Anyway, the patches look good, except I think there's a whole bunch of
architectures that still need fixing. In particular since loongson
'borrowed' the whole lot from MIPS, they need an identical fix. But I'm
sure there's more architectures affected.
Re: [PATCH 00/10] timers/cpuidle: Fixes and cleanups
Posted by Frederic Weisbecker 2 years, 3 months ago
On Tue, Aug 15, 2023 at 06:10:43PM +0200, Peter Zijlstra wrote:
> On Fri, Aug 11, 2023 at 07:00:39PM +0200, Frederic Weisbecker wrote:
> > Hi,
> > 
> > The first part of the series is cpuidle callback fixes against timers,
> > some of which haven't been Signed by Peter yet.
> > 
> > Another part is removing the overhead of useless TIF_NR_POLLING clearing.
> 
> So I've again forgotten why we don't simply set TIF_NEED_RESCHED if we
> need the timer re-programmed. That is by far the simplest fix.
> 
> I'm sure we talked about it, but this was a long time ago and I can't
> remember :/

I don't think we did but the rationale is that with forcing setting
TIF_NEED_RESCHED, you force a needless timer restart (which is then going
to be cancelled shortly after) and a round trip to the scheduler with the
rq lock overhead, etc...

Just for the fun I just tried the following change:

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index c52c2eba7c73..ec43d135cf65 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1132,8 +1132,10 @@ static void wake_up_idle_cpu(int cpu)
 {
 	struct rq *rq = cpu_rq(cpu);
 
-	if (cpu == smp_processor_id())
+	if (cpu == smp_processor_id()) {
+		set_tsk_need_resched(current);
 		return;
+	}
 
 	if (set_nr_and_not_polling(rq->idle))
 		smp_send_reschedule(cpu);


Then I computed the average of 100 runs of "make clean -C tools/perf; make -C
tools/perf/" before and after this patch.

I observed an average regression of 1.27% less time spent in C-states.

So this has a measurable impact.

> 
> Anyway, the patches look good, except I think there's a whole bunch of
> architectures that still need fixing. In particular since loongson
> 'borrowed' the whole lot from MIPS, they need an identical fix. But I'm
> sure there's more architectures affected.

MIPS at least yes, I only did a quick check and it seems that most archs
use a "wfi" like instruction. I'll check for others.

Thanks.