| 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(-) > (int)true most definitely is guaranteed to be 1.
On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > Change return type to bool for better clarity. Update the kernel doc > comment accordingly, including fixing "@value" to "@val" and adjusting > examples. Also mark the function with __attribute_const__ to allow > potential compiler optimizations. > > 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> > --- > include/linux/bitops.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index c1cb53cf2f0f..44e5765b8bec 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > /** > * parity8 - get the parity of an u8 value > - * @value: the value to be examined > + * @val: the value to be examined > * > * Determine the parity of the u8 argument. > * > * Returns: > - * 0 for even parity, 1 for odd parity > + * false for even parity, true for odd parity This occurs somehow inverted to me. When something is in parity means that it has equal number of 1s and 0s. I.e. return true for even distribution. Dunno what others think? Or perhaps this should be dubbed odd_parity() when bool is returned? Then you'd return true for odd. 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)
Hi Jiri, 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? > In the previous thread, David and Yury had different opinions regarding the implementation details of the parity() macro. I am trying to find a solution that satisfies most people while keeping it as simple as possible. I cannot point to any specific users who are particularly concerned about efficiency, so personally, I am not really concerned about the generated code either. However, I am not a fan of the #if gcc #else approach, and Yury also mentioned that he does not like the >> 16 >> 16 hack. At the same time, David pointed out that GCC might generate double-register math. Given these concerns, I leaned toward reverting to the parityXX() approach. If you still prefer using the parity() macro, we can revisit and discuss its implementation details further. > > 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, > === > I gave a more detailed response in my reply to Peter. If we can confirm that casting bool to int will only result in 1 or 0, I will remove the !! hack in the next version. Regards, Kuan-Wei
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,
* Jiri Slaby <jirislaby@kernel.org> wrote: > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > > Change return type to bool for better clarity. Update the kernel doc > > comment accordingly, including fixing "@value" to "@val" and adjusting > > examples. Also mark the function with __attribute_const__ to allow > > potential compiler optimizations. > > > > 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> > > --- > > include/linux/bitops.h | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > > index c1cb53cf2f0f..44e5765b8bec 100644 > > --- a/include/linux/bitops.h > > +++ b/include/linux/bitops.h > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > /** > > * parity8 - get the parity of an u8 value > > - * @value: the value to be examined > > + * @val: the value to be examined > > * > > * Determine the parity of the u8 argument. > > * > > * Returns: > > - * 0 for even parity, 1 for odd parity > > + * false for even parity, true for odd parity > > This occurs somehow inverted to me. When something is in parity means that > it has equal number of 1s and 0s. I.e. return true for even distribution. > Dunno what others think? Or perhaps this should be dubbed odd_parity() when > bool is returned? Then you'd return true for odd. OTOH: - '0' is an even number and is returned for even parity, - '1' is an odd number and is returned for odd parity. Thanks, Ingo
On 07. 03. 25, 12:38, Ingo Molnar wrote: > > * Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >>> Change return type to bool for better clarity. Update the kernel doc >>> comment accordingly, including fixing "@value" to "@val" and adjusting >>> examples. Also mark the function with __attribute_const__ to allow >>> potential compiler optimizations. >>> >>> 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> >>> --- >>> include/linux/bitops.h | 10 +++++----- >>> 1 file changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h >>> index c1cb53cf2f0f..44e5765b8bec 100644 >>> --- a/include/linux/bitops.h >>> +++ b/include/linux/bitops.h >>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >>> /** >>> * parity8 - get the parity of an u8 value >>> - * @value: the value to be examined >>> + * @val: the value to be examined >>> * >>> * Determine the parity of the u8 argument. >>> * >>> * Returns: >>> - * 0 for even parity, 1 for odd parity >>> + * false for even parity, true for odd parity >> >> This occurs somehow inverted to me. When something is in parity means that >> it has equal number of 1s and 0s. I.e. return true for even distribution. >> Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> bool is returned? Then you'd return true for odd. > > OTOH: > > - '0' is an even number and is returned for even parity, > - '1' is an odd number and is returned for odd parity. Yes, that used to make sense for me. For bool/true/false, it no longer does. But as I wrote, it might be only me... thanks,
* Jiri Slaby <jirislaby@kernel.org> wrote: > On 07. 03. 25, 12:38, Ingo Molnar wrote: > > > > * Jiri Slaby <jirislaby@kernel.org> wrote: > > > > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > > > > Change return type to bool for better clarity. Update the kernel doc > > > > comment accordingly, including fixing "@value" to "@val" and adjusting > > > > examples. Also mark the function with __attribute_const__ to allow > > > > potential compiler optimizations. > > > > > > > > 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> > > > > --- > > > > include/linux/bitops.h | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > > > > index c1cb53cf2f0f..44e5765b8bec 100644 > > > > --- a/include/linux/bitops.h > > > > +++ b/include/linux/bitops.h > > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > > > > /** > > > > * parity8 - get the parity of an u8 value > > > > - * @value: the value to be examined > > > > + * @val: the value to be examined > > > > * > > > > * Determine the parity of the u8 argument. > > > > * > > > > * Returns: > > > > - * 0 for even parity, 1 for odd parity > > > > + * false for even parity, true for odd parity > > > > > > This occurs somehow inverted to me. When something is in parity means that > > > it has equal number of 1s and 0s. I.e. return true for even distribution. > > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when > > > bool is returned? Then you'd return true for odd. > > > > OTOH: > > > > - '0' is an even number and is returned for even parity, > > - '1' is an odd number and is returned for odd parity. > > Yes, that used to make sense for me. For bool/true/false, it no longer does. > But as I wrote, it might be only me... No strong opinion on this from me either, I'd guess existing practice with other parity functions should probably control. (If a coherent praxis exists.). Thanks, Ingo
On March 7, 2025 4:13:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > >* Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >> > >> > * Jiri Slaby <jirislaby@kernel.org> wrote: >> > >> > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >> > > > Change return type to bool for better clarity. Update the kernel doc >> > > > comment accordingly, including fixing "@value" to "@val" and adjusting >> > > > examples. Also mark the function with __attribute_const__ to allow >> > > > potential compiler optimizations. >> > > > >> > > > 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> >> > > > --- >> > > > include/linux/bitops.h | 10 +++++----- >> > > > 1 file changed, 5 insertions(+), 5 deletions(-) >> > > > >> > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h >> > > > index c1cb53cf2f0f..44e5765b8bec 100644 >> > > > --- a/include/linux/bitops.h >> > > > +++ b/include/linux/bitops.h >> > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >> > > > /** >> > > > * parity8 - get the parity of an u8 value >> > > > - * @value: the value to be examined >> > > > + * @val: the value to be examined >> > > > * >> > > > * Determine the parity of the u8 argument. >> > > > * >> > > > * Returns: >> > > > - * 0 for even parity, 1 for odd parity >> > > > + * false for even parity, true for odd parity >> > > >> > > This occurs somehow inverted to me. When something is in parity means that >> > > it has equal number of 1s and 0s. I.e. return true for even distribution. >> > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> > > bool is returned? Then you'd return true for odd. >> > >> > OTOH: >> > >> > - '0' is an even number and is returned for even parity, >> > - '1' is an odd number and is returned for odd parity. >> >> Yes, that used to make sense for me. For bool/true/false, it no longer does. >> But as I wrote, it might be only me... > >No strong opinion on this from me either, I'd guess existing practice >with other parity functions should probably control. (If a coherent >praxis exists.). > >Thanks, > > Ingo Instead of "bool" think of it as "bit" and it makes more sense
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
Hi Yury, On Fri, Mar 07, 2025 at 10:55:13AM -0500, Yury Norov wrote: > 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. > To be clear, I use parityXX() was mainly because you dislike the >> 16 >> 16 hack, and I dislike the #if gcc #else hack—not due to performance or generated code considerations. > With the absence of _any_ feedback, I'm not going to take this series, > of course, for the reason: overengineering. > I'm quite surprised that three separate one-line functions are considered overengineering compared to a multi-line approach that requires special handling to satisfy gcc. > 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. > I'm fine with unifying everything to a single parity(u64) function. Before I submit the next version, please let me know if anyone has objections to this approach. Regards, Kuan-Wei
On Fri, Mar 07, 2025 at 04:14:34AM -0800, H. Peter Anvin wrote: > On March 7, 2025 4:13:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote: > > > >* Jiri Slaby <jirislaby@kernel.org> wrote: > > > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: > >> > > >> > * Jiri Slaby <jirislaby@kernel.org> wrote: > >> > > >> > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > >> > > > Change return type to bool for better clarity. Update the kernel doc > >> > > > comment accordingly, including fixing "@value" to "@val" and adjusting > >> > > > examples. Also mark the function with __attribute_const__ to allow > >> > > > potential compiler optimizations. > >> > > > > >> > > > 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> > >> > > > --- > >> > > > include/linux/bitops.h | 10 +++++----- > >> > > > 1 file changed, 5 insertions(+), 5 deletions(-) > >> > > > > >> > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > >> > > > index c1cb53cf2f0f..44e5765b8bec 100644 > >> > > > --- a/include/linux/bitops.h > >> > > > +++ b/include/linux/bitops.h > >> > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > >> > > > /** > >> > > > * parity8 - get the parity of an u8 value > >> > > > - * @value: the value to be examined > >> > > > + * @val: the value to be examined > >> > > > * > >> > > > * Determine the parity of the u8 argument. > >> > > > * > >> > > > * Returns: > >> > > > - * 0 for even parity, 1 for odd parity > >> > > > + * false for even parity, true for odd parity > >> > > > >> > > This occurs somehow inverted to me. When something is in parity means that > >> > > it has equal number of 1s and 0s. I.e. return true for even distribution. > >> > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when > >> > > bool is returned? Then you'd return true for odd. > >> > > >> > OTOH: > >> > > >> > - '0' is an even number and is returned for even parity, > >> > - '1' is an odd number and is returned for odd parity. > >> > >> Yes, that used to make sense for me. For bool/true/false, it no longer does. > >> But as I wrote, it might be only me... > > > >No strong opinion on this from me either, I'd guess existing practice > >with other parity functions should probably control. (If a coherent > >praxis exists.). > > > >Thanks, > > > > Ingo > > Instead of "bool" think of it as "bit" and it makes more sense So, to help people thinking that way we can introduce a corresponding type: typedef unsigned _BitInt(1) u1; It already works for clang, and GCC is going to adopt it with std=c23. We can make u1 an alias to bool for GCC for a while. If you guys like it, I can send a patch. For clang it prints quite a nice overflow warning: tst.c:59:9: warning: implicit conversion from 'int' to 'u1' (aka 'unsigned _BitInt(1)') changes value from 2 to 0 [-Wconstant-conversion] 59 | u1 r = 2; | ~ ^ Thanks, Yury
On March 7, 2025 11:30:08 AM PST, Yury Norov <yury.norov@gmail.com> wrote: >On Fri, Mar 07, 2025 at 04:14:34AM -0800, H. Peter Anvin wrote: >> On March 7, 2025 4:13:26 AM PST, Ingo Molnar <mingo@kernel.org> wrote: >> > >> >* Jiri Slaby <jirislaby@kernel.org> wrote: >> > >> >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >> >> > >> >> > * Jiri Slaby <jirislaby@kernel.org> wrote: >> >> > >> >> > > On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >> >> > > > Change return type to bool for better clarity. Update the kernel doc >> >> > > > comment accordingly, including fixing "@value" to "@val" and adjusting >> >> > > > examples. Also mark the function with __attribute_const__ to allow >> >> > > > potential compiler optimizations. >> >> > > > >> >> > > > 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> >> >> > > > --- >> >> > > > include/linux/bitops.h | 10 +++++----- >> >> > > > 1 file changed, 5 insertions(+), 5 deletions(-) >> >> > > > >> >> > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h >> >> > > > index c1cb53cf2f0f..44e5765b8bec 100644 >> >> > > > --- a/include/linux/bitops.h >> >> > > > +++ b/include/linux/bitops.h >> >> > > > @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >> >> > > > /** >> >> > > > * parity8 - get the parity of an u8 value >> >> > > > - * @value: the value to be examined >> >> > > > + * @val: the value to be examined >> >> > > > * >> >> > > > * Determine the parity of the u8 argument. >> >> > > > * >> >> > > > * Returns: >> >> > > > - * 0 for even parity, 1 for odd parity >> >> > > > + * false for even parity, true for odd parity >> >> > > >> >> > > This occurs somehow inverted to me. When something is in parity means that >> >> > > it has equal number of 1s and 0s. I.e. return true for even distribution. >> >> > > Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> >> > > bool is returned? Then you'd return true for odd. >> >> > >> >> > OTOH: >> >> > >> >> > - '0' is an even number and is returned for even parity, >> >> > - '1' is an odd number and is returned for odd parity. >> >> >> >> Yes, that used to make sense for me. For bool/true/false, it no longer does. >> >> But as I wrote, it might be only me... >> > >> >No strong opinion on this from me either, I'd guess existing practice >> >with other parity functions should probably control. (If a coherent >> >praxis exists.). >> > >> >Thanks, >> > >> > Ingo >> >> Instead of "bool" think of it as "bit" and it makes more sense > >So, to help people thinking that way we can introduce a corresponding >type: > typedef unsigned _BitInt(1) u1; > >It already works for clang, and GCC is going to adopt it with std=c23. >We can make u1 an alias to bool for GCC for a while. If you guys like >it, I can send a patch. > >For clang it prints quite a nice overflow warning: > >tst.c:59:9: warning: implicit conversion from 'int' to 'u1' (aka 'unsigned _BitInt(1)') changes value from 2 to 0 [-Wconstant-conversion] > 59 | u1 r = 2; > | ~ ^ > >Thanks, >Yury No, for a whole bunch of reasons.
On Fri, 7 Mar 2025 12:42:41 +0100 Jiri Slaby <jirislaby@kernel.org> wrote: > On 07. 03. 25, 12:38, Ingo Molnar wrote: > > > > * Jiri Slaby <jirislaby@kernel.org> wrote: > > > >> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: > >>> Change return type to bool for better clarity. Update the kernel doc > >>> comment accordingly, including fixing "@value" to "@val" and adjusting > >>> examples. Also mark the function with __attribute_const__ to allow > >>> potential compiler optimizations. > >>> > >>> 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> > >>> --- > >>> include/linux/bitops.h | 10 +++++----- > >>> 1 file changed, 5 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h > >>> index c1cb53cf2f0f..44e5765b8bec 100644 > >>> --- a/include/linux/bitops.h > >>> +++ b/include/linux/bitops.h > >>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) > >>> /** > >>> * parity8 - get the parity of an u8 value > >>> - * @value: the value to be examined > >>> + * @val: the value to be examined > >>> * > >>> * Determine the parity of the u8 argument. > >>> * > >>> * Returns: > >>> - * 0 for even parity, 1 for odd parity > >>> + * false for even parity, true for odd parity > >> > >> This occurs somehow inverted to me. When something is in parity means that > >> it has equal number of 1s and 0s. I.e. return true for even distribution. > >> Dunno what others think? Or perhaps this should be dubbed odd_parity() when > >> bool is returned? Then you'd return true for odd. > > > > OTOH: > > > > - '0' is an even number and is returned for even parity, > > - '1' is an odd number and is returned for odd parity. > > Yes, that used to make sense for me. For bool/true/false, it no longer > does. But as I wrote, it might be only me... No me as well, I've made the same comment before. When reading code I don't want to have to look up a function definition. There is even scope for having parity_odd() and parity_even(). And, with the version that shifts a constant right you want to invert the constant! David
On March 7, 2025 11:36:43 AM PST, David Laight <david.laight.linux@gmail.com> wrote: >On Fri, 7 Mar 2025 12:42:41 +0100 >Jiri Slaby <jirislaby@kernel.org> wrote: > >> On 07. 03. 25, 12:38, Ingo Molnar wrote: >> > >> > * Jiri Slaby <jirislaby@kernel.org> wrote: >> > >> >> On 06. 03. 25, 17:25, Kuan-Wei Chiu wrote: >> >>> Change return type to bool for better clarity. Update the kernel doc >> >>> comment accordingly, including fixing "@value" to "@val" and adjusting >> >>> examples. Also mark the function with __attribute_const__ to allow >> >>> potential compiler optimizations. >> >>> >> >>> 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> >> >>> --- >> >>> include/linux/bitops.h | 10 +++++----- >> >>> 1 file changed, 5 insertions(+), 5 deletions(-) >> >>> >> >>> diff --git a/include/linux/bitops.h b/include/linux/bitops.h >> >>> index c1cb53cf2f0f..44e5765b8bec 100644 >> >>> --- a/include/linux/bitops.h >> >>> +++ b/include/linux/bitops.h >> >>> @@ -231,26 +231,26 @@ static inline int get_count_order_long(unsigned long l) >> >>> /** >> >>> * parity8 - get the parity of an u8 value >> >>> - * @value: the value to be examined >> >>> + * @val: the value to be examined >> >>> * >> >>> * Determine the parity of the u8 argument. >> >>> * >> >>> * Returns: >> >>> - * 0 for even parity, 1 for odd parity >> >>> + * false for even parity, true for odd parity >> >> >> >> This occurs somehow inverted to me. When something is in parity means that >> >> it has equal number of 1s and 0s. I.e. return true for even distribution. >> >> Dunno what others think? Or perhaps this should be dubbed odd_parity() when >> >> bool is returned? Then you'd return true for odd. >> > >> > OTOH: >> > >> > - '0' is an even number and is returned for even parity, >> > - '1' is an odd number and is returned for odd parity. >> >> Yes, that used to make sense for me. For bool/true/false, it no longer >> does. But as I wrote, it might be only me... > >No me as well, I've made the same comment before. >When reading code I don't want to have to look up a function definition. >There is even scope for having parity_odd() and parity_even(). >And, with the version that shifts a constant right you want to invert >the constant! > > David > > > > Of course, for me, if I saw "parity_odd()" I would think of it as a function that caused the parity to become odd, i.e. if (!parity(x)) x ^= 1 << 7;
On March 7, 2025 11:53:10 AM PST, David Laight <david.laight.linux@gmail.com> wrote: >On Fri, 07 Mar 2025 11:30:35 -0800 >"H. Peter Anvin" <hpa@zytor.com> wrote: > >> On March 7, 2025 10:49:56 AM PST, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> (int)true most definitely is guaranteed to be 1. >> > >> >That's not technically correct any more. >> > >> >GCC has introduced hardened bools that intentionally have bit patterns >> >other than 0 and 1. >> > >> >https://gcc.gnu.org/gcc-14/changes.html >> > >> >~Andrew >> >> Bit patterns in memory maybe (not that I can see the Linux kernel using them) but >> for compiler-generated conversations that's still a given, or the manager isn't C >> or anything even remotely like it. >> > >The whole idea of 'bool' is pretty much broken by design. >The underlying problem is that values other than 'true' and 'false' can >always get into 'bool' variables. > >Once that has happened it is all fubar. > >Trying to sanitise a value with (say): >int f(bool v) >{ > return (int)v & 1; >} >just doesn't work (see https://www.godbolt.org/z/MEndP3q9j) > >I really don't see how using (say) 0xaa and 0x55 helps. >What happens if the value is wrong? a trap or exception?, good luck recovering >from that. > > David Did you just discover GIGO?
On Fri, Mar 07, 2025 at 12:07:02PM -0800, H. Peter Anvin wrote: > On March 7, 2025 11:53:10 AM PST, David Laight <david.laight.linux@gmail.com> wrote: > >On Fri, 07 Mar 2025 11:30:35 -0800 > >"H. Peter Anvin" <hpa@zytor.com> wrote: > > > >> On March 7, 2025 10:49:56 AM PST, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> >> (int)true most definitely is guaranteed to be 1. > >> > > >> >That's not technically correct any more. > >> > > >> >GCC has introduced hardened bools that intentionally have bit patterns > >> >other than 0 and 1. > >> > > >> >https://gcc.gnu.org/gcc-14/changes.html > >> > > >> >~Andrew > >> > >> Bit patterns in memory maybe (not that I can see the Linux kernel using them) but > >> for compiler-generated conversations that's still a given, or the manager isn't C > >> or anything even remotely like it. > >> > > > >The whole idea of 'bool' is pretty much broken by design. > >The underlying problem is that values other than 'true' and 'false' can > >always get into 'bool' variables. > > > >Once that has happened it is all fubar. > > > >Trying to sanitise a value with (say): > >int f(bool v) > >{ > > return (int)v & 1; > >} > >just doesn't work (see https://www.godbolt.org/z/MEndP3q9j) > > > >I really don't see how using (say) 0xaa and 0x55 helps. > >What happens if the value is wrong? a trap or exception?, good luck recovering > >from that. > > > > David > > Did you just discover GIGO? Thanks for all the suggestions. I don't have a strong opinion on the naming or return type. I'm still a bit confused about whether I can assume that casting bool to int always results in 0 or 1. If that's the case, since most people prefer bool over int as the return type and some are against introducing u1, my current plan is to use the following in the next version: bool parity_odd(u64 val); This keeps the bool return type, renames the function for better clarity, and avoids extra maintenance burden by having just one function. If I can't assume that casting bool to int always results in 0 or 1, would it be acceptable to keep the return type as int? Would this work for everyone? Regards, Kuan-Wei
On 09. 03. 25, 16:48, Kuan-Wei Chiu wrote:
> Would this work for everyone?
+1 for /me.