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

Groups > linux.kernel > #1545985

Re: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing

Show all headers | View raw

On Tue, 20 Dec 2016, Grzegorz Andrejczuk wrote:

So how am I supposed to know which version of these patches is the right
one? Both subject lines are identical. 

> Enable ring 3 MONITOR/MWAIT for Intel Xeon Phi x200
> codenamed Knights Landing.
> 
> The patch:

From your cover letter:

     Removed "This patch" from commit messages

Why is 'The patch any better' ?

> - Sets CPU feature X86_FEATURE_RING3MWAIT.
> - Sets HWCAP2_RING3MWAIT bit in ELF_HWCAP2.
> - Adds the ring3mwait=disable command line parameter.
> - Sets bit 1 of the MSR MISC_FEATURE_ENABLES or clears it when
>   the ring3mwait=disable command line parameter is used.

Changelogs should not describe WHAT the patch is doing. We can see that
from the patch. Changelogs should describe the WHY and CONCEPTS not
implementation details.

> +static void probe_xeon_phi_r3mwait(struct cpuinfo_x86 *c)
> +{
> +	/*
> +	 * Ring 3 MONITOR/MWAIT feature cannot be detected without
> +	 * cpu model and family comparison.
> +	 */
> +	if (c->x86 != 6 || c->x86_model != INTEL_FAM6_XEON_PHI_KNL)
> +		return;
> +
> +	if (ring3mwait_disabled) {
> +		msr_clear_bit(MSR_MISC_FEATURE_ENABLES,
> +			      MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
> +		return;
> +	}
> +
> +	msr_set_bit(MSR_MISC_FEATURE_ENABLES,
> +		    MSR_MISC_FEATURE_ENABLES_RING3MWAIT_BIT);
> +	set_cpu_cap(c, X86_FEATURE_RING3MWAIT);
> +	set_bit(HWCAP2_RING3MWAIT, (unsigned long *)&ELF_HWCAP2);

From your cover letter:

     "Removed warning from 32-bit build"

First of all, the warning

   arch/x86/include/asm/bitops.h:72:1: note: expected 'volatile long unsigned int *'
but argument is of type 'unsigned int *'
    set_bit(long nr, volatile unsigned long *addr)

is not at all 32bit specific.

Handing an unsigned int pointer to a function which expects a unsigned long
is even more wrong on 64bit.

So now for your 'removal fix': It's just as sloppy as anything else what
I've seen from you before.

Handing a typecasted unsigned int pointer to a function which expects an
unsigned long pointer is just broken and a clear sign of careless
tinkering.

The only reason why this 'works' is because x86 is a little endian
architecture and the bit number is a constant and set_bit gets translated
it into:

    orb 0x02, 0x0(%rip) 

Now if you look really close to that disassembly then you might notice,
that this sets bit 1 and not as you tell in patch 2/5:

   "Introduce ELF_HWCAP2 variable for x86 and reserve its bit 0 to expose
    the ring 3 MONITOR/MWAIT."

So why does it not set bit 0?

Simply because you hand in HWCAP2_RING3MWAIT as bit number, which is
defined as:

+#define HWCAP2_RING3MWAIT              (1 << 0)

Crap, crap, crap.

What's so !$@&*(? wrong with doing the simple, obvious and correct:

       ELF_HWCAP2 |= HWCAP2_RING3MWAIT;

C is really hard, right?

Yours grumpy

      tglx



       

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

Thread

[PATCH v11 0/5] Enabling Ring 3 MONITOR/MWAIT feature for Knights Landing Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 14:50 +0100
  [PATCH v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 14:50 +0100
    [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 19:40 +0100
      Re: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights  Landing Thomas Gleixner <tglx@linutronix.de> - 2016-12-21 21:30 +0100
        RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights  Landing "Andrejczuk, Grzegorz" <grzegorz.andrejczuk@intel.com> - 2016-12-22 11:30 +0100
          RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights  Landing Thomas Gleixner <tglx@linutronix.de> - 2016-12-22 12:10 +0100
            RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights  Landing "Andrejczuk, Grzegorz" <grzegorz.andrejczuk@intel.com> - 2016-12-22 12:40 +0100
              RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights  Landing Thomas Gleixner <tglx@linutronix.de> - 2016-12-22 15:30 +0100
                RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights  Landing "Andrejczuk, Grzegorz" <grzegorz.andrejczuk@intel.com> - 2016-12-23 19:20 +0100
  [PATCH v11 2/5] x86/elf: add HWCAP2 to expose ring 3 MONITOR/MWAIT Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 14:50 +0100
  [PATCH v11 5/5] x86/cpufeature: enable RING3MWAIT for Knights Mill Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 15:00 +0100
  [PATCH v11 3/5] x86/cpufeature: add RING3MWAIT to CPU features Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 15:10 +0100
  [PATCH v11 1/5] x86/msr: add MSR_MISC_FEATURE_ENABLES and RING3MWAIT bit Grzegorz Andrejczuk <grzegorz.andrejczuk@intel.com> - 2016-12-20 15:20 +0100
    Re: [PATCH v11 1/5] x86/msr: add MSR_MISC_FEATURE_ENABLES and  RING3MWAIT bit Thomas Gleixner <tglx@linutronix.de> - 2016-12-21 21:30 +0100
      RE: [PATCH v11 1/5] x86/msr: add MSR_MISC_FEATURE_ENABLES and  RING3MWAIT bit "Andrejczuk, Grzegorz" <grzegorz.andrejczuk@intel.com> - 2016-12-22 10:20 +0100

csiph-web