From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 074A91B4224 for ; Mon, 13 Apr 2026 17:53:18 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776102800; cv=none; b=VRpDdSHcm/gRMMiYLCYY24WQjMENbJrT+Jcdbh2bZUgrZKA7GqCEEJNAOvBB89JkZMt0ZrCbmCv2lzz0sD14N+i4Wsq++Yq0dBI6mGZYSqtLVTmesbtH967tsVSxHcLDMqbm3bWDJV3IPAUUGeuhFJuJHeCeTwZrr5loSMx4IZ0= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1776102800; c=relaxed/simple; bh=G6B2729pPs4lXsNyzUaSdWZBnoa3k42U/vOmX9B6sB4=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=H1bVrai+4FJI4t/2vadsMVeMEcAANMDrFCM76YAIqJoUi3EHF7TncjvBVLc7rFNcJl6rS9NcO9K5hW7yHPYUj2BC2wkZFBLsmPlXw1DqgAd8ODqO5nToLVs1r7CBWr9w1roS6g0MxDeWYrzA8/l+6q7tZ1+OQcpWbDG0Bsuends= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=AixE8qbj; arc=none smtp.client-ip=209.85.128.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="AixE8qbj" Received: by mail-wm1-f41.google.com with SMTP id 5b1f17b1804b1-488b8bc6bc9so33686885e9.3 for ; Mon, 13 Apr 2026 10:53:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1776102797; x=1776707597; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=3kgsy1ZnGeF8QKwde3KB0blIM91Ze9pnTJiGiBYerV0=; b=AixE8qbj8gJQmB6mlsj9JXYds1H01aN8QIGxzVecUpgJB1R6LhYuUf+6Ib5desyCAr PIP6yXbLmzKgWHUs8Tzt7fdeKGVAmuI5OTC5R3cal0qKv514cQIAbnjBRKDWi++nFb9D wafswRulPUdV1PLUoEqcWNtdWrLz/Tdt3f51FlYf4ZMMc1OJzO+/stvGZ+8a/JzT+1pS qzbSSUcMtnSzmLimLo+ltWEoGrB+xj8y8pqMsqG7TX6GBLvmVrY3Ijrsz2JFWM/zvhjy OOdQeWFXCXrAHzuJEtA1jPoC5BLItnraUrmrdXUwlKRrACquU7Fvtwd/gy+Y7qYo5t7h Ucqw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776102797; x=1776707597; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-gg:x-gm-message-state:from :to:cc:subject:date:message-id:reply-to; bh=3kgsy1ZnGeF8QKwde3KB0blIM91Ze9pnTJiGiBYerV0=; b=jwuh272hcREtnNFWWJr8TOzrW+si/2SKFPqoy5zz51f1lz5Bqby9IErAfv2Sd8Qx7y VIidRdRDqjSzaqIx/rskfss9vxY1C8PWPtYS86L8KI4ImsnpYXSSnGQXVd1YNmYN0wfO qcF1BbKJWVdtAGlD/DjhjaYStIXtZiAhLgZ6PRVLXOhfW6Y2zllBu5DXWA/8vTV87n2r TXyy/shg4wBOW0xKfOKwCYZtKpWKnQSFtizMf2PzZ0d+kV15QR7QjRYnucNEwI1WleI2 kavHPWsEIaybAUcDuZIetQVQau3D6TXyi8aUnXu7NWvVfJ9ah3yY5y1a4W8NPQT69qGQ o1Lw== X-Forwarded-Encrypted: i=1; AFNElJ+h0o6pRftNs66vD2BZ8lqLuv3vLEYaCDUxHadHOIQTmfFtdh0TKz0b9rZWuL6LAiuRwsF+kQB4o2uHwBQ=@vger.kernel.org X-Gm-Message-State: AOJu0YxRs/t+Xler57k7B4jkyYEGxGuxoWI+Y/Upp27Hhf+4qqw/zuvr jWJRZWHn7+1QSvx1I8OowZB3jWQAM+hhxwg1ZVMKCNVSINvn3GlDC3NA X-Gm-Gg: AeBDietArt45qSEPl6bLmL62J4AuVFbt2vEe0kmVwWU9aMkv2Zi1xzjxtrVWUHQF+pP CshlYFz0KmhYhT6OdqaLuBpcqL1G2UpAesR/PJAvwmqoxZ2Mofso4uJSW+h0kvoRRhkVVqlZfnF avNN7oyWOsN4eQ0B36MyT76CELsiIjq4uaIpnIyX2/T3nWzMGtzYpnd+DsL4aaaGmjC4Pfbi5JL cihF7FOP6zWSA9N7yTjpQYK/+Pp8ET9KssKqHqL9sWWT268QJ/S4fiA1xoNAMiwKSeDg/ROx538 Wbm/odHbdfpSw7x2I+eX0Pstix4XYipj7/9oUkaxlc/3xqvkk8hGBWN0hQRcVxftQxBwLc8QDq9 NoWSpEhEDU9e/ItN5rQX6L0WcRpGCRk9/y/MLn6DgStz7b874nbRaW0Nwc6BIrQ0nBoP2Y7TWmW O7Pv0cll+SorfPMXcbCWxuPXZslmiI3X8fZTbrZ07+yrgfZ35W49A2wrpHQwGrlBIg X-Received: by 2002:a05:600c:45c6:b0:488:a797:f0ac with SMTP id 5b1f17b1804b1-488d6ac2226mr161666455e9.28.1776102797175; Mon, 13 Apr 2026 10:53:17 -0700 (PDT) Received: from pumpkin (82-69-66-36.dsl.in-addr.zen.co.uk. [82.69.66.36]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-488d538c39bsm306230065e9.14.2026.04.13.10.53.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Apr 2026 10:53:16 -0700 (PDT) Date: Mon, 13 Apr 2026 18:53:15 +0100 From: David Laight To: Yury Norov Cc: Geert Uytterhoeven , Alexandre Belloni , Jonathan Cameron , Crt Mori , Nuno =?UTF-8?B?U8Oh?= , Richard Genoud , Andy Shevchenko , Yury Norov , Rasmus Villemoes , Matt Coster , open list Subject: Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant Message-ID: <20260413185315.0b5259ec@pumpkin> In-Reply-To: References: <20260410090927.60484-1-david.laight.linux@gmail.com> <20260410194539.560b5fdc@pumpkin> <20260411115415.3724cdcd@pumpkin> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; arm-unknown-linux-gnueabihf) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 13 Apr 2026 12:40:11 -0400 Yury Norov wrote: > On Sat, Apr 11, 2026 at 11:54:15AM +0100, David Laight wrote: > > On Sat, 11 Apr 2026 00:24:28 -0400 > > Yury Norov wrote: > > > > > On Fri, Apr 10, 2026 at 07:45:38PM +0100, David Laight wrote: > > > > On Fri, 10 Apr 2026 12:55:25 -0400 > > > > Yury Norov wrote: > > > > > > ... > > > > > > > > > Note that when 'val' is a variable 'val << constant' is likely > > > > > > to execute faster than 'val * (1 << constant)'. > > > > > > So the normal FIELD_PREP() is best left alone. > > > > > > > > > > Do you have any numbers? I'd prefer to have the codebase consistent > > > > > when possible. > > > > > > > > I think the multiply instruction will have a higher latency than the shift. > > > > So you are talking about a very small number of clocks if the expression > > > > is in the critical register dependency path. > > > > However FIELD_GET() would need to use a divide - and that would be a lot > > > > worse. > > > > > > > > Having written that, ISTR that 'mask' is required to be a constant. > > > > So the compiler may use a shift anyway - if the divide is unsigned. > > > > But for non-constant mask you definitely want a 'shift right'. > > > > > > Non-constant masks are handled with __field_get(), which doesn't use > > > __bf_shf(). > > > > > > > While you might think that it only makes sense to use unsigned values, > > > > I've found one piece of code (IIRC in the x86 fault handler) that > > > > passes a signed value to FIELD_GET() and needs the result sign extended. > > > > So, unless that is changed, FIELD_GET() must use an explicit right shift. > > > > (Of course, right shift of negative values is probably UB...) > > > > > > FIELD_GET() is quite fine with the change: > > > > > > #define __FIELD_GET(mask, reg, pfx) \ > > > ({ \ > > > __BF_FIELD_CHECK_MASK(mask, 0U, pfx); \ > > > - (typeof(mask))(((reg) & (mask)) >> __bf_shf(mask)); \ > > > + (typeof(mask))(((reg) & (mask)) / __bf_low_bit(mask)); \ > > > }) > > > > > > void my_test(void) > > > { > > > f3 0f 1e fa endbr64 > > > 48 83 ec 08 sub $0x8,%rsp > > > volatile int i = -1; > > > > > > pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i)); > > > 48 c7 c7 13 e3 51 82 mov $0xffffffff8251e313,%rdi > > > volatile int i = -1; > > > c7 44 24 04 ff ff ff movl $0xffffffff,0x4(%rsp) > > > ff > > > pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i)); > > > 8b 74 24 04 mov 0x4(%rsp),%esi > > > > > > } > > > 48 83 c4 08 add $0x8,%rsp > > > pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i)); > > > 81 e6 e0 07 00 00 and $0x7e0,%esi > > > 48 c1 ee 05 shr $0x5,%rsi > > > e9 32 aa b9 ff jmp <_printk> > > > > There is a subtle difference between (https://www.godbolt.org/z/KM7MesPWM): > > > > int a(int x) > > { > > return x >> __bf_shf(0xf0u); > > } > > > > int b(int x) > > { > > return x / __bf_low_bit(0xf0); > > } > > > > int c(int x) > > { > > return x / __bf_low_bit(0xf0u); > > } > > > > a: > > movl %edi, %eax > > sarl $4, %eax > > ret > > b: > > testl %edi, %edi > > leal 15(%rdi), %eax > > cmovns %edi, %eax > > sarl $4, %eax > > ret > > c: > > movl %edi, %eax > > shrl $4, %eax > > ret > > > > A while ago I did a compile-test for negative values and found one > > place that requires the sign-replicating right shift. > > Again, please be more certain. When? Which compiler did you use? Which > place have you found? Does that place still exist? Basically the compiler doesn't matter and the code is in arch/x866/mm_extable.c Read the comment at the top of arch/x86/include/asm/extable_fixup_flags.h. I've just done a build that uses a union and C bitfields in struct exception_table_entry, actually compiles to fewer bytes - but there are 4 memory reads instead of one so it may be slower. > > > > > So you'd need that check and to fixup the caller. > > None of them use DIV or MUL expensive instructions, which was your > original concern. If you're concerned about code generation in (b), > you can typecast it to an unsigned with __bf_cast_unsigned(). FFS - that is only of those unnecessary and bloat-worthy defines that makes FIELD_GET(GENMASK(),) expand to kb of source. I've got another new plan for __BF_FIELD_CHECK_REG(). > And I > also think that __bf_low_bit() is a bad name - low bit is always #0. > Maybe ffs_mask(), lsb_mask() or more wordy least_set_bit_mask()? > > Altogether, IMO this would be: > > #define ffs_mask(val) (__bf_cast_unsigned(val, val) & \ > (~(__bf_cast_unsigned(val, val)) + 1) Nope, the most you need is: ((val) & ((~val) + 1)) + 0u + 0ul + 0ull) That will zero-extend the value and the compiler will throw away all the spare high zeros. There is another annoyance that if val is '(int)(1u << 31)' you get a warning about integer overflow. > > -- > > Regardless of __bf_low_bit() discussion, __bf_shf() needs to get fixed > for gcc <= 14, because it's a public API and has over 100 users in the > kernel. I wonder how that happened! the leading __ really ought to have indicated that is was internal. In any case none of those rely on the result being an integer constant expressions (and set the high bit) or there'd have need earlier errors. For this error it really is best to just revert the change to the file that is generating the error - it is a new change to that file. Better change it to something sensible like: BUILD_BUG_ON_ZERO(((a) | (b) | (c) | (d)) > 0xffffu) + (u64)a << 48 | (u64)b << 32 | c << 16 | d David So, Matt, you're very welcome to submit v2. > > Thanks, > Yury