| Message ID | 20240822230635.954557-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/bitops: hweight() cleanup/improvements | expand |
On 23.08.2024 01:06, Andrew Cooper wrote: > This will be used to simplify real logic in the following patch. Add compile > and boot time testing as with other bitops. > > Because the expression is so simple, implement it as a function-like macro > which is generic on the type of it's argument, rather than having multiple > variants. > > Testing function-like macros needs a minor adjustments to the infrastructure > in xen/self-tests.h to avoid bracketing the fn parameter. The utility of this > outweighs the associated risks. We may need to mark these two places as deviated. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with one remark/request: > --- a/xen/common/bitops.c > +++ b/xen/common/bitops.c > @@ -84,8 +84,32 @@ static void __init test_fls(void) > CHECK(fls64, 0x8000000000000001ULL, 64); > } > > +static void __init test_multiple_bits_set(void) > +{ > + /* > + * multiple_bits_set() is generic on the type of it's parameter, as the > + * internal expression is so simple. > + */ > + > + CHECK(multiple_bits_set, 0, false); > + CHECK(multiple_bits_set, 1, false); > + CHECK(multiple_bits_set, 2, false); > + CHECK(multiple_bits_set, 3, true); > + > + CHECK(multiple_bits_set, 1 | (1UL << (BITS_PER_LONG - 1)), true); This is really the same as ... > +#if BITS_PER_LONG > 32 > + CHECK(multiple_bits_set, 1 | (1UL << 32), true); > + CHECK(multiple_bits_set, 1 | (1UL << 63), true); ... this, at least as long as BITS_PER_LONG > 32 in practice means BITS_PER_LONG == 64. Perhaps not really worth keeping that line? Jan
On 26/08/2024 11:30 am, Jan Beulich wrote: > On 23.08.2024 01:06, Andrew Cooper wrote: >> This will be used to simplify real logic in the following patch. Add compile >> and boot time testing as with other bitops. >> >> Because the expression is so simple, implement it as a function-like macro >> which is generic on the type of it's argument, rather than having multiple >> variants. >> >> Testing function-like macros needs a minor adjustments to the infrastructure >> in xen/self-tests.h to avoid bracketing the fn parameter. The utility of this >> outweighs the associated risks. > We may need to mark these two places as deviated. Perhaps, although it would want to be a project-wide deviation. Eclair was green with this patch in place, so it's not blocking. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with one remark/request: > >> --- a/xen/common/bitops.c >> +++ b/xen/common/bitops.c >> @@ -84,8 +84,32 @@ static void __init test_fls(void) >> CHECK(fls64, 0x8000000000000001ULL, 64); >> } >> >> +static void __init test_multiple_bits_set(void) >> +{ >> + /* >> + * multiple_bits_set() is generic on the type of it's parameter, as the >> + * internal expression is so simple. >> + */ >> + >> + CHECK(multiple_bits_set, 0, false); >> + CHECK(multiple_bits_set, 1, false); >> + CHECK(multiple_bits_set, 2, false); >> + CHECK(multiple_bits_set, 3, true); >> + >> + CHECK(multiple_bits_set, 1 | (1UL << (BITS_PER_LONG - 1)), true); > This is really the same as ... > >> +#if BITS_PER_LONG > 32 >> + CHECK(multiple_bits_set, 1 | (1UL << 32), true); >> + CHECK(multiple_bits_set, 1 | (1UL << 63), true); > ... this, at least as long as BITS_PER_LONG > 32 in practice means > BITS_PER_LONG == 64. Perhaps not really worth keeping that line? I suppose not. I'll drop it. However, It occurs to me that I do need a test of 0x8000000000000001ULL mostly for 32bit builds to check that even when the argument is split, the answer is still accurate. ~Andrew
On 27.08.2024 14:01, Andrew Cooper wrote: > On 26/08/2024 11:30 am, Jan Beulich wrote: >> On 23.08.2024 01:06, Andrew Cooper wrote: >>> --- a/xen/common/bitops.c >>> +++ b/xen/common/bitops.c >>> @@ -84,8 +84,32 @@ static void __init test_fls(void) >>> CHECK(fls64, 0x8000000000000001ULL, 64); >>> } >>> >>> +static void __init test_multiple_bits_set(void) >>> +{ >>> + /* >>> + * multiple_bits_set() is generic on the type of it's parameter, as the >>> + * internal expression is so simple. >>> + */ >>> + >>> + CHECK(multiple_bits_set, 0, false); >>> + CHECK(multiple_bits_set, 1, false); >>> + CHECK(multiple_bits_set, 2, false); >>> + CHECK(multiple_bits_set, 3, true); >>> + >>> + CHECK(multiple_bits_set, 1 | (1UL << (BITS_PER_LONG - 1)), true); >> This is really the same as ... >> >>> +#if BITS_PER_LONG > 32 >>> + CHECK(multiple_bits_set, 1 | (1UL << 32), true); >>> + CHECK(multiple_bits_set, 1 | (1UL << 63), true); >> ... this, at least as long as BITS_PER_LONG > 32 in practice means >> BITS_PER_LONG == 64. Perhaps not really worth keeping that line? > > I suppose not. I'll drop it. > > However, It occurs to me that I do need a test of 0x8000000000000001ULL > mostly for 32bit builds to check that even when the argument is split, > the answer is still accurate. IOW you'll insert an #else in the middle. Fine with me; keep the R-b. Jan
On 27/08/2024 1:50 pm, Jan Beulich wrote: > On 27.08.2024 14:01, Andrew Cooper wrote: >> On 26/08/2024 11:30 am, Jan Beulich wrote: >>> On 23.08.2024 01:06, Andrew Cooper wrote: >>>> --- a/xen/common/bitops.c >>>> +++ b/xen/common/bitops.c >>>> @@ -84,8 +84,32 @@ static void __init test_fls(void) >>>> CHECK(fls64, 0x8000000000000001ULL, 64); >>>> } >>>> >>>> +static void __init test_multiple_bits_set(void) >>>> +{ >>>> + /* >>>> + * multiple_bits_set() is generic on the type of it's parameter, as the >>>> + * internal expression is so simple. >>>> + */ >>>> + >>>> + CHECK(multiple_bits_set, 0, false); >>>> + CHECK(multiple_bits_set, 1, false); >>>> + CHECK(multiple_bits_set, 2, false); >>>> + CHECK(multiple_bits_set, 3, true); >>>> + >>>> + CHECK(multiple_bits_set, 1 | (1UL << (BITS_PER_LONG - 1)), true); >>> This is really the same as ... >>> >>>> +#if BITS_PER_LONG > 32 >>>> + CHECK(multiple_bits_set, 1 | (1UL << 32), true); >>>> + CHECK(multiple_bits_set, 1 | (1UL << 63), true); >>> ... this, at least as long as BITS_PER_LONG > 32 in practice means >>> BITS_PER_LONG == 64. Perhaps not really worth keeping that line? >> I suppose not. I'll drop it. >> >> However, It occurs to me that I do need a test of 0x8000000000000001ULL >> mostly for 32bit builds to check that even when the argument is split, >> the answer is still accurate. > IOW you'll insert an #else in the middle. Fine with me; keep the R-b. Oh, I already had that case, out of context below what you quoted. I'll just drop the one intermediate test. ~Andrew
diff --git a/xen/common/bitops.c b/xen/common/bitops.c index 94a8983af99c..4545682aa8e0 100644 --- a/xen/common/bitops.c +++ b/xen/common/bitops.c @@ -84,8 +84,32 @@ static void __init test_fls(void) CHECK(fls64, 0x8000000000000001ULL, 64); } +static void __init test_multiple_bits_set(void) +{ + /* + * multiple_bits_set() is generic on the type of it's parameter, as the + * internal expression is so simple. + */ + + CHECK(multiple_bits_set, 0, false); + CHECK(multiple_bits_set, 1, false); + CHECK(multiple_bits_set, 2, false); + CHECK(multiple_bits_set, 3, true); + + CHECK(multiple_bits_set, 1 | (1UL << (BITS_PER_LONG - 1)), true); +#if BITS_PER_LONG > 32 + CHECK(multiple_bits_set, 1 | (1UL << 32), true); + CHECK(multiple_bits_set, 1 | (1UL << 63), true); +#endif + + CHECK(multiple_bits_set, 0x8000000000000001ULL, true); + CHECK(multiple_bits_set, 0xc000000000000000ULL, true); +} + static void __init __constructor test_bitops(void) { test_ffs(); test_fls(); + + test_multiple_bits_set(); } diff --git a/xen/include/xen/bitops.h b/xen/include/xen/bitops.h index b8bb2ffcd337..74c0d04e6647 100644 --- a/xen/include/xen/bitops.h +++ b/xen/include/xen/bitops.h @@ -274,6 +274,16 @@ static always_inline __pure unsigned int fls64(uint64_t x) } } +/* + * Calculate if a value has two or more bits set. Always use this in + * preference to an expression of the form 'hweight(x) > 1'. + */ +#define multiple_bits_set(x) \ + ({ \ + typeof(x) _v = (x); \ + (_v & (_v - 1)) != 0; \ + }) + /* --------------------- Please tidy below here --------------------- */ #ifndef find_next_bit diff --git a/xen/include/xen/self-tests.h b/xen/include/xen/self-tests.h index 58484fe5a8ae..e5a6ba748aba 100644 --- a/xen/include/xen/self-tests.h +++ b/xen/include/xen/self-tests.h @@ -15,11 +15,14 @@ * * Clang < 8 can't fold constants through static inlines, causing this to * fail. Simply skip it for incredibly old compilers. + * + * N.B. fn is intentionally not bracketed to allow us to test function-like + * macros too. */ #if !defined(CONFIG_CC_IS_CLANG) || CONFIG_CLANG_VERSION >= 80000 #define COMPILE_CHECK(fn, val, res) \ do { \ - typeof((fn)(val)) real = (fn)(val); \ + typeof(fn(val)) real = fn(val); \ \ if ( !__builtin_constant_p(real) ) \ asm ( ".error \"'" STR(fn(val)) "' not compile-time constant\"" ); \ @@ -34,10 +37,13 @@ * Check that Xen's runtime logic for fn(val) gives the expected answer. This * requires using HIDE() to prevent the optimiser from collapsing the logic * into a constant. + * + * N.B. fn is intentionally not bracketed to allow us to test function-like + * macros too. */ #define RUNTIME_CHECK(fn, val, res) \ do { \ - typeof((fn)(val)) real = (fn)(HIDE(val)); \ + typeof(fn(val)) real = fn(HIDE(val)); \ \ if ( real != (res) ) \ panic("%s: %s(%s) expected %u, got %u\n", \
This will be used to simplify real logic in the following patch. Add compile and boot time testing as with other bitops. Because the expression is so simple, implement it as a function-like macro which is generic on the type of it's argument, rather than having multiple variants. Testing function-like macros needs a minor adjustments to the infrastructure in xen/self-tests.h to avoid bracketing the fn parameter. The utility of this outweighs the associated risks. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> CC: Bertrand Marquis <bertrand.marquis@arm.com> CC: Michal Orzel <michal.orzel@amd.com> CC: Oleksii Kurochko <oleksii.kurochko@gmail.com> CC: Shawn Anastasio <sanastasio@raptorengineering.com> Name inevitably subject to nitpicking. I'd prefer it to be shorter but I can't think of anything suitable. --- xen/common/bitops.c | 24 ++++++++++++++++++++++++ xen/include/xen/bitops.h | 10 ++++++++++ xen/include/xen/self-tests.h | 10 ++++++++-- 3 files changed, 42 insertions(+), 2 deletions(-)