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 EE22DD2A541 for ; Wed, 16 Oct 2024 19:20:25 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0F0626B0093; Wed, 16 Oct 2024 15:20:25 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0A1036B0095; Wed, 16 Oct 2024 15:20:25 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EAB0A6B0096; Wed, 16 Oct 2024 15:20:24 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id CCD316B0093 for ; Wed, 16 Oct 2024 15:20:24 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 3B4F61A0AEA for ; Wed, 16 Oct 2024 19:20:06 +0000 (UTC) X-FDA: 82680431358.15.DDB761C Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf08.hostedemail.com (Postfix) with ESMTP id 443E916000A for ; Wed, 16 Oct 2024 19:20:16 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=s+AE5uFV; spf=none (imf08.hostedemail.com: domain of peterz@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=peterz@infradead.org; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729106277; 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=r2FMoZv7GZi36tQ4QRe51XXDDSE6b5fb9oopiYGqumo=; b=rjQYefLC8+/StgQeG8IFikJwMD2/mXqbrE7k874BSGdynqfwYhxtDN1qr1rSzCkOQLZPL1 PmsTxhuLjg+FeePbx7lwgXjUvvT7OCzW67ISeQfDolRnktfAyW+V96xNjPS2YY65Bo2t3O ZgKneVamLsrzEDTKDgsYhZ8aEQn40uc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729106277; a=rsa-sha256; cv=none; b=DuSwlxgyHcJGq4OjhC/RQFhrhmb0ml+0U60kKTXkQ7QlHN+k62Jg+Zm9n4alWgbNSs7lZ/ 8pFwLg+sUbYOhguPdKgTur5hgbI94/k3YVwDBCXd2yGdQ06TSRT+ddqDXzdUwuVqJ/B2DP zxnqOwFUKIZhs5VSh3+PfXE0OvYgC7c= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=s+AE5uFV; spf=none (imf08.hostedemail.com: domain of peterz@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=peterz@infradead.org; dmarc=none DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=r2FMoZv7GZi36tQ4QRe51XXDDSE6b5fb9oopiYGqumo=; b=s+AE5uFVm06tAz5ZjzacGLzCuJ 7YUtR0tG2I+4egdsgithCbAgd3zsLGdV8yCaIEjKbeBgV87U71Ofh39b9aSu8hDRqf14HqKd1KZrE vmWv06vwCWlEgqjpQ+ZysBpfND0RsjWHsYjxoX2/jifAMK6emeUb3AsW0FZQDmp0/MKAGPRdjCnY5 RmYUt/FDwOnzXNlD8SZxhtVXakkE+zrsYTk0T1kambUxLcvA2qKy2rZrvA8PGSBG4PhbxYIXWkMjV 10fV2Vuop+Bhe86fVueT9fbwJpx3ofurJWTW93Llo69b3bU2QYK8Fp0OeyG1jG4ijS2kmShyFtPkn wvn5DgSA==; Received: from j130084.upc-j.chello.nl ([24.132.130.84] helo=noisy.programming.kicks-ass.net) by casper.infradead.org with esmtpsa (Exim 4.98 #2 (Red Hat Linux)) id 1t19Z6-00000008naR-10Hi; Wed, 16 Oct 2024 19:20:13 +0000 Received: by noisy.programming.kicks-ass.net (Postfix, from userid 1000) id CE99B300777; Wed, 16 Oct 2024 21:20:11 +0200 (CEST) Date: Wed, 16 Oct 2024 21:20:11 +0200 From: Peter Zijlstra To: Dave Hansen Cc: Andy Shevchenko , 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: <20241016192011.GY17263@noisy.programming.kicks-ass.net> 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: X-Stat-Signature: u1w47qyu19om1w3jfe5cf9t7ao16dg8p X-Rspamd-Queue-Id: 443E916000A X-Rspam-User: X-Rspamd-Server: rspam08 X-HE-Tag: 1729106416-431813 X-HE-Meta: U2FsdGVkX1/XIClcFg5HuNcoJtND1fOzU120Zt5yTiDStTwnzGZbfIv2qsWy9gnd3UqUfDuQ82I5hlC8UYDpk2v9PeVWq5XswNu07W+vwNSccA6gaLsNvr31TXNcPIQICSjjrHz7UkrKM1aXDE2Fd1T6YP0OFQGi+j8g9dGc8REWwebsQodwOf2n9RxDgMb4n5z0gZRpOdaLmQl/+rityEp10HsBp5D95GBFfr5Juotgo9ooxHdkVydmU9WFBku9Rl2FhtTKsdB/nc7+ZXrKwFCcvkOqhlCDQRmXMIS4mt8J4CL6dOIV0jPA9KO7jdp/w5dGH+ZZHWpMnocROyG7x7Nj96llAvf36s7wrdFhM22Sbn3ahZBnh93Hdv/yiWW1gK2eiR2ic3mCnimiEzl8H5wEBy5PqTP3Y/ITDm9R7EMOoNZzJLLUghRabwDBXG3jtz/i4H/B/bGBhYz9onUI6pHVaFTwmGBpvAiBeX6imuiqPr5pwE/o7I5ZXeMCyXHYf9s8E0DtL3jK9GPoikKDXxogsgGGVSTyZ8yTrZlxDlQ+vgViBz41qSvXJqhA1AO9h9TRq0p7xFjAafWFGkJCy6SpNa9X0wDaN6MgIr57jY7lAeE8V4TIm0RnHDuLreScHp2XGr0hWqZpIuodcve4TAJiMmOmIZhUPSo/yOOrGqMb1Ye8yklaxHaTrZhDEkF8quKQMWxjduS/VF+Z8JvX671GWWgS4QsyBcsJvMyHYG99Z2QIVG5XDU73WeQ1XI/HILFsv0ungaKiHr+J0zN50hPQMWpe9Pqz/0EwrPTUodtwVz4Klkd2Cro40meJ7LaC6JhNveNBBX+5BQjaHo+yP2UwhOO+oQIvaOh+qZQN2fVbrqtZ18EQHr+vKSPTaBxwWGl2nLqHdO3pAAO6UK+uYf/YUtshGGhZgm58YNGQxM+iKHdrxZZlceinnt3Bfa9BgJ9Sp9QMGrsYRDTyae/ ja/06suV aKwjCFC9Da4umRp+Tt02DdcSwiJKfWwKVNozCPiTCx1P7V610OqFncI0JFs9/ViRDxTSf2M7jaLzhMyiXIP33d1+oxf4L2ZxJlJIvjscn1jUhc8ojwtOAf/yR5cJ8kA3rR1kysrQR8N1vTsMsdRx+tUZ5MsKuilSQUMjNuZBQ0DCnRCUQS8nbdPA9YcBpkQG3oKQ4aQow4/I97re/5w6mRWNVKzS/9HPVWN8luZvILw4kqIHVmjrSJleGxkd8td8sgrO4PhTGmjVikpgLSa510rQ9nER80FPhbYxE 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 > > > --- 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. > > 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 Which is correct, no? Add of 255 to an u8 is the same as decrement one. > 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. You're correct in that it does not achieve the desired result (in all cases). But this is because (int)(val) will never turn into -1 when val == 255. > 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? > > 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. This is very very wrong. No u8 value when cast to int will ever equal -1. Notably (int)(u8)255 == 255. > 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? Or > do most clang users just ignore all the warnings? Or are you using a > bleeding edge version of clang that spits out new warnings that other > clang users aren't seeing? The code as is, is wrong, I don't think we'll ever end up in the dec case for 'short' unsigned types. Clang is just clever enough to realize this and issues a warning. Something like so might work: const int pao_ID__ = __builtin_constant_p(val) ? ((typeof(var))(val) == 1 ? 1 : ((typeof(var))(val) == (typeof(var))-1 ? -1 : 0 )) : 0; This should get, assuming typeof(var) is u8, a dec for both 255 and -1.