Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait

From: Thomas Gleixner <tglx@linutronix.de>
To: Artem Bityutskiy <dedekind1@gmail.com>,
	x86@kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Linux PM Mailing List <linux-pm@vger.kernel.org>,
	Arjan van de Ven <arjan@linux.intel.com>,
	Artem Bityutskiy <dedekind1@gmail.com>
Subject: Re: [PATCH v3 1/2] x86/mwait: Add support for idle via umwait
Date: Fri, 30 Jun 2023 00:04:03 +0200	[thread overview]
Message-ID: <87352a0vu4.ffs@tglx> (raw)
In-Reply-To: <20230610183518.4061159-2-dedekind1@gmail.com>

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