Message-ID: <8fc262c0af694a2e8f5f71b6574a61af@AcuMS.aculab.com>
Date: Tue, 17 Dec 2024 09:56:38 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Yury Norov' <yury.norov@...il.com>
CC: "'linux-kernel@...r.kernel.org'" <linux-kernel@...r.kernel.org>, "'Rasmus
 Villemoes'" <linux@...musvillemoes.dk>, 'Andrew Morton'
	<akpm@...ux-foundation.org>, 'Masahiro Yamada' <masahiroy@...nel.org>,
	'Vincent Mailhol' <mailhol.vincent@...adoo.fr>, 'Linus Torvalds'
	<torvalds@...ux-foundation.org>
Subject: RE: [PATCH v2 next] linux/bits.h: Simplify GENMASK()

From: Yury Norov
> Sent: 17 December 2024 02:18
> 
> On Mon, Dec 16, 2024 at 09:39:23AM +0000, David Laight wrote:
> > Change 95b980d62d52c replaced ~0ul and ~0ull with ~UL(0) and ~ULL(0)
> > in the GENMASK() defines as part of a change to allow the bitops
> > definitions be used from assembly.
> >
> > The definitions have since been moved to a uapi header which
> > probably shouldn't require some other header be included first.
> 
> Sorry, I don't understand that. Are you saying that uapi headers
> should not include one another? What is exactly wrong with that
> for you? Or did you mean something else?

It doesn't explicitly include whatever defines UL(x)

> > The definition of __GENMASK() is also overcomplicated partially
> > due to avoiding overflow warnings from shifting ~0u left.
> >
> > Implement GENMASK() using the simpler (1u << hi) * 2 - (1u << lo) formula.
> 
> I don't think that this formula is any simpler than the original one.

The original one has a horrid double-subtract to avoid a warning
from overflow when shifting ~0u left.

> > This doesn't rely on right shifts
> 
> What is wrong with right shifts?
> 
> > and doesn't need to know the number
> > of bits in the integral type.
> 
> What is wrong in BITS_PER_LONG?

Well apart from needing to have included the correct header it is
wrong for the expansions when ASSEMBLER is defined.

> > It can be used for different types by just changing the type of the 1u.
> > __GENMASK() __GENMASK_ULL() and __GENMASK_U128() can now implemeted
> > using a single ___GENMASK(one, hi, lo).
> 
> I like idea of generic implementation or different flavors of GENMASK().
> I even proposed something similar back then for fixed-type genmasks:
> 
> https://lkml.org/lkml/2023/6/21/1734

The _U8 and _U16 versions are broken (and pretty pointless).
The result type is 'signed int'.
You have to cast the result of the last arithmetic operation,
and even then the result will be converted back to 'signed int'
pretty much as soon as it is used.

> 
> Both you and I do the same - provide type as a macro parameter. I like
> my way of doing it a bit more because it's more explicit. But now that
> we have _Generic(), we don't need this hack at all.
> 
> >
> > Overflow warnings (from shifting out the MSB) are avoided by subtracting 1
> > before the multiply and adding two back in later.
> > The complers see straight through the subterfuge when generating code.
> 
> Ironically... You're trying to simplify a part that silences Woverflow, and
> end up by adding another part that silences Woverflow...

It is a much simpler fix :-)
The existing isn't obviously generating the correct value.

> 
> > Since there are already conditionals for ASSEMBLY in bits.h, for ASSEMBLY
> > directly expand GENMASK() and GENMASK_ULL() as ___GENMASK(1, hi, lo)
> > rather than through the __GENMASK() and __GENMASK_ULL() uapi defines.
> > Remove the UL(x) and ULL(x) from the uapi file.
> >
> > GENMASK() and GENMASK_ULL() now generate the correct values when
> > ASSEMBLY is defined.
> > Fortunately they've never been used.
> 
> They were used before. They are not used now - that's true.

And they've never generated the correct value!

> Can you explain in details how are those macros are broken for assemblers?
> Can you add a test?

Well UL(x) and ULL(x) both expand to (x) so GENMASK() and GENMASK_ULL()
cannot both be correct.

I suspect they are also signed so '~1 >> n' doesn't have the correct
value at all.

> > Rename 'h' to 'hi' and 'l' to 'lo' because 'l' looks like '1' in many fonts.
> 
> No please don't!

Most of the lines got changed anyway.

	David

> 
> This type of changes don't help at all. They effectively wipe all the
> history for absolutely nothing. If your fonts mess characters, please
> use other fonts.
> 
> > Signed-off-by: David Laight <david.laight@...lab.com>
> > ---
> >
> > v2: '__uint128' => 'unsigned __int128'
> >
> >  include/linux/bits.h      | 43 ++++++++++++++++-----------------------
> >  include/uapi/linux/bits.h | 15 +++++++-------
> >  2 files changed, 24 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/linux/bits.h b/include/linux/bits.h
> > index 60044b608817..d5cf0ec22e43 100644
> > --- a/include/linux/bits.h
> > +++ b/include/linux/bits.h
> > @@ -14,41 +14,32 @@
> >  #define BITS_PER_BYTE		8
> >
> >  /*
> > - * Create a contiguous bitmask starting at bit position @l and ending at
> > - * position @h. For example
> > + * Create a contiguous bitmask starting at bit position @lo and ending at
> > + * position @hi. For example
> >   * GENMASK_ULL(39, 21) gives us the 64bit vector 0x000000ffffe00000.
> >   */
> >  #if !defined(__ASSEMBLY__)
> >  #include <linux/build_bug.h>
> > -#define GENMASK_INPUT_CHECK(h, l) \
> > +#define GENMASK_INPUT_CHECK(hi, lo) \
> >  	(BUILD_BUG_ON_ZERO(__builtin_choose_expr( \
> > -		__is_constexpr((l) > (h)), (l) > (h), 0)))
> > -#else
> > -/*
> > - * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > - * disable the input check if that is the case.
> > - */
> > -#define GENMASK_INPUT_CHECK(h, l) 0
> > -#endif
> > +		__is_constexpr((lo) > (hi)), (lo) > (hi), 0)))
> >
> > -#define GENMASK(h, l) \
> > -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> > -#define GENMASK_ULL(h, l) \
> > -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l))
> > +#define GENMASK(hi, lo) \
> > +	(GENMASK_INPUT_CHECK(hi, lo) + __GENMASK(hi, lo))
> > +#define GENMASK_ULL(hi, lo) \
> > +	(GENMASK_INPUT_CHECK(hi, lo) + __GENMASK_ULL(hi, lo))
> >
> > -#if !defined(__ASSEMBLY__)
> > +#define GENMASK_U128(hi, lo) \
> > +	(GENMASK_INPUT_CHECK(hi, lo) + __GENMASK_U128(hi, lo))
> > +#else
> >  /*
> > - * Missing asm support
> > - *
> > - * __GENMASK_U128() depends on _BIT128() which would not work
> > - * in the asm code, as it shifts an 'unsigned __init128' data
> > - * type instead of direct representation of 128 bit constants
> > - * such as long and unsigned long. The fundamental problem is
> > - * that a 128 bit constant will get silently truncated by the
> > - * gcc compiler.
> > + * BUILD_BUG_ON_ZERO is not available in h files included from asm files,
> > + * 128bit exprssions don't work, neither can C 0UL (etc) constants be used.
> > + * These definitions only have to work for constants and don't require
> > + * that ~0 have any specific number of set bits.
> >   */
> > -#define GENMASK_U128(h, l) \
> > -	(GENMASK_INPUT_CHECK(h, l) + __GENMASK_U128(h, l))
> > +#define GENMASK(hi, lo) ___GENMASK(1, hi, lo)
> > +#define GENMASK_ULL(hi, lo) ___GENMASK(1, hi, lo)
> >  #endif
> >
> >  #endif	/* __LINUX_BITS_H */
> > diff --git a/include/uapi/linux/bits.h b/include/uapi/linux/bits.h
> > index 5ee30f882736..a25d9dfb7072 100644
> > --- a/include/uapi/linux/bits.h
> > +++ b/include/uapi/linux/bits.h
> > @@ -4,15 +4,14 @@
> >  #ifndef _UAPI_LINUX_BITS_H
> >  #define _UAPI_LINUX_BITS_H
> >
> > -#define __GENMASK(h, l) \
> > -        (((~_UL(0)) - (_UL(1) << (l)) + 1) & \
> > -         (~_UL(0) >> (__BITS_PER_LONG - 1 - (h))))
> > +/* Result is '(1u << (hi + 1)) - (1u << lo)' coded to avoid overflow. */
> > +#define ___GENMASK(one, hi, lo) \
> > +	((((one) << (hi)) - 1) * 2 + 1 - (((one) << (lo)) - 1))
> >
> > -#define __GENMASK_ULL(h, l) \
> > -        (((~_ULL(0)) - (_ULL(1) << (l)) + 1) & \
> > -         (~_ULL(0) >> (__BITS_PER_LONG_LONG - 1 - (h))))
> > +#define __GENMASK(hi, lo) ___GENMASK(1UL, hi, lo)
> >
> > -#define __GENMASK_U128(h, l) \
> > -	((_BIT128((h)) << 1) - (_BIT128(l)))
> > +#define __GENMASK_ULL(hi, lo) ___GENMASK(1ULL, hi, lo)
> > +
> > +#define __GENMASK_U128(hi, lo) ___GENMASK((unsigned __int128)1, hi, lo)
> >
> >  #endif /* _UAPI_LINUX_BITS_H */
> > --
> > 2.17.1
> >
> > -
> > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> > Registration No: 1397386 (Wales)

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists