linux-kernel - Re: [PATCH v6 1/4] x86/smp: Allow calling mwait_play

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z0lCtdZKzZQXTWxF@BLRRASHENOY1.amd.com>
Date: Fri, 29 Nov 2024 09:57:33 +0530
From: "Gautham R. Shenoy" <gautham.shenoy@....com>
To: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	rafael.j.wysocki@...el.com, peterz@...radead.org,
	dave.hansen@...ux.intel.com, tglx@...utronix.de,
	len.brown@...el.com, artem.bityutskiy@...ux.intel.com
Subject: Re: [PATCH v6 1/4] x86/smp: Allow calling mwait_play_dead with an
 arbitrary hint

Hello Patryk,

On Wed, Nov 27, 2024 at 05:15:15PM +0100, Patryk Wlazlyn wrote:
> The MWAIT instruction needs different hints on different CPUs to reach
> specific idle states. The current hint calculation in mwait_play_dead()
> code works in practice on current Intel hardware, but is not documented
> and fails on a recent Intel's Sierra Forest and possibly some future
> ones. Those newer CPUs' power efficiency suffers when the CPU is put
> offline.
> 
> Allow cpuidle code to provide mwait_play_dead with a known hint for
> efficient play_dead code.


Just a couple of minor nits:

You could just reword this something along the lines of:

"Introduce a helper function to allow offlined CPUs to enter FFh idle
states with a specific MWAIT hint. The new helper will be used in
subsequent patches by the acpi_idle and intel_idle drivers. This patch
should not have any functional impact."

There is no need to mention MWAIT hint calculation and the Sierra
Forest failure in this patch, as this patch is not doing anything to
fix the issue. Very likely you will be covering that in Patch 4.

> 
> Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>
> ---
>  arch/x86/include/asm/smp.h |  4 +-
>  arch/x86/kernel/smpboot.c  | 86 ++++++++++++++++++++------------------
>  2 files changed, 49 insertions(+), 41 deletions(-)
> 
> diff --git a/arch/x86/include/asm/smp.h b/arch/x86/include/asm/smp.h
> index ca073f40698f..ab90b95037f3 100644
> --- a/arch/x86/include/asm/smp.h
> +++ b/arch/x86/include/asm/smp.h
> @@ -114,6 +114,7 @@ void wbinvd_on_cpu(int cpu);
>  int wbinvd_on_all_cpus(void);
>  
>  void smp_kick_mwait_play_dead(void);
> +void mwait_play_dead_with_hint(unsigned int hint);
>  
>  void native_smp_send_reschedule(int cpu);
>  void native_send_call_func_ipi(const struct cpumask *mask);
> @@ -151,7 +152,6 @@ static inline struct cpumask *cpu_l2c_shared_mask(int cpu)
>  {
>  	return per_cpu(cpu_l2c_shared_map, cpu);
>  }
> -
>  #else /* !CONFIG_SMP */
>  #define wbinvd_on_cpu(cpu)     wbinvd()
>  static inline int wbinvd_on_all_cpus(void)

This hunk is not relevant to this patch.

The rest of the patch looks good to me.
--
Thanks and Regards
gautham.