On Tue, May 10, 2022 at 8:48 AM Yury Norov <[email protected]> wrote:
>
> arch/xtensa/kernel/traps.c and include/linux/log2.h define very similar
> functions with different behaviour. XTENSA defines IS_POW2(), and
> log2.h defines is_power_of_2(). The difference is that IS_POW2()
> considers 0 as power of 2, while is_power_of_2() - does not.
>
> This discrepancy may confuse reader. From mathematical point of view,
> 0 is not a power of 2. So let's introduce macro MANY_BITS(), which
> returns true if 2 or more bits are set in a number (which is what
> XTENSA actually needs), and use it in is_power_of_2().
>
> CC: Alexei Starovoitov <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: Chris Zankel <[email protected]>
> CC: Christophe Leroy <[email protected]>
> CC: Eric W. Biederman <[email protected]>
> CC: Kumar Kartikeya Dwivedi <[email protected]>
> CC: Max Filippov <[email protected]>
> CC: Toke Høiland-Jørgensen <[email protected]>
> CC: [email protected]
> CC: [email protected]
> Signed-off-by: Yury Norov <[email protected]>
> ---
> arch/xtensa/kernel/traps.c | 5 +----
> include/linux/bitops.h | 3 +++
> include/linux/log2.h | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/xtensa/kernel/traps.c b/arch/xtensa/kernel/traps.c
> index 138a86fbe9d7..040ec38bfce2 100644
> --- a/arch/xtensa/kernel/traps.c
> +++ b/arch/xtensa/kernel/traps.c
> @@ -203,10 +203,7 @@ static void do_multihit(struct pt_regs *regs)
>
> #if XTENSA_FAKE_NMI
>
> -#define IS_POW2(v) (((v) & ((v) - 1)) == 0)
> -
> -#if !(PROFILING_INTLEVEL == XCHAL_EXCM_LEVEL && \
> - IS_POW2(XTENSA_INTLEVEL_MASK(PROFILING_INTLEVEL)))
> +#if (MANY_BITS(XTENSA_INTLEVEL_MASK(PROFILING_INTLEVEL)) || PROFILING_INTLEVEL != XCHAL_EXCM_LEVEL)
> #warning "Fake NMI is requested for PMM, but there are other IRQs at or above its level."
> #warning "Fake NMI will be used, but there will be a bugcheck if one of those IRQs fire."
>
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 7aaed501f768..96bc6a2552d6 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -21,6 +21,9 @@
> #define BITS_TO_U32(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(u32))
> #define BITS_TO_BYTES(nr) __KERNEL_DIV_ROUND_UP(nr, BITS_PER_TYPE(char))
>
> +/* Return: nonzero if 2 or more bits are set */
> +#define MANY_BITS(n) ((n) & ((n) - 1))
> +
> extern unsigned int __sw_hweight8(unsigned int w);
> extern unsigned int __sw_hweight16(unsigned int w);
> extern unsigned int __sw_hweight32(unsigned int w);
> diff --git a/include/linux/log2.h b/include/linux/log2.h
> index 9f30d087a128..335b9dbd302d 100644
> --- a/include/linux/log2.h
> +++ b/include/linux/log2.h
> @@ -44,7 +44,7 @@ int __ilog2_u64(u64 n)
> static inline __attribute__((const))
> bool is_power_of_2(unsigned long n)
> {
> - return (n != 0 && ((n & (n - 1)) == 0));
> + return n != 0 && !MANY_BITS(n);
> }
Please don't. Open coded version is much easier to read.
From: Alexei Starovoitov
> Sent: 10 May 2022 17:51
...
> +/* Return: nonzero if 2 or more bits are set */
> +#define MANY_BITS(n) ((n) & ((n) - 1))
You can't have a macro that expands its argument twice.
...
> > static inline __attribute__((const))
> > bool is_power_of_2(unsigned long n)
> > {
> > - return (n != 0 && ((n & (n - 1)) == 0));
> > + return n != 0 && !MANY_BITS(n);
> > }
>
> Please don't. Open coded version is much easier to read.
Especially if you remove all the spare parenthesis.
return n && !(n & (n - 1));
I bet a lot of callers know the value is non-zero.
I suspect you'll find at least one caller that uses
is_power_of_2() assuming it is !(n & (n - 1)) and
so is wrong for zero.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, May 10, 2022 at 05:54:13PM +0000, David Laight wrote:
> From: Alexei Starovoitov
> > Sent: 10 May 2022 17:51
> ...
> > +/* Return: nonzero if 2 or more bits are set */
> > +#define MANY_BITS(n) ((n) & ((n) - 1))
>
> You can't have a macro that expands its argument twice.
Yes, I'll fix it.
> ...
> > > static inline __attribute__((const))
> > > bool is_power_of_2(unsigned long n)
> > > {
> > > - return (n != 0 && ((n & (n - 1)) == 0));
> > > + return n != 0 && !MANY_BITS(n);
> > > }
> >
> > Please don't. Open coded version is much easier to read.
To me the human-readable version is easier to read. Still, if you thing
that n & (n - 1) is simpler, what for this function is needed at all?
> Especially if you remove all the spare parenthesis.
> return n && !(n & (n - 1));
>
> I bet a lot of callers know the value is non-zero.
>
> I suspect you'll find at least one caller that uses
> is_power_of_2() assuming it is !(n & (n - 1)) and
> so is wrong for zero.
Another thing is that despite __attribute__(const), gcc sometimes doesn't
recognize it as constant expression, and people have to workaround it.
XTENSA is the example for 1st case, and for the 2nd:
arch/powerpc/mm/init-common.c:
unsigned long minalign = max(MAX_PGTABLE_INDEX_SIZE + 1,
HUGEPD_SHIFT_MASK + 1);
/* It would be nice if this was a BUILD_BUG_ON(), but at the
* moment, gcc doesn't seem to recognize is_power_of_2
* as a constant expression, so so much for that. */
BUG_ON(!is_power_of_2(minalign));
This convinced me that we need a simple macro that is decoupled with
pow_2 semantics and can be used in another macros like BUILD_BUG_ON().
Thanks,
Yury
On 10/05/2022 21.11, Yury Norov wrote:
> Another thing is that despite __attribute__(const), gcc sometimes doesn't
> recognize it as constant expression,
An ICE, Integer Constant Expression, is a C language thing.
__attribute__(const) has nothing to do with that, and no use of that
attribute is ever gonna convince gcc to treat an expression containing a
function call as an ICE [C++ is of course a whole different story].
The __attribute__(const) on a static inline function is utterly
pointless, the compiler can certainly see for itself that the function
has no side effects and that the expression can be CSE'ed.
All that aside, please drop that MANY_BITS thing. It will not make any
code any easier to read (MANY, is that four or five? or is it "more than
half the bit width"?). The n&(n-1) pattern is a well-known idiom, I
expect anybody hacking on an OS kernel to recognize
Rasmus