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: <2d541e31-b5e0-41d9-9f08-8b6f0a20d716@linux.intel.com>
Date: Fri, 29 Nov 2024 13:09:33 +0100
From: Patryk Wlazlyn <patryk.wlazlyn@...ux.intel.com>
To: "Gautham R. Shenoy" <gautham.shenoy@....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

>> 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.

Ok. I thought that giving some context on how the change originated might be
useful, but people doesn't seem to like that that much ;]

> "(...) This patch should not have any functional impact."

Yeah, forgot to put that no functional change intended.

>>
>> 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);

That actually is needed.

>>  
>>  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);
>>  }
>> -

Yeah, missed that one when submitting.

>>  #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.