Sapphire Rapids C0.x idle states support

From: Artem Bityutskiy <[email protected]>

Rename 'intel_idle_hlt_irq_on()' to 'intel_idle_hlt_irq()' for consistency with 'intel_idle_irq()'.

While on it, fix indentation in 'intel_idle_hlt_irq()' declaration to use tabs instead of white-spaces.

Signed-off-by Artem Bityutskiy <[email protected]>

---
 drivers/idle/intel_idle.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/idle/intel_idle.c +5 -6

index b930036edbbe..0a835f97de72 100644
@@ -224,8 +224,8 @@ static __cpuidle int intel_idle_hlt(struct cpuidle_device *dev,
        return __intel_idle_hlt(dev, drv, index);
 }
 
-static __cpuidle int intel_idle_hlt_irq_on(struct cpuidle_device *dev,
-                                   struct cpuidle_driver *drv, int index)
+static __cpuidle int intel_idle_hlt_irq(struct cpuidle_device *dev,
+                                       struct cpuidle_driver *drv, int index)
 {
        int ret;
 

@@ -1900,11 +1900,11 @@ static void state_update_enter_method(struct cpuidle_state *state, int cstate)
        if (state->enter == intel_idle_hlt) {
                if (force_irq_on) {
                        pr_info("forced intel_idle_irq for state %d\n", cstate);
-                       state->enter = intel_idle_hlt_irq_on;
+                       state->enter = intel_idle_hlt_irq;
                }
                return;
        }
-       if (state->enter == intel_idle_hlt_irq_on)
+       if (state->enter == intel_idle_hlt_irq)
                return; /* no update scenarios */
 
        if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {

@@ -1949,7 +1949,7 @@ static bool should_verify_mwait(struct cpuidle_state *state)
 {
        if (state->enter == intel_idle_hlt)
                return false;
-       if (state->enter == intel_idle_hlt_irq_on)
+       if (state->enter == intel_idle_hlt_irq)
                return false;
 
        return true;
--

2.40.1

Quoted 9 lines · Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

Rename 'intel_idle_hlt_irq_on()' to 'intel_idle_hlt_irq()' for consistency with 'intel_idle_irq()'.

While on it, fix indentation in 'intel_idle_hlt_irq()' declaration to use tabs instead of white-spaces.

Quoted 1 line

Signed-off-by Artem Bityutskiy <[email protected]>

I think I can apply this right away without the rest of the series as it doesn't seem to depend on the other patches.

Any concerns regarding this?

Any concerns regarding this?

Quoted 9 lines · Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

Rename 'intel_idle_hlt_irq_on()' to 'intel_idle_hlt_irq()' for consistency with 'intel_idle_irq()'.

While on it, fix indentation in 'intel_idle_hlt_irq()' declaration to use tabs instead of white-spaces.

Quoted 1 line

Signed-off-by Artem Bityutskiy <[email protected]>

I think I can apply this right away without the rest of the series as it doesn't seem to depend on the other patches.

Any concerns regarding this?

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

Rename 'intel_idle_hlt_irq_on()' to 'intel_idle_hlt_irq()' for consistency with 'intel_idle_irq()'.

While on it, fix indentation in 'intel_idle_hlt_irq()' declaration to use tabs instead of white-spaces.

Signed-off-by Artem Bityutskiy <[email protected]>

Quoted 4 lines

I think I can apply this right away without the rest of the series as it doesn't seem to depend on the other patches.

Any concerns regarding this?

Yeah, as per:

   https://lkml.kernel.org/r/20230711132553.GN3062772%40hirez.programming.kicks-ass.net

Quoted 16 lines · Peter Zijlstra

Quoted 15 lines · Rafael J. Wysocki

Quoted 10 lines · Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

Rename 'intel_idle_hlt_irq_on()' to 'intel_idle_hlt_irq()' for consistency with 'intel_idle_irq()'.

While on it, fix indentation in 'intel_idle_hlt_irq()' declaration to use tabs instead of white-spaces.

Signed-off-by Artem Bityutskiy <[email protected]>

I think I can apply this right away without the rest of the series as it doesn't seem to depend on the other patches.

Quoted 5 lines

Quoted 1 line

Any concerns regarding this?

Yeah, as per:

   https://lkml.kernel.org/r/20230711132553.GN3062772%40hirez.programming.kicks-ass.net

and it's going to get deleted - renaming is harmless for that

From: Artem Bityutskiy <[email protected]>

Background
----------

Idle states reduce power consumption when a CPU has no work to do. The most shallow CPU idle state is "POLL". It has lowest wake up latency, but saves little power. The next idle state on Intel platforms is "C1". It has has higher latency, but saves more power than "POLL".

Sapphire Rapids Xeons add new C0.1 and C0.2 (later C0.x) idle states which conceptually sit between "POLL" and "C1". These provide a very attractive
midpoint: near-POLL wake-up latency and power consumption halfway between "POLL" and "C1".

In other words, the expectation is that most latency-sensitive users will prefer C0.x over POLL.

Enable C0.2 idle state support on Sapphire Rapids Xeon (later - SPR) by adding it between POLL and C1.

Base commit
-----------

Based on the "linux-next" branch of "linux-pm" git tree.

base-commit: bd9bb08847da3b1eba2ea8cebf514d9287e7f4fb

Changelog
---------

* v4:
- Address issues pointed out by Thomas Gleixner.
. mwait.h: use 'IS_ENABLED()' instead of '#ifdef'.
. mwait.h: use '__always_inline'.
. mwait.h: use inline stub instead for macro for "!CONFIG_X86_64" case. . mwait.h: use proper commentaries for '#endif' and '#else'. . mwait.h: tested with llvm/clang.
. Use imperative form (removed "this patch").
- intel_idle: rename 'intel_idle_hlt_irq_on()' for consistency.

* v3
- Dropped patch 'x86/umwait: Increase tpause and umwait quanta' after, as suggested by Andy Lutomirski.
- Followed Peter Zijlstra's suggestion and removed explicit 'umwait' deadline. Rely on the global implicit deadline instead.
- Rebased on top of Arjan's patches.
- C0.2 was tested in a VM by Arjan van de Ven.
- Re-measured on 2S and 4S Sapphire Rapids Xeon.
* v2
- Do not mix 'raw_local_irq_enable()' and 'local_irq_disable()'. I failed to directly verify it, but I believe it'll address the '.noinstr.text' warning.
- Minor kerneldoc commentary fix.

C0.2 vs POLL latency and power
------------------------------

I compared POLL to C0.2 using 'wult' tool (https://github.com/intel/wult),
which measures idle states latency.

* In "POLL" experiments, all C-states except for POLL were disabled.
* In "C0.2" experiments, all C-states except for POLL and C0.2 were disabled.

Here are the measurement results. The numbers are percent change from POLL to C0.2.

-----------|-----------|----------|-----------
 Median IR | 99th % IR | AC Power | RAPL power
-----------|-----------|----------|-----------
 24%       | 12%       | -13%     | -18%
-----------------------|----------|-----------

* IR stands for interrupt latency. The table provides the median and 99th percentile. Wult measures it as the delay between the moment a timer interrupt fires to the moment the CPU reaches the interrupt handler.
* AC Power is the wall socket AC power.
* RAPL power is the CPU package power, measured using the 'turbostat' tool.

Hackbench measurements
----------------------

I ran the 'hackbench' benchmark using the following commands:

# 4 groups, 200 threads
hackbench -s 128 -l 100000000 -g4 -f 25 -P
# 8 groups, 400 threads.
hackbench -s 128 -l 100000000 -g8 -f 25 -P

My SPR system has 224 CPUs, so the first command did not use all CPUs, the second command used all of them. However, in both cases CPU power reached TDP.

I ran hackbench 5 times for every configuration and compared hackbench "score" averages.

In case of 4 groups, C0.2 improved the score by about 4%, and in case of 8 groups by about 0.6%.

Q&A
---

1. Can C0.2 be disabled?

C0.2 can be disabled via sysfs and with the following kernel boot option:

intel_idle.states_off=2

2. Why C0.2, not C0.1?

I measured both C0.1 and C0.2, but did not notice a clear C0.1 advantage in terms of latency, but did notice that C0.2 saves more power.

But if users want to try using C0.1 instead of C0.2, they can do this:

echo 0 > /sys/devices/system/cpu/umwait_control/enable_c02

This will make sure that C0.2 requests from 'intel_idle' are automatically converted to C0.1 requests.

3. How did you verify that system enters C0.2?

I used 'perf' to read the corresponding PMU counters:

perf stat -e CPU_CLK_UNHALTED.C01,CPU_CLK_UNHALTED.C02,cycles -a sleep 1

4. Ho to change the global explicit 'umwait' deadline?

Via '/sys/devices/system/cpu/umwait_control/max_time'

Artem Bityutskiy (4):
x86/umwait: use 'IS_ENABLED()'
x86/mwait: Add support for idle via umwait
intel_idle: rename 'intel_idle_hlt_irq_on()'
intel_idle: add C0.2 state for Sapphire Rapids Xeon

 arch/x86/include/asm/mwait.h | 85 ++++++++++++++++++++++++++++++++----
 drivers/idle/intel_idle.c    | 52 +++++++++++++++++++---
 2 files changed, 123 insertions(+), 14 deletions(-)

--
2.40.1

Artem Bityutskiy (4):
  x86/umwait: use 'IS_ENABLED()'
  x86/mwait: Add support for idle via umwait

  intel_idle: rename 'intel_idle_hlt_irq_on()'
  intel_idle: add C0.2 state for Sapphire Rapids Xeon

Hi,

Quoted 3 lines · Artem Bityutskiy

Artem Bityutskiy (4):
  x86/umwait: use 'IS_ENABLED()'
  x86/mwait: Add support for idle via umwait

If these 2 patches are OK now, is there a chance to get them merged?

Quoted 8 lines · Artem Bityutskiy

Hi,

Quoted 3 lines · Artem Bityutskiy

Artem Bityutskiy (4):
x86/umwait: use 'IS_ENABLED()'
x86/mwait: Add support for idle via umwait

Quoted 1 line

If these 2 patches are OK now, is there a chance to get them merged?

Hi, I wonder if these 2 patches have any chances to get merged?

May be there is something I am expected to do have not done? If so, please, accept my apologies and give me a direction.

Quoted 12 lines · Artem Bityutskiy

Quoted 9 lines · Artem Bityutskiy

Hi,

Quoted 3 lines · Artem Bityutskiy

Artem Bityutskiy (4):
  x86/umwait: use 'IS_ENABLED()'
  x86/mwait: Add support for idle via umwait

If these 2 patches are OK now, is there a chance to get them merged?

Quoted 4 lines

Hi, I wonder if these 2 patches have any chances to get merged?

May be there is something I am expected to do have not done? If so, please, accept my apologies and give me a direction.

I think that they were based on the Arjan\s changes that got dropped. If so, they need to be rebased and resent.

Quoted 18 lines · Rafael J. Wysocki

Quoted 16 lines · Artem Bityutskiy

Quoted 9 lines · Artem Bityutskiy

Hi,

Quoted 3 lines · Artem Bityutskiy

Artem Bityutskiy (4):
x86/umwait: use 'IS_ENABLED()'
x86/mwait: Add support for idle via umwait

If these 2 patches are OK now, is there a chance to get them merged?

Hi, I wonder if these 2 patches have any chances to get merged?

May be there is something I am expected to do have not done? If so, please, accept my apologies and give me a direction.

Quoted 2 lines

I think that they were based on the Arjan\s changes that got dropped. If so, they need to be rebased and resent.

The above quoted 2 patches are x86 bits, they introduce the basic umwait primitives. They are not based on Arjan's patches. They still apply cleanly, and do not need a refresh, as far as I can see.

I was hoping to get arch/x86 bits merged.

Then I would send the refreshed version of the intel_idle patches.

From: Artem Bityutskiy <[email protected]>

Background
----------

Idle states reduce power consumption when a CPU has no work to do. The most shallow CPU idle state is "POLL". It has lowest wake up latency, but saves little power. The next idle state on Intel platforms is "C1". It has has higher latency, but saves more power than "POLL".

Sapphire Rapids Xeons add new C0.1 and C0.2 (later C0.x) idle states which conceptually sit between "POLL" and "C1". These provide a very attractive
midpoint: near-POLL wake-up latency and power consumption halfway between "POLL" and "C1".

In other words, the expectation is that most latency-sensitive users will prefer C0.x over POLL.

Enable C0.2 idle state support on Sapphire Rapids Xeon (later - SPR) by adding it between POLL and C1.

Base commit
-----------

Based on the "linux-next" branch of "linux-pm" git tree.

base-commit: bd9bb08847da3b1eba2ea8cebf514d9287e7f4fb

Changelog
---------

* v4:
- Address issues pointed out by Thomas Gleixner.
. mwait.h: use 'IS_ENABLED()' instead of '#ifdef'.
. mwait.h: use '__always_inline'.
. mwait.h: use inline stub instead for macro for "!CONFIG_X86_64" case. . mwait.h: use proper commentaries for '#endif' and '#else'.
. mwait.h: tested with llvm/clang.
. Use imperative form (removed "this patch").
- intel_idle: rename 'intel_idle_hlt_irq_on()' for consistency.
* v3
- Dropped patch 'x86/umwait: Increase tpause and umwait quanta' after, as suggested by Andy Lutomirski.
- Followed Peter Zijlstra's suggestion and removed explicit 'umwait' deadline. Rely on the global implicit deadline instead.
- Rebased on top of Arjan's patches.
- C0.2 was tested in a VM by Arjan van de Ven.
- Re-measured on 2S and 4S Sapphire Rapids Xeon.
* v2
- Do not mix 'raw_local_irq_enable()' and 'local_irq_disable()'. I failed to directly verify it, but I believe it'll address the '.noinstr.text' warning.
- Minor kerneldoc commentary fix.

C0.2 vs POLL latency and power
------------------------------

I compared POLL to C0.2 using 'wult' tool (https://github.com/intel/wult),
which measures idle states latency.

* In "POLL" experiments, all C-states except for POLL were disabled.
* In "C0.2" experiments, all C-states except for POLL and C0.2 were disabled.

Here are the measurement results. The numbers are percent change from POLL to C0.2.

-----------|-----------|----------|-----------
Median IR | 99th % IR | AC Power | RAPL power
-----------|-----------|----------|-----------
24% | 12% | -13% | -18%
-----------------------|----------|-----------

* IR stands for interrupt latency. The table provides the median and 99th percentile. Wult measures it as the delay between the moment a timer interrupt fires to the moment the CPU reaches the interrupt handler.
* AC Power is the wall socket AC power.
* RAPL power is the CPU package power, measured using the 'turbostat' tool.

Hackbench measurements
----------------------

I ran the 'hackbench' benchmark using the following commands:

# 4 groups, 200 threads
hackbench -s 128 -l 100000000 -g4 -f 25 -P
# 8 groups, 400 threads.
hackbench -s 128 -l 100000000 -g8 -f 25 -P

My SPR system has 224 CPUs, so the first command did not use all CPUs, the second command used all of them. However, in both cases CPU power reached TDP.

I ran hackbench 5 times for every configuration and compared hackbench "score" averages.

In case of 4 groups, C0.2 improved the score by about 4%, and in case of 8 groups by about 0.6%.

Q&A
---

1. Can C0.2 be disabled?

C0.2 can be disabled via sysfs and with the following kernel boot option:

intel_idle.states_off=2

2. Why C0.2, not C0.1?

I measured both C0.1 and C0.2, but did not notice a clear C0.1 advantage in terms of latency, but did notice that C0.2 saves more power.

But if users want to try using C0.1 instead of C0.2, they can do this:

echo 0 > /sys/devices/system/cpu/umwait_control/enable_c02

This will make sure that C0.2 requests from 'intel_idle' are automatically converted to C0.1 requests.

3. How did you verify that system enters C0.2?

I used 'perf' to read the corresponding PMU counters:

perf stat -e CPU_CLK_UNHALTED.C01,CPU_CLK_UNHALTED.C02,cycles -a sleep 1

4. Ho to change the global explicit 'umwait' deadline?

Via '/sys/devices/system/cpu/umwait_control/max_time'

Artem Bityutskiy (4):
x86/umwait: use 'IS_ENABLED()'
x86/mwait: Add support for idle via umwait
intel_idle: rename 'intel_idle_hlt_irq_on()'
intel_idle: add C0.2 state for Sapphire Rapids Xeon

arch/x86/include/asm/mwait.h | 85 ++++++++++++++++++++++++++++++++----
drivers/idle/intel_idle.c | 52 +++++++++++++++++++---

2 files changed, 123 insertions(+), 14 deletions(-)

Hey Artem, Linux PM list,
I stumbled upon this patch set from a while back, and saw that it looks like this was the last swing at it. The data looks compelling, was wondering if there was any appetite to pick this back up?

If you’d like, I’d be happy to reignite this work if it hadn’t been killed intentionally to take it off the collective plate?