| Message ID | 20220603055156.2830463-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f913ad6559e337b464cbd935f0ec1bacbdd95405 |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] libbpf: Fix is_pow_of_2 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
| bpf/vmtest-bpf-PR | pending | PR summary |
| bpf/vmtest-bpf-VM_Test-1 | pending | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-VM_Test-2 | pending | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-VM_Test-3 | pending | Logs for Kernel LATEST on z15 with gcc |
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 >
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
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 >
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!
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 > >
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 > > > >
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 > > > > > >
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 > > > > > > > >
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 > > > > > > > > > >
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;