Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
On Wed, 12 Oct 2016, Dave Hansen wrote:
> On 10/12/2016 06:34 AM, Thomas Gleixner wrote:
> >> > +if (c->x86 == 6 &&
> >> > +c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> >> > +phir3mwait) {
> >> > +u64 prev;
> >> > +
> >> > +rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> >> > +if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> >> > +wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> >> > + prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
> > The codingstyle here is just convoluted crap. What's wrong with writing it
> > proper?
> >
> > if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
> > u64 msr;
> >
> > rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> > msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
> > wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> >
> > }
> >
> > No horrible to read line breaks, no redundant check for x->x86 == 6 because
> > model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
> > conditional is pointless as the feature is default disabled. And even if it
> > is enabled the extra msr write is not a problem at all. This is early init
> > code and not some hot path.
>
> Hi Thomas,
>
> We really do need to check for family=6 (c->x86==6).
> INTEL_FAM6_XEON_PHI_KNL is just for the model and doesn't check family.
> It implies that you've already checked for family 6.
Indeed. It came to me after sending the mail and closing the notebook to
head out for more conference fun. I expected someone to notice it :)
> Looking at the name, though, it's pretty clear that the naming can
> easily trip folks up.
>
> I do think we've probably screwed up the way we use our 'struct
> x86_cpu_id' mechanism. Maybe we should be providing the
> vendor/family/model sets from a common place to the drivers, instead of
> making them all repeat it individually.
>
> Like have a big header full of:
>
> DECLARE_CPU(INTEL_XEON_PHI_KNL, INTEL..., 6, MODEL_XYZ...);
>
> Once we have that, everybody can just do:
>
> if(cpu_is(c, INTEL_XEON_PHI_KNL))
> ...
>
> and get all the checking they need.
Right, and we should do the following:
__u8x86;
__u8x86_vendor;
__u8x86_model;
__u8x86_mask;
u32 x86_fvm;
set x86_fvm to family | vendor << 8 | model << 16; and then do the
comparison on that instead of checking 3 bytes in a row.
Thanks,
tglx
Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
On 10/12/2016 05:16 AM, Grzegorz Andrejczuk wrote:
> @@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> }
>
> check_mpx_erratum(c);
> +
> + /*
> + * Setting ring 3 MONITOR/MWAIT for all threads
> + * when CPU is Xeon Phi Family x200
> + * This can be disabled with phir3mwait=disable cmdline switch.
> + * We preserve the reserved values and set only 2nd bit.
> + * Ref:
> + *
> https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
> + */
> + if (c->x86 == 6 &&
> + c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> + phir3mwait) {
> + u64 prev;
> +
> + rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> + if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> + wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> +prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
> + }
> }
I'd really prefer that we put hunks like this into helpers just like the
nice check_mpx_erratum(). I know early_init_intel() looks a lot like
what you have here, but I think a little:
probe_xeon_phi_mwait()
would be a lot nicer.
BTW, this hunk totally indicates how badly named 'phir3mwait' is. Could
you please give it a sane name like 'phi_r3_mwait_disabled'?
Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
On 10/12/2016 06:34 AM, Thomas Gleixner wrote:
>> > + if (c->x86 == 6 &&
>> > + c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
>> > + phir3mwait) {
>> > + u64 prev;
>> > +
>> > + rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
>> > + if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
>> > + wrmsrl(MSR_PHI_MISC_THD_FEATURE,
>> > + prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
> The codingstyle here is just convoluted crap. What's wrong with writing it
> proper?
>
> if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
> u64 msr;
>
> rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
> msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
> wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
>
> }
>
> No horrible to read line breaks, no redundant check for x->x86 == 6 because
> model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
> conditional is pointless as the feature is default disabled. And even if it
> is enabled the extra msr write is not a problem at all. This is early init
> code and not some hot path.
Hi Thomas,
We really do need to check for family=6 (c->x86==6).
INTEL_FAM6_XEON_PHI_KNL is just for the model and doesn't check family.
It implies that you've already checked for family 6.
Looking at the name, though, it's pretty clear that the naming can
easily trip folks up.
I do think we've probably screwed up the way we use our 'struct
x86_cpu_id' mechanism. Maybe we should be providing the
vendor/family/model sets from a common place to the drivers, instead of
making them all repeat it individually.
Like have a big header full of:
DECLARE_CPU(INTEL_XEON_PHI_KNL, INTEL..., 6, MODEL_XYZ...);
Once we have that, everybody can just do:
if(cpu_is(c, INTEL_XEON_PHI_KNL))
...
and get all the checking they need.
Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
On 10/12/2016 06:35 AM, Thomas Gleixner wrote: > On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote: >> > + /* >> > + * Setting ring 3 MONITOR/MWAIT for all threads >> > + * when CPU is Xeon Phi Family x200 >> > + * This can be disabled with phir3mwait=disable cmdline switch. >> > + * We preserve the reserved values and set only 2nd bit. >> > + * Ref: >> > + * >> > https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait > Please don't put links like this into comments. They are stale before this > hits Linus tree. Yeah, I'm worried about this too. It's probably best to include some version of the text in that blog entry, or that text itself. That way, when Intel reorganizes and culls old blog entries, we won't completely lose our source.
Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote:
>
> +static int phir3mwait = 1;
> +static int __init phir3mwait_disable(char *value)
Can someone @Intel please tell everyone to stop this annoying habit of
glueing variable declarations without a newline to the function? And the
variable should be not in the middle of the code either. We have such stuff
on top of the file normaly if there is not a damned good reason to stick it
elsewhere.
That's just horrible and hard to read.
> +{
> + phir3mwait = 0;
> + return 1;
> +}
> +__setup("intel-phir3mwait=disable", phir3mwait_disable);
> +
> static void early_init_intel(struct cpuinfo_x86 *c)
> {
> u64 misc_enable;
> @@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
> }
>
> check_mpx_erratum(c);
> +
> + /*
> + * Setting ring 3 MONITOR/MWAIT for all threads
> + * when CPU is Xeon Phi Family x200
> + * This can be disabled with phir3mwait=disable cmdline switch.
> + * We preserve the reserved values and set only 2nd bit.
> + * Ref:
> + *
> https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
> + */
> + if (c->x86 == 6 &&
> + c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
> + phir3mwait) {
> + u64 prev;
> +
> + rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
> + if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
> + wrmsrl(MSR_PHI_MISC_THD_FEATURE,
> +prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
The codingstyle here is just convoluted crap. What's wrong with writing it
proper?
if (c->x86_model == INTEL_FAM6_XEON_PHI_KNL && phir3mwait) {
u64 msr;
rdmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
msr |= MSR_PHI_MISC_THD_FEATURE_R3MWAIT;
wrmsrl(MSR_PHI_MISC_THD_FEATURE, msr);
}
No horrible to read line breaks, no redundant check for x->x86 == 6 because
model cannot be INTEL_FAM6_XEON_PHI_KNL if x->x86 != 6. Also the
conditional is pointless as the feature is default disabled. And even if it
is enabled the extra msr write is not a problem at all. This is early init
code and not some hot path.
Thanks,
tglx
Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
On Wed, 12 Oct 2016, Grzegorz Andrejczuk wrote: > + /* > + * Setting ring 3 MONITOR/MWAIT for all threads > + * when CPU is Xeon Phi Family x200 > + * This can be disabled with phir3mwait=disable cmdline switch. > + * We preserve the reserved values and set only 2nd bit. > + * Ref: > + * > https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait Please don't put links like this into comments. They are stale before this hits Linus tree. Thanks, tglx
[PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL
If processor is Intel Xeon Phi we enable user-level mwait feature.
Enabling this feature suppreses invalid-opcode error, when MONITOR/MWAIT
is called from ring 3.
Change-Id: I1c7defb99296b022790a068a6c725b3e860cd68c
Signed-off-by: Grzegorz Andrejczuk
---
arch/x86/kernel/cpu/intel.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index fcd484d..ac6df08 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -61,6 +61,14 @@ void check_mpx_erratum(struct cpuinfo_x86 *c)
}
}
+static int phir3mwait = 1;
+static int __init phir3mwait_disable(char *value)
+{
+ phir3mwait = 0;
+ return 1;
+}
+__setup("intel-phir3mwait=disable", phir3mwait_disable);
+
static void early_init_intel(struct cpuinfo_x86 *c)
{
u64 misc_enable;
@@ -211,6 +219,25 @@ static void early_init_intel(struct cpuinfo_x86 *c)
}
check_mpx_erratum(c);
+
+ /*
+ * Setting ring 3 MONITOR/MWAIT for all threads
+ * when CPU is Xeon Phi Family x200
+ * This can be disabled with phir3mwait=disable cmdline switch.
+ * We preserve the reserved values and set only 2nd bit.
+ * Ref:
+ *
https://software.intel.com/en-us/blogs/2016/10/06/intel-xeon-phi-product-family-x200-knl-user-mode-ring-3-monitor-and-mwait
+ */
+ if (c->x86 == 6 &&
+ c->x86_model == INTEL_FAM6_XEON_PHI_KNL &&
+ phir3mwait) {
+ u64 prev;
+
+ rdmsrl(MSR_PHI_MISC_THD_FEATURE, prev);
+ if ((prev & MSR_PHI_MISC_THD_FEATURE_R3MWAIT) == 0)
+ wrmsrl(MSR_PHI_MISC_THD_FEATURE,
+ prev | MSR_PHI_MISC_THD_FEATURE_R3MWAIT);
+ }
}
#ifdef CONFIG_X86_32
--
2.5.1
