All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v3 0/3] Sapphire Rapids C0.x idle states support
@ 2023-06-10 18:35 Artem Bityutskiy
  2023-06-10 18:35 ` [PATCH v3 1/2] x86/mwait: Add support for idle via umwait Artem Bityutskiy
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Artem Bityutskiy @ 2023-06-10 18:35 UTC (permalink / raw
  To: x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven, Artem Bityutskiy

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

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 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, we expect all but the most latency-sensitive users to prefer
these idle state over POLL.

This patch-set enables C0.2 idle state support on Sapphire Rapids Xeon (later -
SPR). The new idle state is added between POLL and C1.

Patch-set overview
------------------

This patch-set is based on the "linux-next" branch of the "linux-pm" plus
patches from Arjan van de Ven, submitted to linux-pm mailing list
on Jun 5, 2023:
 * Cover letter: [PATCH 0/4 v2] Add support for running in VM guests to intel_idle
 * https://patchwork.kernel.org/project/linux-pm/patch/20230605154716.840930-2-arjan@linux.intel.com/

In other words, the base commit is 'e8195eaff86fd2ddb5f00646b5f76e40cd1164a8',
then Arjan's patches should be applied, and then these patches on top.

Patch #1 does not depend on Arjan's patches, but patch #2 requires the cleanups
from Arjan's patch-set.

Changelog
---------

* 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 (2):
  x86/mwait: Add support for idle via umwait
  intel_idle: add C0.2 state for Sapphire Rapids Xeon

 arch/x86/include/asm/mwait.h | 65 ++++++++++++++++++++++++++++++++++++
 drivers/idle/intel_idle.c    | 44 +++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 1 deletion(-)

-- 
2.40.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
  2023-06-10 18:35 [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Artem Bityutskiy
@ 2023-06-10 18:35 ` Artem Bityutskiy
  2023-06-29 19:03   ` Rafael J. Wysocki
  2023-06-29 22:04   ` Thomas Gleixner
  2023-06-10 18:35 ` [PATCH v3 2/2] intel_idle: add C0.2 state for Sapphire Rapids Xeon Artem Bityutskiy
  2023-06-29 22:05 ` [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Thomas Gleixner
  2 siblings, 2 replies; 9+ messages in thread
From: Artem Bityutskiy @ 2023-06-10 18:35 UTC (permalink / raw
  To: x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven, Artem Bityutskiy

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

On Intel platforms, C-states are requested using the 'monitor/mwait'
instructions pair, as implemented in 'mwait_idle_with_hints()'. This
mechanism allows for entering C1 and deeper C-states.

Sapphire Rapids Xeon supports new idle states - C0.1 and C0.2 (later C0.x).
These idle states have lower latency comparing to C1, and can be requested
with either 'tpause' and 'umwait' instructions.

Linux already uses the 'tpause' instruction in delay functions like
'udelay()'. This patch adds 'umwait' and 'umonitor' instructions support.

'umwait' and 'tpause' instructions are very similar - both send the CPU to
C0.x and have the same break out rules. But unlike 'tpause', 'umwait' works
together with 'umonitor' and exits the C0.x when the monitored memory
address is modified (similar idea as with 'monitor/mwait').

This patch implements the 'umwait_idle()' function, which works very
similarly to existing 'mwait_idle_with_hints()', but requests C0.x. The
intention is to use it from the 'intel_idle' driver.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 arch/x86/include/asm/mwait.h | 65 ++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
index 778df05f8539..681c281eeaa7 100644
--- a/arch/x86/include/asm/mwait.h
+++ b/arch/x86/include/asm/mwait.h
@@ -141,4 +141,69 @@ static inline void __tpause(u32 ecx, u32 edx, u32 eax)
 	#endif
 }
 
+#ifdef CONFIG_X86_64
+/*
+ * Monitor a memory address at 'rcx' using the 'umonitor' instruction.
+ */
+static inline void __umonitor(const void *rcx)
+{
+	/* "umonitor %rcx" */
+#ifdef CONFIG_AS_TPAUSE
+	asm volatile("umonitor %%rcx\n"
+		     :
+		     : "c"(rcx));
+#else
+	asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf1\t\n"
+		     :
+		     : "c"(rcx));
+#endif
+}
+
+/*
+ * Same as '__tpause()', but uses the 'umwait' instruction. It is very
+ * similar to 'tpause', but also breaks out if the data at the address
+ * monitored with 'umonitor' is modified.
+ */
+static inline void __umwait(u32 ecx, u32 edx, u32 eax)
+{
+	/* "umwait %ecx, %edx, %eax;" */
+#ifdef CONFIG_AS_TPAUSE
+	asm volatile("umwait %%ecx\n"
+		     :
+		     : "c"(ecx), "d"(edx), "a"(eax));
+#else
+	asm volatile(".byte 0xf2, 0x0f, 0xae, 0xf1\t\n"
+		     :
+		     : "c"(ecx), "d"(edx), "a"(eax));
+#endif
+}
+
+/*
+ * Enter C0.1 or C0.2 state and stay there until an event happens (an interrupt
+ * or the 'need_resched()'), the explicit deadline is reached, or the implicit
+ * global limit is reached.
+ *
+ * The deadline is the absolute TSC value to exit the idle state at. If it
+ * exceeds the global limit in the 'IA32_UMWAIT_CONTROL' register, the global
+ * limit prevails, and the idle state is exited earlier than the deadline.
+ */
+static inline void umwait_idle(u64 deadline, u32 state)
+{
+	if (!current_set_polling_and_test()) {
+		u32 eax, edx;
+
+		eax = lower_32_bits(deadline);
+		edx = upper_32_bits(deadline);
+
+		__umonitor(&current_thread_info()->flags);
+		if (!need_resched())
+			__umwait(state, edx, eax);
+	}
+	current_clr_polling();
+}
+#else
+#define umwait_idle(deadline, state) \
+		WARN_ONCE(1, "umwait CPU instruction is not supported")
+#endif /* CONFIG_X86_64 */
+
 #endif /* _ASM_X86_MWAIT_H */
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v3 2/2] intel_idle: add C0.2 state for Sapphire Rapids Xeon
  2023-06-10 18:35 [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Artem Bityutskiy
  2023-06-10 18:35 ` [PATCH v3 1/2] x86/mwait: Add support for idle via umwait Artem Bityutskiy
@ 2023-06-10 18:35 ` Artem Bityutskiy
  2023-06-29 22:05 ` [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Thomas Gleixner
  2 siblings, 0 replies; 9+ messages in thread
From: Artem Bityutskiy @ 2023-06-10 18:35 UTC (permalink / raw
  To: x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven, Artem Bityutskiy

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Add Sapphire Rapids Xeon C0.2 state support. This state has a lower exit
latency comparing to C1, and saves energy comparing to POLL.

C0.2 may also improve performance (e.g., as measured by 'hackbench'), because
idle CPU power savings in C0.2 increase busy CPU power budget and therefore,
improve turbo boost of the busy CPU.

Suggested-by: Len Brown <len.brown@intel.com>
Suggested-by: Arjan Van De Ven <arjan.van.de.ven@intel.com>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/idle/intel_idle.c | 44 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/idle/intel_idle.c b/drivers/idle/intel_idle.c
index 0bf5e9f5bed8..51f56001e2cd 100644
--- a/drivers/idle/intel_idle.c
+++ b/drivers/idle/intel_idle.c
@@ -130,6 +130,11 @@ static unsigned int mwait_substates __initdata;
 #define flg2MWAIT(flags) (((flags) >> 24) & 0xFF)
 #define MWAIT2flg(eax) ((eax & 0xFF) << 24)
 
+/*
+ * The maximum possible 'umwait' deadline value.
+ */
+#define UMWAIT_MAX_DEADLINE (~((u64)0))
+
 static __always_inline int __intel_idle(struct cpuidle_device *dev,
 					struct cpuidle_driver *drv, int index)
 {
@@ -263,6 +268,32 @@ static __cpuidle int intel_idle_s2idle(struct cpuidle_device *dev,
 	return 0;
 }
 
+/**
+ * intel_idle_umwait_irq - Request C0.x using the 'umwait' instruction.
+ * @dev: cpuidle device of the target CPU.
+ * @drv: cpuidle driver (assumed to point to intel_idle_driver).
+ * @index: Target idle state index.
+ *
+ * Request C0.1 or C0.2 using 'umwait' instruction with interrupts enabled.
+ */
+static __cpuidle int intel_idle_umwait_irq(struct cpuidle_device *dev,
+					   struct cpuidle_driver *drv,
+					   int index)
+{
+	u32 state = flg2MWAIT(drv->states[index].flags);
+
+	raw_local_irq_enable();
+	/*
+	 * Use the maximum possible deadline value. This means that 'C0.x'
+	 * residency will be limited by the global limit in
+	 * 'IA32_UMWAIT_CONTROL'.
+	 */
+	umwait_idle(UMWAIT_MAX_DEADLINE, state);
+	raw_local_irq_disable();
+
+	return index;
+}
+
 /*
  * States are indexed by the cstate number,
  * which is also the index into the MWAIT hint array.
@@ -1006,6 +1037,13 @@ static struct cpuidle_state adl_n_cstates[] __initdata = {
 };
 
 static struct cpuidle_state spr_cstates[] __initdata = {
+	{
+		.name = "C0.2",
+		.desc = "UMWAIT C0.2",
+		.flags = MWAIT2flg(TPAUSE_C02_STATE) | CPUIDLE_FLAG_IRQ_ENABLE,
+		.exit_latency_ns = 200,
+		.target_residency_ns = 200,
+		.enter = &intel_idle_umwait_irq, },
 	{
 		.name = "C1",
 		.desc = "MWAIT 0x00",
@@ -1904,7 +1942,9 @@ static void state_update_enter_method(struct cpuidle_state *state, int cstate)
 		}
 		return;
 	}
-	if (state->enter == intel_idle_hlt_irq_on)
+
+	if (state->enter == intel_idle_hlt_irq_on ||
+	    state->enter == intel_idle_umwait_irq)
 		return; /* no update scenarios */
 
 	if (state->flags & CPUIDLE_FLAG_INIT_XSTATE) {
@@ -1959,6 +1999,8 @@ static bool should_verify_mwait(struct cpuidle_state *state)
 		return false;
 	if (state->enter == intel_idle_hlt_irq_on)
 		return false;
+	if (state->enter == intel_idle_umwait_irq)
+		return false;
 
 	return true;
 }
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
  2023-06-10 18:35 ` [PATCH v3 1/2] x86/mwait: Add support for idle via umwait Artem Bityutskiy
@ 2023-06-29 19:03   ` Rafael J. Wysocki
  2023-06-29 22:04   ` Thomas Gleixner
  1 sibling, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2023-06-29 19:03 UTC (permalink / raw
  To: Artem Bityutskiy, x86
  Cc: Rafael J. Wysocki, Linux PM Mailing List, Arjan van de Ven

On Sat, Jun 10, 2023 at 8:35 PM Artem Bityutskiy <dedekind1@gmail.com> wrote:
>
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>
> On Intel platforms, C-states are requested using the 'monitor/mwait'
> instructions pair, as implemented in 'mwait_idle_with_hints()'. This
> mechanism allows for entering C1 and deeper C-states.
>
> Sapphire Rapids Xeon supports new idle states - C0.1 and C0.2 (later C0.x).
> These idle states have lower latency comparing to C1, and can be requested
> with either 'tpause' and 'umwait' instructions.
>
> Linux already uses the 'tpause' instruction in delay functions like
> 'udelay()'. This patch adds 'umwait' and 'umonitor' instructions support.
>
> 'umwait' and 'tpause' instructions are very similar - both send the CPU to
> C0.x and have the same break out rules. But unlike 'tpause', 'umwait' works
> together with 'umonitor' and exits the C0.x when the monitored memory
> address is modified (similar idea as with 'monitor/mwait').
>
> This patch implements the 'umwait_idle()' function, which works very
> similarly to existing 'mwait_idle_with_hints()', but requests C0.x. The
> intention is to use it from the 'intel_idle' driver.
>
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

x86 folks, any comments on this?

Barring any concerns, I would like to queue it up for 6.6 when the
merge is over.

> ---
>  arch/x86/include/asm/mwait.h | 65 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
>
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index 778df05f8539..681c281eeaa7 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -141,4 +141,69 @@ static inline void __tpause(u32 ecx, u32 edx, u32 eax)
>         #endif
>  }
>
> +#ifdef CONFIG_X86_64
> +/*
> + * Monitor a memory address at 'rcx' using the 'umonitor' instruction.
> + */
> +static inline void __umonitor(const void *rcx)
> +{
> +       /* "umonitor %rcx" */
> +#ifdef CONFIG_AS_TPAUSE
> +       asm volatile("umonitor %%rcx\n"
> +                    :
> +                    : "c"(rcx));
> +#else
> +       asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf1\t\n"
> +                    :
> +                    : "c"(rcx));
> +#endif
> +}
> +
> +/*
> + * Same as '__tpause()', but uses the 'umwait' instruction. It is very
> + * similar to 'tpause', but also breaks out if the data at the address
> + * monitored with 'umonitor' is modified.
> + */
> +static inline void __umwait(u32 ecx, u32 edx, u32 eax)
> +{
> +       /* "umwait %ecx, %edx, %eax;" */
> +#ifdef CONFIG_AS_TPAUSE
> +       asm volatile("umwait %%ecx\n"
> +                    :
> +                    : "c"(ecx), "d"(edx), "a"(eax));
> +#else
> +       asm volatile(".byte 0xf2, 0x0f, 0xae, 0xf1\t\n"
> +                    :
> +                    : "c"(ecx), "d"(edx), "a"(eax));
> +#endif
> +}
> +
> +/*
> + * Enter C0.1 or C0.2 state and stay there until an event happens (an interrupt
> + * or the 'need_resched()'), the explicit deadline is reached, or the implicit
> + * global limit is reached.
> + *
> + * The deadline is the absolute TSC value to exit the idle state at. If it
> + * exceeds the global limit in the 'IA32_UMWAIT_CONTROL' register, the global
> + * limit prevails, and the idle state is exited earlier than the deadline.
> + */
> +static inline void umwait_idle(u64 deadline, u32 state)
> +{
> +       if (!current_set_polling_and_test()) {
> +               u32 eax, edx;
> +
> +               eax = lower_32_bits(deadline);
> +               edx = upper_32_bits(deadline);
> +
> +               __umonitor(&current_thread_info()->flags);
> +               if (!need_resched())
> +                       __umwait(state, edx, eax);
> +       }
> +       current_clr_polling();
> +}
> +#else
> +#define umwait_idle(deadline, state) \
> +               WARN_ONCE(1, "umwait CPU instruction is not supported")
> +#endif /* CONFIG_X86_64 */
> +
>  #endif /* _ASM_X86_MWAIT_H */
> --
> 2.40.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
  2023-06-10 18:35 ` [PATCH v3 1/2] x86/mwait: Add support for idle via umwait Artem Bityutskiy
  2023-06-29 19:03   ` Rafael J. Wysocki
@ 2023-06-29 22:04   ` Thomas Gleixner
  2023-06-29 22:36     ` Thomas Gleixner
                       ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: Thomas Gleixner @ 2023-06-29 22:04 UTC (permalink / raw
  To: Artem Bityutskiy, x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven, Artem Bityutskiy

On Sat, Jun 10 2023 at 21:35, Artem Bityutskiy wrote:
> On Intel platforms, C-states are requested using the 'monitor/mwait'
> instructions pair, as implemented in 'mwait_idle_with_hints()'. This
> mechanism allows for entering C1 and deeper C-states.
>
> Sapphire Rapids Xeon supports new idle states - C0.1 and C0.2 (later C0.x).
> These idle states have lower latency comparing to C1, and can be requested
> with either 'tpause' and 'umwait' instructions.

s/and/or/

>
> Linux already uses the 'tpause' instruction in delay functions like
> 'udelay()'. This patch adds 'umwait' and 'umonitor' instructions
> support.

# git grep 'This patch' Documentation/process/

Please fix it all over the place.

> +#ifdef CONFIG_X86_64
> +/*
> + * Monitor a memory address at 'rcx' using the 'umonitor' instruction.
> + */
> +static inline void __umonitor(const void *rcx)
> +{
> +	/* "umonitor %rcx" */
> +#ifdef CONFIG_AS_TPAUSE

Are you sure that the instruction check for TPAUSE is sufficient to also
include UMONITOR on all toolchains which support TPAUSE?

Also:

          if (IS_ENABLED(CONFIG_AS_TPAUSE) {

> +	asm volatile("umonitor %%rcx\n"
> +		     :
> +		     : "c"(rcx));
          } else {

> +#else
> +	asm volatile(".byte 0xf3, 0x0f, 0xae, 0xf1\t\n"
> +		     :
> +		     : "c"(rcx));

        }

please.

> +/*
> + * Same as '__tpause()', but uses the 'umwait' instruction. It is very
> + * similar to 'tpause', but also breaks out if the data at the address
> + * monitored with 'umonitor' is modified.
> + */
> +static inline void __umwait(u32 ecx, u32 edx, u32 eax)
> +{
> +	/* "umwait %ecx, %edx, %eax;" */
> +#ifdef CONFIG_AS_TPAUSE
> +	asm volatile("umwait %%ecx\n"
> +		     :
> +		     : "c"(ecx), "d"(edx), "a"(eax));
> +#else
> +	asm volatile(".byte 0xf2, 0x0f, 0xae, 0xf1\t\n"
> +		     :
> +		     : "c"(ecx), "d"(edx), "a"(eax));
> +#endif

Ditto.

> +/*
> + * Enter C0.1 or C0.2 state and stay there until an event happens (an interrupt
> + * or the 'need_resched()'), the explicit deadline is reached, or the implicit
> + * global limit is reached.
> + *
> + * The deadline is the absolute TSC value to exit the idle state at. If it
> + * exceeds the global limit in the 'IA32_UMWAIT_CONTROL' register, the global
> + * limit prevails, and the idle state is exited earlier than the deadline.
> + */
> +static inline void umwait_idle(u64 deadline, u32 state)
> +{
> +	if (!current_set_polling_and_test()) {
> +		u32 eax, edx;
> +
> +		eax = lower_32_bits(deadline);
> +		edx = upper_32_bits(deadline);
> +
> +		__umonitor(&current_thread_info()->flags);
> +		if (!need_resched())
> +			__umwait(state, edx, eax);
> +	}
> +	current_clr_polling();
> +}
> +#else
> +#define umwait_idle(deadline, state) \
> +		WARN_ONCE(1, "umwait CPU instruction is not supported")

Please implement the stub as a proper inline.

> +#endif /* CONFIG_X86_64 */

This comment is wrong.

#ifdef CONFIG_X86_64
       ....
#else /* CONFIG_X86_64 */

#endif /* !CONFIG_X86_64 */

makes it clear what the scope is.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 0/3] Sapphire Rapids C0.x idle states support
  2023-06-10 18:35 [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Artem Bityutskiy
  2023-06-10 18:35 ` [PATCH v3 1/2] x86/mwait: Add support for idle via umwait Artem Bityutskiy
  2023-06-10 18:35 ` [PATCH v3 2/2] intel_idle: add C0.2 state for Sapphire Rapids Xeon Artem Bityutskiy
@ 2023-06-29 22:05 ` Thomas Gleixner
  2 siblings, 0 replies; 9+ messages in thread
From: Thomas Gleixner @ 2023-06-29 22:05 UTC (permalink / raw
  To: Artem Bityutskiy, x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven, Artem Bityutskiy

On Sat, Jun 10 2023 at 21:35, Artem Bityutskiy wrote:

Something is wrong with your patch mail script.

          [PATCH v3 0/3]

but then the actual patches are

          [PATCH v3 1/2]
          [PATCH v3 2/2]

Something does not add up here.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
  2023-06-29 22:04   ` Thomas Gleixner
@ 2023-06-29 22:36     ` Thomas Gleixner
  2023-06-30 16:54     ` Artem Bityutskiy
  2023-07-07 17:13     ` Artem Bityutskiy
  2 siblings, 0 replies; 9+ messages in thread
From: Thomas Gleixner @ 2023-06-29 22:36 UTC (permalink / raw
  To: Artem Bityutskiy, x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven, Artem Bityutskiy

On Fri, Jun 30 2023 at 00:04, Thomas Gleixner wrote:
>> +static inline void __umonitor(const void *rcx)

These inlines must be __always_inline to prevent the compiler from
instrumenting this.

For further information see:

  0e985e9d2286 ("cpuidle: Add comments about noinstr/__cpuidle usage")
  2b5a0e425e6e ("objtool/idle: Validate __cpuidle code as noinstr")

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
  2023-06-29 22:04   ` Thomas Gleixner
  2023-06-29 22:36     ` Thomas Gleixner
@ 2023-06-30 16:54     ` Artem Bityutskiy
  2023-07-07 17:13     ` Artem Bityutskiy
  2 siblings, 0 replies; 9+ messages in thread
From: Artem Bityutskiy @ 2023-06-30 16:54 UTC (permalink / raw
  To: Thomas Gleixner, x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven

On Fri, 2023-06-30 at 00:04 +0200, Thomas Gleixner wrote:
> > +#ifdef CONFIG_X86_64
> > +/*
> > + * Monitor a memory address at 'rcx' using the 'umonitor' instruction.
> > + */
> > +static inline void __umonitor(const void *rcx)
> > +{
> > +       /* "umonitor %rcx" */
> > +#ifdef CONFIG_AS_TPAUSE
> 
> Are you sure that the instruction check for TPAUSE is sufficient to also
> include UMONITOR on all toolchains which support TPAUSE?

Good point, I checked only GNU compiler. I can check Clang/LLVM.


Will also address other issues that you pointed, thanks!

Artem.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
  2023-06-29 22:04   ` Thomas Gleixner
  2023-06-29 22:36     ` Thomas Gleixner
  2023-06-30 16:54     ` Artem Bityutskiy
@ 2023-07-07 17:13     ` Artem Bityutskiy
  2 siblings, 0 replies; 9+ messages in thread
From: Artem Bityutskiy @ 2023-07-07 17:13 UTC (permalink / raw
  To: Thomas Gleixner, x86, Rafael J. Wysocki
  Cc: Linux PM Mailing List, Arjan van de Ven

On Fri, 2023-06-30 at 00:04 +0200, Thomas Gleixner wrote:
> > +#ifdef CONFIG_X86_64
> > +/*
> > + * Monitor a memory address at 'rcx' using the 'umonitor' instruction.
> > + */
> > +static inline void __umonitor(const void *rcx)
> > +{
> > +       /* "umonitor %rcx" */
> > +#ifdef CONFIG_AS_TPAUSE
> 
> Are you sure that the instruction check for TPAUSE is sufficient to also
> include UMONITOR on all toolchains which support TPAUSE?

I've verified by building the kernel with gcc/binutils and clang/LLVM.
Builds, boots, umwait works, C0.2 happens with both.

I inspected gcc, binutils, and clang/llvm git logs: support for
'tpause' and 'umwait' arrived in the same commit. Details below.

'tpause' and 'umwait' instructions are very similar, arrived together,
guarded together by CPUID.7's "MWAITPKG" bit. Based on this, I'd generally
expect toolchains to support both or none.

I can add a note about this in the commit message too.

Details on commits in the projects I checked.

1. binutils-gcc git tree:
   de89d0a34d5 Enable Intel WAITPKG instructions.

2. gcc git tree:
   55f31ed10fd i386-common.c (OPTION_MASK_ISA_WAITPKG_SET, [...]): New defines.

3. llvm-project git tree:
   2e02579a76cf [OpenMP] Add use of TPAUSE

It'll take some time to re-test and and partially re-measure power/perf,
so I'll send new version a bit later.

Thanks,
Artem.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-07-07 17:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-10 18:35 [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Artem Bityutskiy
2023-06-10 18:35 ` [PATCH v3 1/2] x86/mwait: Add support for idle via umwait Artem Bityutskiy
2023-06-29 19:03   ` Rafael J. Wysocki
2023-06-29 22:04   ` Thomas Gleixner
2023-06-29 22:36     ` Thomas Gleixner
2023-06-30 16:54     ` Artem Bityutskiy
2023-07-07 17:13     ` Artem Bityutskiy
2023-06-10 18:35 ` [PATCH v3 2/2] intel_idle: add C0.2 state for Sapphire Rapids Xeon Artem Bityutskiy
2023-06-29 22:05 ` [PATCH v3 0/3] Sapphire Rapids C0.x idle states support Thomas Gleixner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.