From: "Carroll, Lewis" <Lewis.Carroll@amd.com>
To: Borislav Petkov <bp@alien8.de>
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 21:49:27 +0000 [thread overview]
Message-ID: <MN2PR12MB394969DC4BAF9B91A0E8A560FAE49@MN2PR12MB3949.namprd12.prod.outlook.com> (raw)
In-Reply-To: <Ykyo00r8aIibvLpP@zn.tnic>
[Public]
> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, April 5, 2022 4:39 PM
> To: Carroll, Lewis <Lewis.Carroll@amd.com>
> Cc: linux-kernel@vger.kernel.org; peterz@infradead.org;
> 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; x86@kernel.org;
> tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com;
> chang.seok.bae@intel.com; keescook@chromium.org; metze@samba.org;
> zhengqi.arch@bytedance.com; mark.rutland@arm.com
> Subject: Re: [PATCH] x86: Prefer MWAIT over HALT on AMD processors
>
> [CAUTION: External Email]
>
> 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?
Just when I thought I was being thorough. Any of the above will block the
cpuidle driver from loading. As will absence of _CST ACPI methods (add that
as a fourth cause). Brings back memories of code comments about a certain
idle driver being created to work around broken BIOSes...
>
> > 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.
We will have to see what we can sanitize. The original performance observation
(packet loss in a networking application) led to discovery of lots of cycles
in the various go-to-sleep-via-halt and wake-from-halt-via-IPI functions. Wyes
collected the raw data on the relative idle+wake-up latency and included that
in the commit msg. Think of that delta as the root cause of the performance
regression in this case.
>
> > 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?
Yes we are saying use MWAIT instead of HLT on all known (as of today) Zen
uarch CPUs (AMD >= 17h and Hygon).
>
> 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. :-)
Ack.
>
> Thx.
>
> --
> Regards/Gruss,
> Boris.
next prev parent reply other threads:[~2022-04-06 2:47 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-05 13:00 [PATCH] x86: Prefer MWAIT over HALT on AMD processors Wyes Karny
2022-04-05 14:05 ` Borislav Petkov
2022-04-05 20:26 ` Carroll, Lewis
2022-04-05 20:38 ` Borislav Petkov
2022-04-05 21:49 ` Carroll, Lewis [this message]
2022-04-06 9:30 ` Borislav Petkov
2022-04-06 6:14 ` Wyes Karny
2022-04-06 9:25 ` Borislav Petkov
2022-04-05 14:07 ` Peter Zijlstra
2022-04-05 15:10 ` Dave Hansen
2022-04-05 15:34 ` Limonciello, Mario
2022-04-05 15:47 ` Dave Hansen
2022-04-05 20:40 ` Limonciello, Mario
2022-04-06 1:44 ` Thomas Gleixner
2022-04-06 14:23 ` Limonciello, Mario
2022-04-07 21:16 ` Dave Hansen
2022-04-08 1:24 ` Limonciello, Mario
2022-04-14 21:06 ` Limonciello, Mario
2022-04-07 2:19 ` Wen Pu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MN2PR12MB394969DC4BAF9B91A0E8A560FAE49@MN2PR12MB3949.namprd12.prod.outlook.com \
--to=lewis.carroll@amd.com \
--cc=Ananth.Narayan@amd.com \
--cc=Mario.Limonciello@amd.com \
--cc=Wyes.Karny@amd.com \
--cc=bharata@amd.com \
--cc=bp@alien8.de \
--cc=chang.seok.bae@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=gautham.shenoy@amd.com \
--cc=hpa@zytor.com \
--cc=keescook@chromium.org \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=metze@samba.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=zhengqi.arch@bytedance.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).