| Message ID | 20220603041701.2799595-1-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: Fix is_pow_of_2 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | pending | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | pending | Logs for Kernel LATEST on z15 with gcc |
| 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 |
On Thu, Jun 2, 2022 at 9:17 PM Ian Rogers <irogers@google.com> wrote: > > From: Yuze Chi <chiyuze@google.com> > > There is a missing not. Consider a power of 2 number like 4096: > > x && (x & (x - 1)) > 4096 && (4096 & (4096 - 1)) > 4096 && (4096 & 4095) > 4096 && 0 > 0 > > with the not this is: > x && !(x & (x - 1)) > 4096 && !(4096 & (4096 - 1)) > 4096 && !(4096 & 4095) > 4096 && !0 > 4096 && 1 > 1 > > 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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 3f4f18684bd3..fd0414ea00df 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); > > static bool is_pow_of_2(size_t x) > { > - return x && (x & (x - 1)); > + return x && !(x & (x - 1)); No idea if anyone cares about the consistency, but in linker.c (same directory) the same static function is defined using == 0 at the end instead of using the not operator. Aside from the consistency issue, personally I find the == 0 version a little bit easier to read and understand because it's a bit less dense (and a "!" next to a "(" is an easy character to overlook). > } > > static size_t adjust_ringbuf_sz(size_t sz) > -- > 2.36.1.255.ge46751e96f-goog >
On Thu, Jun 2, 2022 at 9:31 PM Zvi Effron <zeffron@riotgames.com> wrote: > > On Thu, Jun 2, 2022 at 9:17 PM Ian Rogers <irogers@google.com> wrote: > > > > From: Yuze Chi <chiyuze@google.com> > > > > There is a missing not. Consider a power of 2 number like 4096: > > > > x && (x & (x - 1)) > > 4096 && (4096 & (4096 - 1)) > > 4096 && (4096 & 4095) > > 4096 && 0 > > 0 > > > > with the not this is: > > x && !(x & (x - 1)) > > 4096 && !(4096 & (4096 - 1)) > > 4096 && !(4096 & 4095) > > 4096 && !0 > > 4096 && 1 > > 1 > > > > 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 | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 3f4f18684bd3..fd0414ea00df 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); > > > > static bool is_pow_of_2(size_t x) > > { > > - return x && (x & (x - 1)); > > + return x && !(x & (x - 1)); ugh... *facepalm* > > No idea if anyone cares about the consistency, but in linker.c (same directory) > the same static function is defined using == 0 at the end instead of using the > not operator. > > Aside from the consistency issue, personally I find the == 0 version a little > bit easier to read and understand because it's a bit less dense (and a "!" next > to a "(" is an easy character to overlook). > I agree, even more so, logical not used with arbitrary integer (not a pointer or bool) is a mental stumbling block for me, so much so that I avoid doing !strcmp(), for example. But in this case, I'm not sure why I copy/pasted is_pow_of_2() instead of moving the one from linker.c into libbpf_internal.h as static inline. Let's do that instead? > > } > > > > static size_t adjust_ringbuf_sz(size_t sz) > > -- > > 2.36.1.255.ge46751e96f-goog > >
On Thu, Jun 2, 2022 at 10:33 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Jun 2, 2022 at 9:31 PM Zvi Effron <zeffron@riotgames.com> wrote: > > > > On Thu, Jun 2, 2022 at 9:17 PM Ian Rogers <irogers@google.com> wrote: > > > > > > From: Yuze Chi <chiyuze@google.com> > > > > > > There is a missing not. Consider a power of 2 number like 4096: > > > > > > x && (x & (x - 1)) > > > 4096 && (4096 & (4096 - 1)) > > > 4096 && (4096 & 4095) > > > 4096 && 0 > > > 0 > > > > > > with the not this is: > > > x && !(x & (x - 1)) > > > 4096 && !(4096 & (4096 - 1)) > > > 4096 && !(4096 & 4095) > > > 4096 && !0 > > > 4096 && 1 > > > 1 > > > > > > Reported-by: Yuze Chi <chiyuze@google.com> > > > Signed-off-by: Yuze Chi <chiyuze@google.com> > > > Signed-off-by: Ian Rogers <irogers@google.com> also can you please add Fixes: tag? > > > --- > > > tools/lib/bpf/libbpf.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index 3f4f18684bd3..fd0414ea00df 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); > > > > > > static bool is_pow_of_2(size_t x) > > > { > > > - return x && (x & (x - 1)); > > > + return x && !(x & (x - 1)); > > ugh... *facepalm* > > > > > No idea if anyone cares about the consistency, but in linker.c (same directory) > > the same static function is defined using == 0 at the end instead of using the > > not operator. > > > > Aside from the consistency issue, personally I find the == 0 version a little > > bit easier to read and understand because it's a bit less dense (and a "!" next > > to a "(" is an easy character to overlook). > > > > I agree, even more so, logical not used with arbitrary integer (not a > pointer or bool) is a mental stumbling block for me, so much so that I > avoid doing !strcmp(), for example. > > But in this case, I'm not sure why I copy/pasted is_pow_of_2() instead > of moving the one from linker.c into libbpf_internal.h as static > inline. Let's do that instead? > > > > } > > > > > > static size_t adjust_ringbuf_sz(size_t sz) > > > -- > > > 2.36.1.255.ge46751e96f-goog > > >
Hi! > From: Yuze Chi <chiyuze@google.com> > > +++ b/tools/lib/bpf/libbpf.c > @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); > > static bool is_pow_of_2(size_t x) > { > - return x && (x & (x - 1)); > + return x && !(x & (x - 1)); > } I'm pretty sure we have this test in macro in includes somewhere... should we use that instead? Pavel
On Sun, Jun 19, 2022 at 12:13 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > From: Yuze Chi <chiyuze@google.com> > > > > +++ b/tools/lib/bpf/libbpf.c > > @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); > > > > static bool is_pow_of_2(size_t x) > > { > > - return x && (x & (x - 1)); > > + return x && !(x & (x - 1)); > > } > > I'm pretty sure we have this test in macro in includes somewhere... should we use > that instead? I went looking for a macro that provided this check and could not find one. I did find the inlined static function is_power_of_2 in log2.h, though, that we could use. > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 06/22/2022 07:03 AM, Zvi Effron wrote: > On Sun, Jun 19, 2022 at 12:13 PM Pavel Machek <pavel@ucw.cz> wrote: >> >> Hi! >> >>> From: Yuze Chi <chiyuze@google.com> >>> >>> +++ b/tools/lib/bpf/libbpf.c >>> @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); >>> >>> static bool is_pow_of_2(size_t x) >>> { >>> - return x && (x & (x - 1)); >>> + return x && !(x & (x - 1)); >>> } >> >> I'm pretty sure we have this test in macro in includes somewhere... should we use >> that instead? > > I went looking for a macro that provided this check and could not find one. I arch/microblaze/mm/pgtable.c #define is_power_of_2(x) ((x) != 0 && (((x) & ((x) - 1)) == 0)) > did find the inlined static function is_power_of_2 in log2.h, though, that we > could use. Here is a patch, but it seems that this is not worth the extra pain. https://lore.kernel.org/bpf/8e5291b7-bd89-6fea-bfb7-954cacdb8523@iogearbox.net/ Thanks, Tiezhu
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 3f4f18684bd3..fd0414ea00df 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4956,7 +4956,7 @@ static void bpf_map__destroy(struct bpf_map *map); static bool is_pow_of_2(size_t x) { - return x && (x & (x - 1)); + return x && !(x & (x - 1)); } static size_t adjust_ringbuf_sz(size_t sz)