Hi Yury,
On Mon, Feb 24, 2025 at 02:27:03PM -0500, Yury Norov wrote:
> On Mon, Feb 24, 2025 at 12:42:02AM +0800, Kuan-Wei Chiu wrote:
> > Several parts of the kernel open-code parity calculations using
> > different methods. Add a generic parity64() helper implemented with the
> > same efficient approach as parity8().
>
> No reason to add parity32() and parity64() in separate patches
Ack.
>
> > Co-developed-by: Yu-Chun Lin <eleanor15x@xxxxxxxxx>
> > Signed-off-by: Yu-Chun Lin <eleanor15x@xxxxxxxxx>
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@xxxxxxxxx>
> > ---
> > include/linux/bitops.h | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> > index fb13dedad7aa..67677057f5e2 100644
> > --- a/include/linux/bitops.h
> > +++ b/include/linux/bitops.h
> > @@ -281,6 +281,28 @@ static inline int parity32(u32 val)
> > return (0x6996 >> (val & 0xf)) & 1;
> > }
> >
> > +/**
> > + * parity64 - get the parity of an u64 value
> > + * @value: the value to be examined
> > + *
> > + * Determine the parity of the u64 argument.
> > + *
> > + * Returns:
> > + * 0 for even parity, 1 for odd parity
> > + */
> > +static inline int parity64(u64 val)
> > +{
> > + /*
> > + * One explanation of this algorithm:
> > + * https://funloop.org/codex/problem/parity/README.html
>
> This is already referenced in sources. No need to spread it for more.
Ack.
>
> > + */
> > + val ^= val >> 32;
> > + val ^= val >> 16;
> > + val ^= val >> 8;
> > + val ^= val >> 4;
> > + return (0x6996 >> (val & 0xf)) & 1;
>
> It's better to avoid duplicating the same logic again and again.
Ack.
>
> > +}
> > +
>
> So maybe make it a macro?
>
>
> From f17a28ae3429f49825d65ebc0f7717c6a191a3e2 Mon Sep 17 00:00:00 2001
> From: Yury Norov <yury.norov@xxxxxxxxx>
> Date: Mon, 24 Feb 2025 14:14:27 -0500
> Subject: [PATCH] bitops: generalize parity8()
>
> The generic parity calculation approach may be easily generalized for
> other standard types. Do that and drop sub-optimal implementation of
> parity calculation in x86 code.
>
> Signed-off-by: Yury Norov [NVIDIA] <yury.norov@xxxxxxxxx>
> ---
> arch/x86/kernel/bootflag.c | 14 +-----------
> include/linux/bitops.h | 47 +++++++++++++++++++++++++++-----------
> 2 files changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/kernel/bootflag.c b/arch/x86/kernel/bootflag.c
> index 3fed7ae58b60..4a85c69a28f8 100644
> --- a/arch/x86/kernel/bootflag.c
> +++ b/arch/x86/kernel/bootflag.c
> @@ -2,6 +2,7 @@
> /*
> * Implement 'Simple Boot Flag Specification 2.0'
> */
> +#include <linux/bitops.h>
> #include <linux/types.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> @@ -20,19 +21,6 @@
>
> int sbf_port __initdata = -1; /* set via acpi_boot_init() */
>
> -static int __init parity(u8 v)
> -{
> - int x = 0;
> - int i;
> -
> - for (i = 0; i < 8; i++) {
> - x ^= (v & 1);
> - v >>= 1;
> - }
> -
> - return x;
> -}
> -
> static void __init sbf_write(u8 v)
> {
> unsigned long flags;
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index c1cb53cf2f0f..29601434f5f4 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -230,10 +230,10 @@ static inline int get_count_order_long(unsigned long l)
> }
>
> /**
> - * parity8 - get the parity of an u8 value
> + * parity - get the parity of a value
> * @value: the value to be examined
> *
> - * Determine the parity of the u8 argument.
> + * Determine parity of the argument.
> *
> * Returns:
> * 0 for even parity, 1 for odd parity
> @@ -241,24 +241,45 @@ static inline int get_count_order_long(unsigned long l)
> * Note: This function informs you about the current parity. Example to bail
> * out when parity is odd:
> *
> - * if (parity8(val) == 1)
> + * if (parity(val) == 1)
> * return -EBADMSG;
> *
> * If you need to calculate a parity bit, you need to draw the conclusion from
> * this result yourself. Example to enforce odd parity, parity bit is bit 7:
> *
> - * if (parity8(val) == 0)
> + * if (parity(val) == 0)
> * val ^= BIT(7);
> + *
> + * One explanation of this algorithm:
> + * https://funloop.org/codex/problem/parity/README.html
> */
> -static inline int parity8(u8 val)
> -{
> - /*
> - * One explanation of this algorithm:
> - * https://funloop.org/codex/problem/parity/README.html
> - */
> - val ^= val >> 4;
> - return (0x6996 >> (val & 0xf)) & 1;
> -}
> +#define parity(val) \
> +({ \
> + u64 __v = (val); \
> + int __ret; \
> + switch (BITS_PER_TYPE(val)) { \
> + case 64: \
> + __v ^= __v >> 32; \
> + fallthrough; \
> + case 32: \
> + __v ^= __v >> 16; \
> + fallthrough; \
> + case 16: \
> + __v ^= __v >> 8; \
> + fallthrough; \
> + case 8: \
> + __v ^= __v >> 4; \
> + __ret = (0x6996 >> (__v & 0xf)) & 1; \
> + break; \
> + default: \
> + BUILD_BUG(); \
> + } \
> + __ret; \
> +})
> +
> +#define parity8(val) parity((u8)(val))
> +#define parity32(val) parity((u32)(val))
> +#define parity64(val) parity((u64)(val))
>
What do you think about using these inline functions instead of macros?
Except for parity8(), each function is a single line and follows the
same logic. I find inline functions more readable, and coding-style.rst
also recommends them over macros.
Regards,
Kuan-Wei
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index c1cb53cf2f0f..d518a382f1fe 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -260,6 +260,26 @@ static inline int parity8(u8 val)
return (0x6996 >> (val & 0xf)) & 1;
}
+static inline parity16(u16 val)
+{
+ return parity8(val ^ (val >> 8));
+}
+
+static inline parity16(u16 val)
+{
+ return parity8(val ^ (val >> 8));
+}
+
+static inline parity32(u32)
+{
+ return parity16(val ^ (val >> 16));
+}
+
+static inline parity64(u64)
+{
+ return parity32(val ^ (val >> 32));
+}
+
/**
* __ffs64 - find first set bit in a 64 bit word
* @word: The 64 bit word
> /**
> * __ffs64 - find first set bit in a 64 bit word
> --
> 2.43.0
>
