[PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Andy Shevchenko]

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(-)

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,
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Alexander Lobakin]

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

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Masami Hiramatsu]

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


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Alexander Lobakin]

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

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Yury Norov]

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

Re: [PATCH v1 1/1] bitops: Share BYTES_TO_BITS() for everyone

[Alexander Lobakin]

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