Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936234AbcLVKTz convert rfc822-to-8bit (ORCPT ); Thu, 22 Dec 2016 05:19:55 -0500 Received: from mga04.intel.com ([192.55.52.120]:56691 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757235AbcLVKTx (ORCPT ); Thu, 22 Dec 2016 05:19:53 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,387,1477983600"; d="scan'208";a="21588436" From: "Andrejczuk, Grzegorz" To: Thomas Gleixner CC: "mingo@redhat.com" , "hpa@zytor.com" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "Luc, Piotr" , "dave.hansen@linux.intel.com" Subject: RE: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing Thread-Topic: [Patch v11 4/5] x86/cpufeature: enable RING3MWAIT for Knights Landing Thread-Index: AQHSWsfWby0ag3ZTSUq6LLYbMivcDaERKPyAgAGx5YCAANYpEA== Date: Thu, 22 Dec 2016 10:19:50 +0000 Message-ID: References: <1482241726-27310-5-git-send-email-grzegorz.andrejczuk@intel.com> <1482258687-4582-1-git-send-email-grzegorz.andrejczuk@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2523 Lines: 60 > > 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. So enable for Ring 3 MWAIT for Knights Landing + explanation of model comparison and potential issues below. Should be Ok. >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. I thought this to be 32 issue because it popped up in 32 build. The reason for this is probably that sizeof(int) is equal to sizeof(long) on x64. I used the cast following set_cpu_cap define which does exactly the same thing with u32* type. Is using unsigned long would be OK. >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? I used set_bit because I wanted to be sure that this operation to be done atomically. There might be data race when multiple values of ELF_HWCAP2 will be set by multiple threads. Thanks for the bit 1 issue I missed that. I can define HWCAP_RING3MWAIT_BIT 0 and set it by set_bit? I could use OR operator as there are no other HWCAP2 values. I would choose option 1 but as you have seen I have some tendency to write sloppy code and not respond to emails. But I am willing to change. Best Regards, Grzegorz