| Message ID | 20250306162541.2633025-1-visitorckw@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce and use generic parity16/32/64 helper | expand |
On March 6, 2025 8:25:25 AM PST, Kuan-Wei Chiu <visitorckw@gmail.com> wrote: >Several parts of the kernel contain redundant implementations of parity >calculations for 16/32/64-bit values. Introduces generic >parity16/32/64() helpers in bitops.h, providing a standardized >and optimized implementation. > >Subsequent patches refactor various kernel components to replace >open-coded parity calculations with the new helpers, reducing code >duplication and improving maintainability. > >Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> >Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> >Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> >--- >In v3, I use parityXX() instead of the parity() macro since the >parity() macro may generate suboptimal code and requires special hacks >to make GCC happy. If anyone still prefers a single parity() macro, >please let me know. > >Additionally, I changed parityXX() << y users to !!parityXX() << y >because, unlike C++, C does not guarantee that true casts to int as 1. > >Changes in v3: >- Avoid using __builtin_parity. >- Change return type to bool. >- Drop parity() macro. >- Change parityXX() << y to !!parityXX() << y. > > >Changes in v2: >- Provide fallback functions for __builtin_parity() when the compiler > decides not to inline it >- Use __builtin_parity() when no architecture-specific implementation > is available >- Optimize for constant folding when val is a compile-time constant >- Add a generic parity() macro >- Drop the x86 bootflag conversion patch since it has been merged into > the tip tree > >v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/ >v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/ > >Kuan-Wei Chiu (16): > bitops: Change parity8() return type to bool > bitops: Add parity16(), parity32(), and parity64() helpers > media: media/test_drivers: Replace open-coded parity calculation with > parity8() > media: pci: cx18-av-vbi: Replace open-coded parity calculation with > parity8() > media: saa7115: Replace open-coded parity calculation with parity8() > serial: max3100: Replace open-coded parity calculation with parity8() > lib/bch: Replace open-coded parity calculation with parity32() > Input: joystick - Replace open-coded parity calculation with > parity32() > net: ethernet: oa_tc6: Replace open-coded parity calculation with > parity32() > wifi: brcm80211: Replace open-coded parity calculation with parity32() > drm/bridge: dw-hdmi: Replace open-coded parity calculation with > parity32() > mtd: ssfdc: Replace open-coded parity calculation with parity32() > fsi: i2cr: Replace open-coded parity calculation with parity32() > fsi: i2cr: Replace open-coded parity calculation with parity64() > Input: joystick - Replace open-coded parity calculation with > parity64() > nfp: bpf: Replace open-coded parity calculation with parity64() > > drivers/fsi/fsi-master-i2cr.c | 18 ++----- > .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 8 +-- > drivers/input/joystick/grip_mp.c | 17 +----- > drivers/input/joystick/sidewinder.c | 24 ++------- > drivers/media/i2c/saa7115.c | 12 +---- > drivers/media/pci/cx18/cx18-av-vbi.c | 12 +---- > .../media/test-drivers/vivid/vivid-vbi-gen.c | 8 +-- > drivers/mtd/ssfdc.c | 20 ++----- > drivers/net/ethernet/netronome/nfp/nfp_asm.c | 7 +-- > drivers/net/ethernet/oa_tc6.c | 19 ++----- > .../broadcom/brcm80211/brcmsmac/dma.c | 16 +----- > drivers/tty/serial/max3100.c | 3 +- > include/linux/bitops.h | 52 +++++++++++++++++-- > lib/bch.c | 14 +---- > 14 files changed, 77 insertions(+), 153 deletions(-) > !!x is used with a value that is not necessary booleanized already, and is exactly equivalent to (x ? true : false). It is totally redundant on a value known to be bool. If (int)true wasn't inherently 1, then !! wouldn't work either. There was a time when some code would use as a temporary hack: typedef enum { false, true } bool; ... when compiling on pre-C99 compilers; in that case a (bool) case wouldn't necessarily work as expected, whereas !! would. Furthermore, unlike (bool), !! works in the preprocessor.
On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > Several parts of the kernel contain redundant implementations of parity > calculations for 16/32/64-bit values. Introduces generic > parity16/32/64() helpers in bitops.h, providing a standardized > and optimized implementation. > > Subsequent patches refactor various kernel components to replace > open-coded parity calculations with the new helpers, reducing code > duplication and improving maintainability. > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > --- > In v3, I use parityXX() instead of the parity() macro since the > parity() macro may generate suboptimal code and requires special hacks > to make GCC happy. If anyone still prefers a single parity() macro, > please let me know. What is suboptimal and where exactly it matters? Have you actually measured it? > Additionally, I changed parityXX() << y users to !!parityXX() << y > because, unlike C++, C does not guarantee that true casts to int as 1. How comes? ANSI C99 exactly states: === true which expands to the integer constant 1, === thanks,
+Cc Waiman Long for bool cast to int discussion Hi Peter, On Thu, Mar 06, 2025 at 07:14:13PM -0800, H. Peter Anvin wrote: > On March 6, 2025 8:25:25 AM PST, Kuan-Wei Chiu <visitorckw@gmail.com> wrote: > >Several parts of the kernel contain redundant implementations of parity > >calculations for 16/32/64-bit values. Introduces generic > >parity16/32/64() helpers in bitops.h, providing a standardized > >and optimized implementation. > > > >Subsequent patches refactor various kernel components to replace > >open-coded parity calculations with the new helpers, reducing code > >duplication and improving maintainability. > > > >Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > >Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > >Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > >--- > >In v3, I use parityXX() instead of the parity() macro since the > >parity() macro may generate suboptimal code and requires special hacks > >to make GCC happy. If anyone still prefers a single parity() macro, > >please let me know. > > > >Additionally, I changed parityXX() << y users to !!parityXX() << y > >because, unlike C++, C does not guarantee that true casts to int as 1. > > > >Changes in v3: > >- Avoid using __builtin_parity. > >- Change return type to bool. > >- Drop parity() macro. > >- Change parityXX() << y to !!parityXX() << y. > > > > > >Changes in v2: > >- Provide fallback functions for __builtin_parity() when the compiler > > decides not to inline it > >- Use __builtin_parity() when no architecture-specific implementation > > is available > >- Optimize for constant folding when val is a compile-time constant > >- Add a generic parity() macro > >- Drop the x86 bootflag conversion patch since it has been merged into > > the tip tree > > > >v1: https://lore.kernel.org/lkml/20250223164217.2139331-1-visitorckw@gmail.com/ > >v2: https://lore.kernel.org/lkml/20250301142409.2513835-1-visitorckw@gmail.com/ > > > >Kuan-Wei Chiu (16): > > bitops: Change parity8() return type to bool > > bitops: Add parity16(), parity32(), and parity64() helpers > > media: media/test_drivers: Replace open-coded parity calculation with > > parity8() > > media: pci: cx18-av-vbi: Replace open-coded parity calculation with > > parity8() > > media: saa7115: Replace open-coded parity calculation with parity8() > > serial: max3100: Replace open-coded parity calculation with parity8() > > lib/bch: Replace open-coded parity calculation with parity32() > > Input: joystick - Replace open-coded parity calculation with > > parity32() > > net: ethernet: oa_tc6: Replace open-coded parity calculation with > > parity32() > > wifi: brcm80211: Replace open-coded parity calculation with parity32() > > drm/bridge: dw-hdmi: Replace open-coded parity calculation with > > parity32() > > mtd: ssfdc: Replace open-coded parity calculation with parity32() > > fsi: i2cr: Replace open-coded parity calculation with parity32() > > fsi: i2cr: Replace open-coded parity calculation with parity64() > > Input: joystick - Replace open-coded parity calculation with > > parity64() > > nfp: bpf: Replace open-coded parity calculation with parity64() > > > > drivers/fsi/fsi-master-i2cr.c | 18 ++----- > > .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 8 +-- > > drivers/input/joystick/grip_mp.c | 17 +----- > > drivers/input/joystick/sidewinder.c | 24 ++------- > > drivers/media/i2c/saa7115.c | 12 +---- > > drivers/media/pci/cx18/cx18-av-vbi.c | 12 +---- > > .../media/test-drivers/vivid/vivid-vbi-gen.c | 8 +-- > > drivers/mtd/ssfdc.c | 20 ++----- > > drivers/net/ethernet/netronome/nfp/nfp_asm.c | 7 +-- > > drivers/net/ethernet/oa_tc6.c | 19 ++----- > > .../broadcom/brcm80211/brcmsmac/dma.c | 16 +----- > > drivers/tty/serial/max3100.c | 3 +- > > include/linux/bitops.h | 52 +++++++++++++++++-- > > lib/bch.c | 14 +---- > > 14 files changed, 77 insertions(+), 153 deletions(-) > > > > !!x is used with a value that is not necessary booleanized already, and is exactly equivalent to (x ? true : false). It is totally redundant on a value known to be bool. > I used to believe that casting a boolean variable to int would always result in 0 or 1 until a few months ago when Waiman Long explicitly pointed out during a review that C does not guarantee this. So I revisited the C11 standard, which states that casting to _Bool always results in 0 or 1 [1]. Another section specifies that bool, true, and false are macros defined in <stdbool.h>, with true expanding to 1 and false to 0. However, these macros can be #undef and redefined to other values [2]. I'm not sure if this is sufficient to conclude that casting bool to int will always result in 0 or 1, but if the consensus is that it does, I'll remove the !! hack in the next version. > If (int)true wasn't inherently 1, then !! wouldn't work either. > The C standard guarantees that the ! operator returns an int, either 0 or 1. So regardless of how true casts, using !! should work. Right? > There was a time when some code would use as a temporary hack: > > typedef enum { false, true } bool; > > ... when compiling on pre-C99 compilers; in that case a (bool) case wouldn't necessarily work as expected, whereas !! would. Furthermore, unlike (bool), !! works in the preprocessor. I'm not entirely sure how !! works in the preprocessor. I always thought it was handled by the compiler. Could you elaborate on this? Regards, Kuan-Wei [1]: 6.3.1.2 Boolean type 1 When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.59) [2]: 7.18 Boolean type and values <stdbool.h> 1 The header <stdbool.h> defines four macros. 2 The macro bool expands to _Bool. 3 The remaining three macros are suitable for use in #if preprocessing directives. They are true which expands to the integer constant 1, false which expands to the integer constant 0, and _ _bool_true_false_are_defined which expands to the integer constant 1. 4 Notwithstanding the provisions of 7.1.3, a program may undefine and perhaps then redefine the macros bool, true, and false. 259)
On 07. 03. 25, 10:19, Kuan-Wei Chiu wrote: > I used to believe that casting a boolean variable to int would always > result in 0 or 1 until a few months ago when Waiman Long explicitly > pointed out during a review that C does not guarantee this. > > So I revisited the C11 standard, which states that casting to _Bool > always results in 0 or 1 [1]. Another section specifies that bool, > true, and false are macros defined in <stdbool.h>, with true expanding > to 1 and false to 0. However, these macros can be #undef and redefined > to other values [2]. Note that we do not have/use user's stdbool.h in kernel at all. Instead, in linux/stddef.h, we define: enum { false = 0, true = 1 }; So all is blue. thanks,
On Fri, Mar 07, 2025 at 07:57:48AM +0100, Jiri Slaby wrote: > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > > Several parts of the kernel contain redundant implementations of parity > > calculations for 16/32/64-bit values. Introduces generic > > parity16/32/64() helpers in bitops.h, providing a standardized > > and optimized implementation. > > > > Subsequent patches refactor various kernel components to replace > > open-coded parity calculations with the new helpers, reducing code > > duplication and improving maintainability. > > > > Co-developed-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Signed-off-by: Yu-Chun Lin <eleanor15x@gmail.com> > > Signed-off-by: Kuan-Wei Chiu <visitorckw@gmail.com> > > --- > > In v3, I use parityXX() instead of the parity() macro since the > > parity() macro may generate suboptimal code and requires special hacks > > to make GCC happy. If anyone still prefers a single parity() macro, > > please let me know. > > What is suboptimal and where exactly it matters? Have you actually measured > it? I asked exactly this question at least 3 times, and have never received perf tests or asm listings - nothing. I've never received any comments from driver maintainers about how performance of the parity() is important for them, as well. With the absence of _any_ feedback, I'm not going to take this series, of course, for the reason: overengineering. With that said, the simplest way would be replacing parity8(u8) with parity(u64) 'one size fits all' thing. I even made a one extra step, suggesting a macro that would generate a better code for smaller types with almost no extra maintenance burden. This is another acceptable option to me. Thanks, Yury