| Message ID | 20230824123728.2761663-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v1,1/1] bitops: Share BYTES_TO_BITS() for everyone | expand |
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Thu, 24 Aug 2023 15:37:28 +0300 > 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? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/bitops.h | 2 ++ > kernel/trace/trace_probe.c | 3 +-- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index 2ba557e067fe..66dc091e0c28 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -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); > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > index c68a72707852..da6297d24d61 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -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, [0] https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8 Thanks, Olek
On Fri, 25 Aug 2023 16:49:07 +0200 Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Date: Thu, 24 Aug 2023 15:37:28 +0300 > > > 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, > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > include/linux/bitops.h | 2 ++ > > kernel/trace/trace_probe.c | 3 +-- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > > index 2ba557e067fe..66dc091e0c28 100644 > > --- a/include/linux/bitops.h > > +++ b/include/linux/bitops.h > > @@ -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); > > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > > index c68a72707852..da6297d24d61 100644 > > --- a/kernel/trace/trace_probe.c > > +++ b/kernel/trace/trace_probe.c > > @@ -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, > > [0] > https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8 > > Thanks, > Olek
On Fri, Aug 25, 2023 at 04:49:07PM +0200, Alexander Lobakin wrote: > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Date: Thu, 24 Aug 2023 15:37:28 +0300 > > > 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. > [0] > https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Thu, 31 Aug 2023 16:21:30 +0300 > On Fri, Aug 25, 2023 at 04:49:07PM +0200, Alexander Lobakin wrote: >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Date: Thu, 24 Aug 2023 15:37:28 +0300 >> >>> 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. 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. > >> [0] >> https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8 > Thanks, Olek
On Wed, Sep 06, 2023 at 04:40:39PM +0200, Alexander Lobakin wrote: > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Date: Thu, 31 Aug 2023 16:21:30 +0300 > > On Fri, Aug 25, 2023 at 04:49:07PM +0200, Alexander Lobakin wrote: > >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> Date: Thu, 24 Aug 2023 15:37:28 +0300 > >> > >>> 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. > > 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? > >> [0] > >> https://github.com/alobakin/linux/commit/fd308001fe6d38837fe820427209a6a99e4850a8
On Wed, Sep 06, 2023 at 05:54:26PM +0300, Andy Shevchenko wrote: > On Wed, Sep 06, 2023 at 04:40:39PM +0200, Alexander Lobakin wrote: > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Date: Thu, 31 Aug 2023 16:21:30 +0300 > > > On Fri, Aug 25, 2023 at 04:49:07PM +0200, Alexander Lobakin wrote: > > >> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > >> Date: Thu, 24 Aug 2023 15:37:28 +0300 > > >> > > >>> 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. > > > > 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
From: Yury Norov <yury.norov@gmail.com> Date: Sun, 10 Sep 2023 07:07:16 -0700 > On Wed, Sep 06, 2023 at 05:54:26PM +0300, Andy Shevchenko wrote: >> On Wed, Sep 06, 2023 at 04:40:39PM +0200, Alexander Lobakin wrote: >>> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> Date: Thu, 31 Aug 2023 16:21:30 +0300 >>>> On Fri, Aug 25, 2023 at 04:49:07PM +0200, Alexander Lobakin wrote: >>>>> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>>> Date: Thu, 24 Aug 2023 15:37:28 +0300 >>>>> >>>>>> 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. >>> >>> 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 > > 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. > > Thanks, > Yury Thanks, Olek
diff --git a/include/linux/bitops.h b/include/linux/bitops.h index 2ba557e067fe..66dc091e0c28 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -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); diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c index c68a72707852..da6297d24d61 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -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,
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 <andriy.shevchenko@linux.intel.com> --- include/linux/bitops.h | 2 ++ kernel/trace/trace_probe.c | 3 +-- 2 files changed, 3 insertions(+), 2 deletions(-)