On Tue, Feb 25, 2025 at 09:29:51PM +0800, Kuan-Wei Chiu wrote:
> 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
>
>
Oops... I made a lot of fat-finger mistakes. Here's the correct one.
diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index c1cb53cf2f0f..427e4c06055e 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -260,6 +260,21 @@ static inline int parity8(u8 val)
return (0x6996 >> (val & 0xf)) & 1;
}
+static inline int parity16(u16 val)
+{
+ return parity8(val ^ (val >> 8));
+}
+
+static inline int parity32(u32 val)
+{
+ return parity16(val ^ (val >> 16));
+}
+
+static inline int parity64(u64 val)
+{
+ 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
> >
