Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1499694

Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 0/4] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-10-12 14:20 +0200
  [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-10-12 14:20 +0200
    Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for  KNL Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:40 +0200
      Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-12 16:50 +0200
    Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for  KNL Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:40 +0200
      Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-12 18:40 +0200
        Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for  KNL Thomas Gleixner <tglx@linutronix.de> - 2016-10-13 18:10 +0200
    Re: [PATCH v2 2/4] Add enabling of the R3 MWAIT during boot for KNL Dave Hansen <dave.hansen@linux.intel.com> - 2016-10-12 19:30 +0200
  [PATCH v2 4/4] Add R3MWAIT to CPU features Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-10-12 14:30 +0200
    Re: [PATCH v2 4/4] Add R3MWAIT to CPU features Borislav Petkov <bp@suse.de> - 2016-10-12 15:00 +0200
    Re: [PATCH v2 4/4] Add R3MWAIT to CPU features Thomas Gleixner <tglx@linutronix.de> - 2016-10-12 15:30 +0200

csiph-web