[PATCH v2] libbpf: Fix is_pow_of_2

[PATCH v2] libbpf: Fix is_pow_of_2

[Ian Rogers]

From: Yuze Chi <chiyuze@google.com>

Move the correct definition from linker.c into libbpf_internal.h.

Reported-by: Yuze Chi <chiyuze@google.com>
Signed-off-by: Yuze Chi <chiyuze@google.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/lib/bpf/libbpf.c          | 5 -----
 tools/lib/bpf/libbpf_internal.h | 5 +++++
 tools/lib/bpf/linker.c          | 5 -----
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 3f4f18684bd3..346f941bb995 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4954,11 +4954,6 @@ bpf_object__populate_internal_map(struct bpf_object *obj, struct bpf_map *map)
 
 static void bpf_map__destroy(struct bpf_map *map);
 
-static bool is_pow_of_2(size_t x)
-{
-	return x && (x & (x - 1));
-}
-
 static size_t adjust_ringbuf_sz(size_t sz)
 {
 	__u32 page_sz = sysconf(_SC_PAGE_SIZE);
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index 4abdbe2fea9d..ef5d975078e5 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -580,4 +580,9 @@ struct bpf_link * usdt_manager_attach_usdt(struct usdt_manager *man,
 					   const char *usdt_provider, const char *usdt_name,
 					   __u64 usdt_cookie);
 
+static inline bool is_pow_of_2(size_t x)
+{
+	return x && (x & (x - 1)) == 0;
+}
+
 #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
index 9aa016fb55aa..85c0fddf55d1 100644
--- a/tools/lib/bpf/linker.c
+++ b/tools/lib/bpf/linker.c
@@ -697,11 +697,6 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
 	return err;
 }
 
-static bool is_pow_of_2(size_t x)
-{
-	return x && (x & (x - 1)) == 0;
-}
-
 static int linker_sanity_check_elf(struct src_obj *obj)
 {
 	struct src_sec *sec;
-- 
2.36.1.255.ge46751e96f-goog

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Ian Rogers]

On Thu, Jun 2, 2022 at 10:52 PM Ian Rogers <irogers@google.com> wrote:
>
> From: Yuze Chi <chiyuze@google.com>
>
> Move the correct definition from linker.c into libbpf_internal.h.
>

Sorry I missed this:
Fixes: 0087a681fa8c ("libbpf: Automatically fix up
BPF_MAP_TYPE_RINGBUF size, if necessary")

Thanks,
Ian

> Reported-by: Yuze Chi <chiyuze@google.com>
> Signed-off-by: Yuze Chi <chiyuze@google.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/lib/bpf/libbpf.c          | 5 -----
>  tools/lib/bpf/libbpf_internal.h | 5 +++++
>  tools/lib/bpf/linker.c          | 5 -----
>  3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3f4f18684bd3..346f941bb995 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4954,11 +4954,6 @@ bpf_object__populate_internal_map(struct bpf_object *obj, struct bpf_map *map)
>
>  static void bpf_map__destroy(struct bpf_map *map);
>
> -static bool is_pow_of_2(size_t x)
> -{
> -       return x && (x & (x - 1));
> -}
> -
>  static size_t adjust_ringbuf_sz(size_t sz)
>  {
>         __u32 page_sz = sysconf(_SC_PAGE_SIZE);
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 4abdbe2fea9d..ef5d975078e5 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -580,4 +580,9 @@ struct bpf_link * usdt_manager_attach_usdt(struct usdt_manager *man,
>                                            const char *usdt_provider, const char *usdt_name,
>                                            __u64 usdt_cookie);
>
> +static inline bool is_pow_of_2(size_t x)
> +{
> +       return x && (x & (x - 1)) == 0;
> +}
> +
>  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> index 9aa016fb55aa..85c0fddf55d1 100644
> --- a/tools/lib/bpf/linker.c
> +++ b/tools/lib/bpf/linker.c
> @@ -697,11 +697,6 @@ static int linker_load_obj_file(struct bpf_linker *linker, const char *filename,
>         return err;
>  }
>
> -static bool is_pow_of_2(size_t x)
> -{
> -       return x && (x & (x - 1)) == 0;
> -}
> -
>  static int linker_sanity_check_elf(struct src_obj *obj)
>  {
>         struct src_sec *sec;
> --
> 2.36.1.255.ge46751e96f-goog
>

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Joe Perches]

On Thu, 2022-06-02 at 22:57 -0700, Ian Rogers wrote:
> On Thu, Jun 2, 2022 at 10:52 PM Ian Rogers <irogers@google.com> wrote:
> > From: Yuze Chi <chiyuze@google.com>
[]
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
[]
> > @@ -580,4 +580,9 @@ struct bpf_link * usdt_manager_attach_usdt(struct usdt_manager *man,
> >                                            const char *usdt_provider, const char *usdt_name,
> >                                            __u64 usdt_cookie);
> > 
> > +static inline bool is_pow_of_2(size_t x)
> > +{
> > +       return x && (x & (x - 1)) == 0;
> > +}
> > +
> >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */

If speed of execution is a potential issue, maybe:

#if __has_builtin(__builtin_popcount)
	return __builtin_popcount(x) == 1;
#else
	return x && (x & (x-1)) == 0;
#endif

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Andrii Nakryiko]

On Fri, Jun 3, 2022 at 9:58 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2022-06-02 at 22:57 -0700, Ian Rogers wrote:
> > On Thu, Jun 2, 2022 at 10:52 PM Ian Rogers <irogers@google.com> wrote:
> > > From: Yuze Chi <chiyuze@google.com>
> []
> > > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> []
> > > @@ -580,4 +580,9 @@ struct bpf_link * usdt_manager_attach_usdt(struct usdt_manager *man,
> > >                                            const char *usdt_provider, const char *usdt_name,
> > >                                            __u64 usdt_cookie);
> > >
> > > +static inline bool is_pow_of_2(size_t x)
> > > +{
> > > +       return x && (x & (x - 1)) == 0;
> > > +}
> > > +
> > >  #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
>
> If speed of execution is a potential issue, maybe:

It's not, as it's not in any sort of high-frequency hot path. But even
if it was, we should be careful with __builtin_popcount() because
depending on target CPU architecture __builtin_popcount() can be
turned into a helper function call instead of using hardware
instruction. But either way, keeping it simple is prefereable.

>
> #if __has_builtin(__builtin_popcount)
>         return __builtin_popcount(x) == 1;
> #else
>         return x && (x & (x-1)) == 0;
> #endif
>

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu,  2 Jun 2022 22:51:56 -0700 you wrote:
> From: Yuze Chi <chiyuze@google.com>
> 
> Move the correct definition from linker.c into libbpf_internal.h.
> 
> Reported-by: Yuze Chi <chiyuze@google.com>
> Signed-off-by: Yuze Chi <chiyuze@google.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> 
> [...]

Here is the summary with links:
  - [v2] libbpf: Fix is_pow_of_2
    https://git.kernel.org/bpf/bpf-next/c/f913ad6559e3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Ian Rogers]

On Fri, Jun 3, 2022 at 11:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to bpf/bpf-next.git (master)
> by Andrii Nakryiko <andrii@kernel.org>:
>
> On Thu,  2 Jun 2022 22:51:56 -0700 you wrote:
> > From: Yuze Chi <chiyuze@google.com>
> >
> > Move the correct definition from linker.c into libbpf_internal.h.
> >
> > Reported-by: Yuze Chi <chiyuze@google.com>
> > Signed-off-by: Yuze Chi <chiyuze@google.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > [...]
>
> Here is the summary with links:
>   - [v2] libbpf: Fix is_pow_of_2
>     https://git.kernel.org/bpf/bpf-next/c/f913ad6559e3
>
> You are awesome, thank you!

Will this patch get added to 5.19?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n4948

Thanks,
Ian

> --
> Deet-doot-dot, I am a bot.
> https://korg.docs.kernel.org/patchwork/pwbot.html
>
>

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Andrii Nakryiko]

On Tue, Jun 14, 2022 at 1:41 PM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Jun 3, 2022 at 11:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> >
> > Hello:
> >
> > This patch was applied to bpf/bpf-next.git (master)
> > by Andrii Nakryiko <andrii@kernel.org>:
> >
> > On Thu,  2 Jun 2022 22:51:56 -0700 you wrote:
> > > From: Yuze Chi <chiyuze@google.com>
> > >
> > > Move the correct definition from linker.c into libbpf_internal.h.
> > >
> > > Reported-by: Yuze Chi <chiyuze@google.com>
> > > Signed-off-by: Yuze Chi <chiyuze@google.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > >
> > > [...]
> >
> > Here is the summary with links:
> >   - [v2] libbpf: Fix is_pow_of_2
> >     https://git.kernel.org/bpf/bpf-next/c/f913ad6559e3
> >
> > You are awesome, thank you!
>
> Will this patch get added to 5.19?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n4948
>

I've applied it to bpf-next, so as it stands right now - no. Do you
need this for perf?

> Thanks,
> Ian
>
> > --
> > Deet-doot-dot, I am a bot.
> > https://korg.docs.kernel.org/patchwork/pwbot.html
> >
> >

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Ian Rogers]

On Thu, Jun 16, 2022 at 2:00 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Jun 14, 2022 at 1:41 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Fri, Jun 3, 2022 at 11:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > >
> > > Hello:
> > >
> > > This patch was applied to bpf/bpf-next.git (master)
> > > by Andrii Nakryiko <andrii@kernel.org>:
> > >
> > > On Thu,  2 Jun 2022 22:51:56 -0700 you wrote:
> > > > From: Yuze Chi <chiyuze@google.com>
> > > >
> > > > Move the correct definition from linker.c into libbpf_internal.h.
> > > >
> > > > Reported-by: Yuze Chi <chiyuze@google.com>
> > > > Signed-off-by: Yuze Chi <chiyuze@google.com>
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > >
> > > > [...]
> > >
> > > Here is the summary with links:
> > >   - [v2] libbpf: Fix is_pow_of_2
> > >     https://git.kernel.org/bpf/bpf-next/c/f913ad6559e3
> > >
> > > You are awesome, thank you!
> >
> > Will this patch get added to 5.19?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n4948
> >
>
> I've applied it to bpf-next, so as it stands right now - no. Do you
> need this for perf?

Nope. We carry it as a patch against 5.19 in Google and was surprised
to see I didn't need to drop the patch.  Our internal code had
encountered the bug, hence needing the fix. I'd expect others could
encounter it, but I'm unaware of an issue with it and perf.

Thanks,
Ian

> > Thanks,
> > Ian
> >
> > > --
> > > Deet-doot-dot, I am a bot.
> > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > >
> > >

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Andrii Nakryiko]

On Thu, Jun 16, 2022 at 2:07 PM Ian Rogers <irogers@google.com> wrote:
>
> On Thu, Jun 16, 2022 at 2:00 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Jun 14, 2022 at 1:41 PM Ian Rogers <irogers@google.com> wrote:
> > >
> > > On Fri, Jun 3, 2022 at 11:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > > >
> > > > Hello:
> > > >
> > > > This patch was applied to bpf/bpf-next.git (master)
> > > > by Andrii Nakryiko <andrii@kernel.org>:
> > > >
> > > > On Thu,  2 Jun 2022 22:51:56 -0700 you wrote:
> > > > > From: Yuze Chi <chiyuze@google.com>
> > > > >
> > > > > Move the correct definition from linker.c into libbpf_internal.h.
> > > > >
> > > > > Reported-by: Yuze Chi <chiyuze@google.com>
> > > > > Signed-off-by: Yuze Chi <chiyuze@google.com>
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > >
> > > > > [...]
> > > >
> > > > Here is the summary with links:
> > > >   - [v2] libbpf: Fix is_pow_of_2
> > > >     https://git.kernel.org/bpf/bpf-next/c/f913ad6559e3
> > > >
> > > > You are awesome, thank you!
> > >
> > > Will this patch get added to 5.19?
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n4948
> > >
> >
> > I've applied it to bpf-next, so as it stands right now - no. Do you
> > need this for perf?
>
> Nope. We carry it as a patch against 5.19 in Google and was surprised
> to see I didn't need to drop the patch.  Our internal code had
> encountered the bug, hence needing the fix. I'd expect others could
> encounter it, but I'm unaware of an issue with it and perf.
>

So the fix is in Github mirror ([0]) and it is expected that everyone
is using libbpf based on Github repo, so not sure why you'd care about
this fix in bpf tree. I somehow assumed that you need it due to perf,
but was a bit surprised that perf is affected because I don't think
it's using BPF ringbuf.

So I guess the question I have is why you don't use libbpf from [0]
and what can be done to switch to the official libbpf repo?

  [0] https://github.com/libbpf/libbpf

> Thanks,
> Ian
>
> > > Thanks,
> > > Ian
> > >
> > > > --
> > > > Deet-doot-dot, I am a bot.
> > > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > > >
> > > >

Re: [PATCH v2] libbpf: Fix is_pow_of_2

[Ian Rogers]

On Thu, Jun 16, 2022 at 4:55 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jun 16, 2022 at 2:07 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Jun 16, 2022 at 2:00 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Jun 14, 2022 at 1:41 PM Ian Rogers <irogers@google.com> wrote:
> > > >
> > > > On Fri, Jun 3, 2022 at 11:30 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
> > > > >
> > > > > Hello:
> > > > >
> > > > > This patch was applied to bpf/bpf-next.git (master)
> > > > > by Andrii Nakryiko <andrii@kernel.org>:
> > > > >
> > > > > On Thu,  2 Jun 2022 22:51:56 -0700 you wrote:
> > > > > > From: Yuze Chi <chiyuze@google.com>
> > > > > >
> > > > > > Move the correct definition from linker.c into libbpf_internal.h.
> > > > > >
> > > > > > Reported-by: Yuze Chi <chiyuze@google.com>
> > > > > > Signed-off-by: Yuze Chi <chiyuze@google.com>
> > > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > > >
> > > > > > [...]
> > > > >
> > > > > Here is the summary with links:
> > > > >   - [v2] libbpf: Fix is_pow_of_2
> > > > >     https://git.kernel.org/bpf/bpf-next/c/f913ad6559e3
> > > > >
> > > > > You are awesome, thank you!
> > > >
> > > > Will this patch get added to 5.19?
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n4948
> > > >
> > >
> > > I've applied it to bpf-next, so as it stands right now - no. Do you
> > > need this for perf?
> >
> > Nope. We carry it as a patch against 5.19 in Google and was surprised
> > to see I didn't need to drop the patch.  Our internal code had
> > encountered the bug, hence needing the fix. I'd expect others could
> > encounter it, but I'm unaware of an issue with it and perf.
> >
>
> So the fix is in Github mirror ([0]) and it is expected that everyone
> is using libbpf based on Github repo, so not sure why you'd care about
> this fix in bpf tree. I somehow assumed that you need it due to perf,
> but was a bit surprised that perf is affected because I don't think
> it's using BPF ringbuf.
>
> So I guess the question I have is why you don't use libbpf from [0]
> and what can be done to switch to the official libbpf repo?
>
>   [0] https://github.com/libbpf/libbpf

Agreed on not following Linus' tree for libbpf. Not sure about having
such an obvious bug in Linus' tree.

Thanks,
Ian

> > Thanks,
> > Ian
> >
> > > > Thanks,
> > > > Ian
> > > >
> > > > > --
> > > > > Deet-doot-dot, I am a bot.
> > > > > https://korg.docs.kernel.org/patchwork/pwbot.html
> > > > >
> > > > >