bitops: Share BYTES_TO_BITS() for everyone

PATCH In review Andy Shevchenko 4 participants 8 messages Aug 24, 2023 – Sep 11, 2023

Subsystems (2)

Tracing Bitops API

lore

Share & tools

lore

Andy Shevchenko

author

2023-08-24 12:38 UTC #0 lore raw

PATCH 1/1 bitops: Share BYTES_TO_BITS() for everyone

It may be new callers for the same macro, share it.

Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/bitops.h     | 2 ++
 kernel/trace/trace_probe.c | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

include/linux/bitops.h +2 -0

index 2ba557e067fe..66dc091e0c28 100644
@@ -21,6 +21,8 @@
 #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))
 
+#define BYTES_TO_BITS(nb)	((BITS_PER_LONG * (nb)) / sizeof(long))
+
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
 extern unsigned int __sw_hweight32(unsigned int w);

kernel/trace/trace_probe.c +1 -2

index c68a72707852..da6297d24d61 100644
@@ -11,6 +11,7 @@
  */
 #define pr_fmt(fmt)	"trace_probe: " fmt
 
+#include <linux/bitops.h>
 #include <linux/bpf.h>
 
 #include "trace_probe.h"

@@ -830,8 +831,6 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 	return ret;
 }
 
-#define BYTES_TO_BITS(nb)	((BITS_PER_LONG * (nb)) / sizeof(long))
-
 /* Bitfield type needs to be parsed into a fetch function */
 static int __parse_bitfield_probe_arg(const char *bf,
 				      const struct fetch_type *t,

Signature · 1 line

2.40.0.1.gaa8946217a0b

Alexander Lobakin

2023-08-25 14:51 UTC #1 lore raw

From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300

Quoted 6 lines

It may be new callers for the same macro, share it.
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.

Maybe symmetrical change in tools/ like I did[0] an aeon ago?

Quoted 33 lines

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/bitops.h     | 2 ++
 kernel/trace/trace_probe.c | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

include/linux/bitops.h +2 -0

index 2ba557e067fe..66dc091e0c28 100644
@@ -21,6 +21,8 @@
 #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))
 
+#define BYTES_TO_BITS(nb)	((BITS_PER_LONG * (nb)) / sizeof(long))
+
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
 extern unsigned int __sw_hweight32(unsigned int w);

kernel/trace/trace_probe.c +1 -0

index c68a72707852..da6297d24d61 100644
@@ -11,6 +11,7 @@
  */
 #define pr_fmt(fmt)	"trace_probe: " fmt
 
+#include <linux/bitops.h>
 #include <linux/bpf.h>
 
 #include "trace_probe.h"

kernel/trace/trace_probe.c parse_probe_arg(char *arg, const struct fetch_type *type, · line 836

Quoted 8 lines

kernel/trace/trace_probe.c +0 -2

@@ -830,8 +831,6 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 	return ret;
 }
 
-#define BYTES_TO_BITS(nb)	((BITS_PER_LONG * (nb)) / sizeof(long))
-
 /* Bitfield type needs to be parsed into a fetch function */
 static int __parse_bitfield_probe_arg(const char *bf,
 				      const struct fetch_type *t,

[0]
https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8

Thanks,
Olek

Masami Hiramatsu (Google)

maintainer

2023-08-30 18:36 UTC #2 lore raw

Quoted 11 lines · On Fri, 25 Aug 2023 16:49:07 +0200 Alexander Lobakin…

From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300
Quoted 6 lines
It may be new callers for the same macro, share it.
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.
Maybe symmetrical change in tools/ like I did[0] an aeon ago?

Hm, it looks the same. The reason why I used the macro was I didn't know the BITS_PER_TYPE() at that point. Now it is OK to replace it with 'bytes * BITS_PER_TYPE(char)'.

Thank you,

Quoted 48 lines

Quoted 34 lines

Signed-off-by Andy Shevchenko <[email protected]>

---
 include/linux/bitops.h     | 2 ++
 kernel/trace/trace_probe.c | 3 +--
 2 files changed, 3 insertions(+), 2 deletions(-)

include/linux/bitops.h +2 -0

index 2ba557e067fe..66dc091e0c28 100644
@@ -21,6 +21,8 @@
 #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))
 
+#define BYTES_TO_BITS(nb)	((BITS_PER_LONG * (nb)) / sizeof(long))
+
 extern unsigned int __sw_hweight8(unsigned int w);
 extern unsigned int __sw_hweight16(unsigned int w);
 extern unsigned int __sw_hweight32(unsigned int w);

kernel/trace/trace_probe.c +1 -0

index c68a72707852..da6297d24d61 100644
@@ -11,6 +11,7 @@
  */
 #define pr_fmt(fmt)	"trace_probe: " fmt
 
+#include <linux/bitops.h>
 #include <linux/bpf.h>
 
 #include "trace_probe.h"

kernel/trace/trace_probe.c parse_probe_arg(char *arg, const struct fetch_type *type, · line 836

Quoted 8 lines

kernel/trace/trace_probe.c +0 -2

@@ -830,8 +831,6 @@ parse_probe_arg(char *arg, const struct fetch_type *type,
 	return ret;
 }
 
-#define BYTES_TO_BITS(nb)	((BITS_PER_LONG * (nb)) / sizeof(long))
-
 /* Bitfield type needs to be parsed into a fetch function */
 static int __parse_bitfield_probe_arg(const char *bf,
 				      const struct fetch_type *t,

[0]
https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8

Thanks,
Olek

Signature · 1 line

Masami Hiramatsu (Google) <[email protected]>

Andy Shevchenko

author

2023-08-31 13:21 UTC #3 lore raw

In reply to Alexander Lobakin

Quoted 11 lines · Alexander Lobakin

From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300
Quoted 6 lines
It may be new callers for the same macro, share it.
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.
Maybe symmetrical change in tools/ like I did[0] an aeon ago?

Hmm... Why can't you simply upstream your version? It seems better than mine.

Quoted 2 lines

[0]
https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Alexander Lobakin

2023-09-06 14:42 UTC #4 lore raw

From: Andy Shevchenko <[email protected]>
Date: Thu, 31 Aug 2023 16:21:30 +0300

Quoted 6 lines · Alexander Lobakin

Quoted 5 lines
From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300
Quoted 2 lines
It may be new callers for the same macro, share it.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.
Maybe symmetrical change in tools/ like I did[0] an aeon ago?
Hmm... Why can't you simply upstream your version? It seems better than mine.

It was a part of the Netlink bigint API which is a bit on hold for now (I needed this macro available treewide).
But I can send it as standalone if you're fine with that.

Quoted 4 lines

Quoted 3 lines
[0]
https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8

Thanks,
Olek

Andy Shevchenko

author

2023-09-06 14:54 UTC #5 lore raw

Quoted 13 lines · Alexander Lobakin

From: Andy Shevchenko <[email protected]>
Date: Thu, 31 Aug 2023 16:21:30 +0300
Quoted 11 lines · Alexander Lobakin
Quoted 10 lines
From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300
Quoted 6 lines
It may be new callers for the same macro, share it.
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Maybe symmetrical change in tools/ like I did[0] an aeon ago?
Hmm... Why can't you simply upstream your version? It seems better than mine.
It was a part of the Netlink bigint API which is a bit on hold for now (I needed this macro available treewide).
But I can send it as standalone if you're fine with that.

I'm fine. Yury?

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
[0]
https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Yury Norov

maintainer

2023-09-10 14:07 UTC #6 lore raw

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Alexander Lobakin
From: Andy Shevchenko <[email protected]>
Date: Thu, 31 Aug 2023 16:21:30 +0300
Quoted 13 lines · Alexander Lobakin
Quoted 11 lines
From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300
Quoted 6 lines
It may be new callers for the same macro, share it.
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.
Maybe symmetrical change in tools/ like I did[0] an aeon ago?

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Hmm... Why can't you simply upstream your version? It seems better than mine.
It was a part of the Netlink bigint API which is a bit on hold for now (I needed this macro available treewide).
But I can send it as standalone if you're fine with that.
I'm fine. Yury?

Do we have opencoded BYTES_TO_BITS() somewhere else? If so, it should be fixed in the same series.

Regarding implementation, the current:

        #define BYTES_TO_BITS(nb)      ((BITS_PER_LONG * (nb)) / sizeof(long))

looks weird. Maybe there are some special considerations in a tracing subsystem to make it like this, but as per Masami's email - there's not.

For a general purpose I'd suggest a simpler:

        #define BYTES_TO_BITS(nb)      ((nb) * BITS_PER_BYTE)

Thanks,
Yury

Alexander Lobakin

2023-09-11 21:58 UTC #7 lore raw

From: Yury Norov <[email protected]>
Date: Sun, 10 Sep 2023 07:07:16 -0700

Quoted 19 lines · Andy Shevchenko

Quoted 18 lines · Alexander Lobakin
Quoted 17 lines
From: Andy Shevchenko <[email protected]>
Date: Thu, 31 Aug 2023 16:21:30 +0300
Quoted 14 lines · Alexander Lobakin
Quoted 11 lines
From: Andy Shevchenko <[email protected]>
Date: Thu, 24 Aug 2023 15:37:28 +0300
Quoted 6 lines
It may be new callers for the same macro, share it.
Note, it's unknown why it's represented in the current form instead of simple multiplication and commit 1ff511e35ed8 ("tracing/kprobes: Add bitfield type") doesn't explain that neither. Let leave it as is and we may improve it in the future.
Maybe symmetrical change in tools/ like I did[0] an aeon ago?
Hmm... Why can't you simply upstream your version? It seems better than mine.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
It was a part of the Netlink bigint API which is a bit on hold for now (I needed this macro available treewide).
But I can send it as standalone if you're fine with that.
I'm fine. Yury?
Do we have opencoded BYTES_TO_BITS() somewhere else? If so, it should be fixed in the same series.

Treewide -- a ton.
We could add it so that devs could start using it and stop open-coding :D

Quoted 11 lines

Regarding implementation, the current:
        #define BYTES_TO_BITS(nb)      ((BITS_PER_LONG * (nb)) / sizeof(long))
looks weird. Maybe there are some special considerations in a tracing subsystem to make it like this, but as per Masami's email - there's not.
For a general purpose I'd suggest a simpler:
        #define BYTES_TO_BITS(nb)      ((nb) * BITS_PER_BYTE)

I also didn't notice anything that would require using logic more complex than this one. It would probably make more sense to define it that way when moving.

Quoted 3 lines

Thanks,
Yury

Thanks,
Olek