Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors

From: Borislav Petkov <bp@alien8.de>
To: "Carroll, Lewis" <Lewis.Carroll@amd.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"peterz@infradead.org" <peterz@infradead.org>,
	"dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
	"Karny, Wyes" <Wyes.Karny@amd.com>,
	"Limonciello, Mario" <Mario.Limonciello@amd.com>,
	"Shenoy, Gautham Ranjal" <gautham.shenoy@amd.com>,
	"Narayan, Ananth" <Ananth.Narayan@amd.com>,
	"Rao, Bharata Bhasker" <bharata@amd.com>,
	"len.brown@intel.com" <len.brown@intel.com>,
	"x86@kernel.org" <x86@kernel.org>,
	"tglx@linutronix.de" <tglx@linutronix.de>,
	"mingo@redhat.com" <mingo@redhat.com>,
	"hpa@zytor.com" <hpa@zytor.com>,
	"chang.seok.bae@intel.com" <chang.seok.bae@intel.com>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"metze@samba.org" <metze@samba.org>,
	"zhengqi.arch@bytedance.com" <zhengqi.arch@bytedance.com>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>
Subject: Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
Date: Tue, 5 Apr 2022 22:38:43 +0200	[thread overview]
Message-ID: <Ykyo00r8aIibvLpP@zn.tnic> (raw)
In-Reply-To: <MN2PR12MB3949923BCD8B368F8269565BFAE49@MN2PR12MB3949.namprd12.prod.outlook.com>

On Tue, Apr 05, 2022 at 08:26:53PM +0000, Carroll, Lewis wrote:
> This happens when:
>  * User disables global C-states in BIOS
>  * User disables cpuidle (e.g. idle=off or processor.max_cstate=0)
>  * Kernel does not have CONFIG_ACPI_PROCESSOR_IDLE

All three or any one of those?

> Genesis of this patch is customer performance observations.

Please add that explanation to the changelog - it is important when
looking back, trying to figure out why this was done.

> Yes. We felt the code more readable with the prefer_mwait_c1_over_halt fn.
> Hygon CPU init indeed sets X86_FEATURE ZEN.
> AMD CPU init sets X86_FEATURE_ZEN for family >= 17h (not only 17h).

Yes, but this new logic you're adding, basically says, use MWAIT on all
Zen uarch CPUs, right?

So why not write exactly that?

The simpler the logic and the clearer the code, the better.

> Cleanest solution might be a new CPU feature (e.g.
> X86_PREFER_MWAIT_IDLE) that gets set appropriately, but that would
> require touching more files.

Yes, I thought about it too.

Not really necessary if what I wrote above fits.

And while you're touching files, pls add that change too:

---

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 73e643ae94b6..c1091f78f104 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -219,7 +219,7 @@
 #define X86_FEATURE_IBRS		( 7*32+25) /* Indirect Branch Restricted Speculation */
 #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch Prediction Barrier */
 #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect Branch Predictors */
-#define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 or above (Zen) */
+#define X86_FEATURE_ZEN			( 7*32+28) /* "" Set on CPUs of the Zen uarch */
 #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
 #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
 #define X86_FEATURE_MSR_IA32_FEAT_CTL	( 7*32+31) /* "" MSR IA32_FEAT_CTL configured */

so that dhansen and peterz are not confused anymore. :-)

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette
