From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8E897D2A53C for ; Wed, 16 Oct 2024 18:06:26 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 159716B007B; Wed, 16 Oct 2024 14:06:26 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 10A1A6B0082; Wed, 16 Oct 2024 14:06:26 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F13366B0088; Wed, 16 Oct 2024 14:06:25 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id D5ED86B007B for ; Wed, 16 Oct 2024 14:06:25 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 4F475120A5C for ; Wed, 16 Oct 2024 18:06:16 +0000 (UTC) X-FDA: 82680244752.03.92A261E Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.19]) by imf03.hostedemail.com (Postfix) with ESMTP id A9E642001A for ; Wed, 16 Oct 2024 18:06:17 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=F02sERQs; spf=none (imf03.hostedemail.com: domain of andriy.shevchenko@linux.intel.com has no SPF policy when checking 198.175.65.19) smtp.mailfrom=andriy.shevchenko@linux.intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729101838; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=zNOhzAJNrnM7OInuOn3TdesiTWzm4XLU0pQmwbY6M8o=; b=ODa9fI2SKJzZalgKfSAQoaYNbNwxr1x4Yw7GNrrZIE2j221zh3uZk9sLDPIBcGiGnfFxkb 50J+zmOEHImA/ywn2aqqX6p8Q8OAtDQlIF5CM8oD72YmXIiO8YjsyV/pPzg7iLFRq/Y+Lx A2aFSTZOUs0xJXOZNw4f/Hc8Hnn8hyc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729101838; a=rsa-sha256; cv=none; b=xSyil1tw0I7sRr6/pCRf0NzzOvRzCjBrEU7JtracvOjeGyLEuxvinfsCMk6l+g3Dasd2CC KER5BpY4RKv190JCFV8sDu2CUDFoV4nKpTVO/Ydk3GRq0ksbPv2fygzP9ZFdQlm/AjdFJH Em1ysQyuGtP62JVn/AehWOv291QqCwE= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=F02sERQs; spf=none (imf03.hostedemail.com: domain of andriy.shevchenko@linux.intel.com has no SPF policy when checking 198.175.65.19) smtp.mailfrom=andriy.shevchenko@linux.intel.com; dmarc=pass (policy=none) header.from=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1729101982; x=1760637982; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=w6TvOZ8DUKkyB3QMeexFvF0T7Chl1fSWJvxGf79kMr0=; b=F02sERQstnxLABusYFZfKovvqJgpprD35YeRGpREM1Z/kSqAPtt0NRUx Dn/rd/GQcFGBMcr/DpCc172hMqQwNFaqtiCRoQou1P7gjCju789D7xfKR +7hnixabDEWDqWNw9pDKNOXjYDy1pOMqIXfo9UtaWvDstq+bnqx4aTFn7 hq0FG4ed7BdiFMtkTmJbeGBp/HNTOWKL1OYQWYdPFYwr7ju7lFWs4FFYI ixyzDAKTgoTkEJUCVvoN8AYyrJS0Ce8KVEp/sZG9YZo69Y4CDWYvdDeRU 7ymEsVYwA8RDC/UNZ84fQFEVPuFNc9ZZHFp/zjzFFZu2J0OAWMPEex+cx A==; X-CSE-ConnectionGUID: 8tCJuiNNShGqpZjRWW+iGQ== X-CSE-MsgGUID: Lyo/EN+PRc+HQ7jhAxdt+g== X-IronPort-AV: E=McAfee;i="6700,10204,11222"; a="28451274" X-IronPort-AV: E=Sophos;i="6.11,199,1725346800"; d="scan'208";a="28451274" Received: from orviesa005.jf.intel.com ([10.64.159.145]) by orvoesa111.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2024 11:06:20 -0700 X-CSE-ConnectionGUID: kcO4ws1BQNecWp2AlLE9KQ== X-CSE-MsgGUID: p1h4AFT5TFy1O02nh+y4Vw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,208,1725346800"; d="scan'208";a="83070346" Received: from smile.fi.intel.com ([10.237.72.154]) by orviesa005.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Oct 2024 11:06:17 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.98) (envelope-from ) id 1t18PV-00000003rcY-0FHZ; Wed, 16 Oct 2024 21:06:13 +0300 Date: Wed, 16 Oct 2024 21:06:12 +0300 From: Andy Shevchenko To: Dave Hansen Cc: Ingo Molnar , Uros Bizjak , linux-mm@kvack.org, linux-kernel@vger.kernel.org, llvm@lists.linux.dev, Dennis Zhou , Tejun Heo , Christoph Lameter , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Dave Hansen , x86@kernel.org, "H. Peter Anvin" , Nathan Chancellor , Nick Desaulniers , Bill Wendling , Justin Stitt Subject: Re: [PATCH v1 1/1] x86/percpu: Cast -1 to argument type when comparing in percpu_add_op() Message-ID: References: <20240905170356.260300-1-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-Stat-Signature: tbcsybtcc54szr5e6kcot3h9yg9khg5f X-Rspamd-Queue-Id: A9E642001A X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1729101977-202061 X-HE-Meta: U2FsdGVkX19/qsfVmQkGuikOZ+6ow3PnfJMiR6fSgpdSi8KC3EDm5myKWTlPwaovpWB7IgNvBdk+LCjatzq6nMhXRlXxKsTZYfPNTk0w9s6sGtqs8sWUtee/nPAOoz5YrHI+poW/WNfAKeRs1srEBGMGO8hlgXevH+X6Ttb2ES7FqP1OrlfcTOPLtwiU6a5xV+NA63eXNSu7alkCYSwfP6WDpScDz2bTbIKzw2AYc2h0FYOogS3sJtbytpowdzctjwcIegTz6blk2E6u+F+RGuKxICln/F5n3dfOfg8iwBgCRvZGT7PwS7xXZ2ltcjVht9MebcJMPT3ylDHGd/XR36B3i8kHQV557djX1/iOdbDUbKWwZuZQwUtcEzGfVr6R+q6cfmrFbbaMDhmcnRRco/12wcfUEyXmFRt0Oasqo0GZXro6ewtrIbTtnAEqeXltULvjtFgllZ3rvTVE8+DZaepaTp23897rYCSvPR3PidpJ3flm7cm3Nub2ypKC3L0J5U1++zzwEzBule3tIohJuo+A2kxNRbGA7z0celpj/T0Pnl8Pn6Y1x4JlmA9IcKtXU+aAOS2sHjDgeN+j8PVO8fNNxvhSzpMwLF0TI+/2rh3Dgmnqn0Y3+ujIo5kkiyBHDtUCY3X5s5zeoLcLv630zLlMiv4/WPKeMiMDZ3Ol43djWpn1HBgkJuNcgvAlOd5yf6oHEAY+6f0gpZBkFx3+u+OVscV+l+YEkcbSIR3QwntrSkDM0QN8Dg8HvOnYD0nljmXw2D9WGeHijDgXGb6ILEsIv7Lg0JEIbkXVGA0Q5DDJiFN1UfqM//EZOgK0oaSKbKk/HX3QReSEOQfhDspKAavdjD86kA8wUSvwCsPGBjp9hHOcE6Y6H8vL/eO+iMuKEIvEboGfV1zMgd/mbbgn1bAmvfug2o/M7HBPVm3CjUlStT9gkSFl7+ssgv4bTURAJIYwxhG0FHt1camWpfv Deck8oQK YVewo9r65j+Nw2mcX58DZ1dS9WZzUOoY+jf+PuMVpKcs+HPzcqhSFbo1WSsaJxvOAn9vmuqRVNxpXJ1KiOC+G3V3d0LkRVbimrHK12h5HSilTdfCoSoPioBvOD6FUZg42+r/3FYSjX+CR+vzBjWWdAX6+K0v0Ii15K69w+xOLTXLW7nPKf9Y3jd355nw2DLIvr3r2G8+JXTYpw5MOdP9SNb9GNs/v1NMMHx94XVPVf0OSOtv78G6cjNsV6Rt7VVTlv6+gmVHAVvPyI/gEOjermmmtVpTOjypfP2EbMZG8dDdh2+jgyRmHpU3h2dItMZaf/4nj64IpcHBrkM0= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Oct 16, 2024 at 08:44:56AM -0700, Dave Hansen wrote: > Andy, > > The subject here is not very informative. It explains the "what" of the > patch, but not the "why". > > A better subject might have been: > > x86/percpu: Fix clang warning when dealing with unsigned types Thanks, makes sense! > > --- a/arch/x86/include/asm/percpu.h > > +++ b/arch/x86/include/asm/percpu.h > > @@ -234,9 +234,10 @@ do { \ > > */ > > #define percpu_add_op(size, qual, var, val) \ > > do { \ > > - const int pao_ID__ = (__builtin_constant_p(val) && \ > > - ((val) == 1 || (val) == -1)) ? \ > > - (int)(val) : 0; \ > > + const int pao_ID__ = \ > > + (__builtin_constant_p(val) && \ > > + ((val) == 1 || \ > > + (val) == (typeof(val))-1)) ? (int)(val) : 0; \ > > This doesn't _look_ right. But if feels right if we really want to supply unsigned types here. Maybe some more magic is needed (like in min() case). > Let's assume 'val' is a u8. (u8)-1 is 255, right? So casting the -1 > over to a u8 actually changed its value. So the comparison that you > added would actually trigger for 255: > > (val) == (typeof(val))-1)) > > 255 == (u8)-1 > 255 == 255 > > That's not the end of the world because the pao_ID__ still ends up at > 255 and the lower if() falls into the "add" bucket, but it isn't great > for reading the macro. It seems like it basically works on accident. > Wouldn't casting 'val' over to an int be shorter, more readable, not > have that logical false match *and* line up with the cast later on in > the expression? Maybe more readable, but wouldn't it be theoretically buggy for u64? I'm talking about the case when u64 == UINT_MAX, which will be true in your case and false in mine. > const int pao_ID__ = (__builtin_constant_p(val) && > ((val) == 1 || (int)(val) == -1)) ? > > (int)(val) : 0; > > Other suggestions to make it more readable would be welcome. > > Since I'm making comments, I would have really appreciated some extra > info here like why you are hitting this and nobody else is. This is bog > standard code that everybody compiles. Is clang use _that_ unusual? Why are you asking me about this? I don't know... > Or do most clang users just ignore all the warnings? Same here. I don't know... Both Qs sounds rhetorical to me. > Or are you using a bleeding edge version of clang that spits out new warnings > that other clang users aren't seeing? AFAICT It's *not* even close to the bleeding edge. It's standard Debian supply. > Another nice thing would have been to say that this produces the exact > same code with and without the patch. Or that you had tested it in > *some* way. I have run percpu_test in both cases and also checked code with `bloat-o-meter` and `cmp -b`. Everything is the same. I even added a test case for the above mentioned situation. > It took me a couple of minutes to convince myself that your > version works and doesn't do something silly like a "dec" if you hand in > val==255. It took me much more to find the best solution that appears not everyone likes :-) P.S. And as Nick pointed out it's simple `make W=1`, what the additional information you wanna see here? Care to provide a template? -- With Best Regards, Andy Shevchenko