Message ID | 20200608161150.GA3073@kernel.org |
---|---|
State | Awaiting Upstream |
Delegated to: | BPF Maintainers |
Headers | show |
Series | libbpf's hashmap use of __WORDSIZE | expand |
On Mon, Jun 8, 2020 at 9:11 AM Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com> wrote: > > Hi Andrii, > > We've got that hashmap.[ch] copy from libbpf so that we can > build perf in systems where libbpf isn't available, and to make it build > in all the containers I regularly test build perf I had to add the patch > below, I test build with many versions of both gcc and clang and > multiple libcs. > > https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html > > The way that tools/include/linux/bitops.h has been doing since 2012 is > explained in: > > http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 > > Please take a look and see if you find it acceptable, > > Thanks, > > - Arnaldo > > Warning: Kernel ABI header at 'tools/perf/util/hashmap.h' differs from latest version at 'tools/lib/bpf/hashmap.h' > diff -u tools/perf/util/hashmap.h tools/lib/bpf/hashmap.h > > $ diff -u tools/lib/bpf/hashmap.h tools/perf/util/hashmap.h > --- tools/lib/bpf/hashmap.h 2020-06-05 13:25:27.822079838 -0300 > +++ tools/perf/util/hashmap.h 2020-06-05 13:25:27.838079794 -0300 > @@ -10,10 +10,9 @@ > > #include <stdbool.h> > #include <stddef.h> > -#ifdef __GLIBC__ > -#include <bits/wordsize.h> > -#else > -#include <bits/reg.h> > +#include <limits.h> > +#ifndef __WORDSIZE > +#define __WORDSIZE (__SIZEOF_LONG__ * 8) > #endif This looks fine, I also build-tested it in Travis CI, so all good. There is actually __SIZEOF_SIZE_T__, which is more directly what hash_bits work with, but I don't think it matters for any reasonable system in use :) So yeah, Acked-by: Andrii Nakryiko <andriin@fb.com> Are you going to do this change for libbpf's variant, or should I submit a separate patch? > > static inline size_t hash_bits(size_t h, int bits)
Mi On June 8, 2020 2:32:16 PM GMT-03:00, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >On Mon, Jun 8, 2020 at 9:11 AM Arnaldo Carvalho de Melo ><arnaldo.melo@gmail.com> wrote: >> >> Hi Andrii, >> >> We've got that hashmap.[ch] copy from libbpf so that we can >> build perf in systems where libbpf isn't available, and to make it >build >> in all the containers I regularly test build perf I had to add the >patch >> below, I test build with many versions of both gcc and clang and >> multiple libcs. >> >> https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html >> >> The way that tools/include/linux/bitops.h has been doing since 2012 >is >> explained in: >> >> http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 >> >> Please take a look and see if you find it acceptable, >> >> Thanks, >> >> - Arnaldo >> >> Warning: Kernel ABI header at 'tools/perf/util/hashmap.h' differs >from latest version at 'tools/lib/bpf/hashmap.h' >> diff -u tools/perf/util/hashmap.h tools/lib/bpf/hashmap.h >> >> $ diff -u tools/lib/bpf/hashmap.h tools/perf/util/hashmap.h >> --- tools/lib/bpf/hashmap.h 2020-06-05 13:25:27.822079838 -0300 >> +++ tools/perf/util/hashmap.h 2020-06-05 13:25:27.838079794 -0300 >> @@ -10,10 +10,9 @@ >> >> #include <stdbool.h> >> #include <stddef.h> >> -#ifdef __GLIBC__ >> -#include <bits/wordsize.h> >> -#else >> -#include <bits/reg.h> >> +#include <limits.h> >> +#ifndef __WORDSIZE >> +#define __WORDSIZE (__SIZEOF_LONG__ * 8) >> #endif > >This looks fine, I also build-tested it in Travis CI, so all good. >There is actually __SIZEOF_SIZE_T__, which is more directly what >hash_bits work with, but I don't think it matters for any reasonable >system in use :) > >So yeah, > >Acked-by: Andrii Nakryiko <andriin@fb.com> > >Are you going to do this change for libbpf's variant, or should I >submit a separate patch? I'll send the patch later, Thanks for checking, - Arnaldo > >> >> static inline size_t hash_bits(size_t h, int bits)
Some systems, such as Android, don't have a define for __WORDSIZE, do it in terms of __SIZEOF_LONG__, as done in perf since 2012: http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 For reference: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html I build tested it here and Andrii did some Travis CI build tests too. Acked-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Hey Arnaldo, On 6/9/20 5:34 PM, Arnaldo Carvalho de Melo wrote: > Some systems, such as Android, don't have a define for __WORDSIZE, do it > in terms of __SIZEOF_LONG__, as done in perf since 2012: > > http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 > > For reference: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html > > I build tested it here and Andrii did some Travis CI build tests too. > > Acked-by: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> Diff missing? Thanks, Daniel
Em Tue, Jun 09, 2020 at 10:37:48PM +0200, Daniel Borkmann escreveu: > Hey Arnaldo, > > On 6/9/20 5:34 PM, Arnaldo Carvalho de Melo wrote: > > Some systems, such as Android, don't have a define for __WORDSIZE, do it > > in terms of __SIZEOF_LONG__, as done in perf since 2012: > > > > http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 > > > > For reference: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html > > > > I build tested it here and Andrii did some Travis CI build tests too. > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > > Diff missing? Oh well, sorry about that, EBADCOFFEE or something: From: Arnaldo Carvalho de Melo <acme@kernel.org> Some systems, such as Android, don't have a define for __WORDSIZE, do it in terms of __SIZEOF_LONG__, as done in perf since 2012: http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 For reference: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html I build tested it here and Andrii did some Travis CI build tests too. Acked-by: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h index e823b35e7371..df59fd4fc95b 100644 --- a/tools/lib/bpf/hashmap.h +++ b/tools/lib/bpf/hashmap.h @@ -10,10 +10,9 @@ #include <stdbool.h> #include <stddef.h> -#ifdef __GLIBC__ -#include <bits/wordsize.h> -#else -#include <bits/reg.h> +#include <limits.h> +#ifndef __WORDSIZE +#define __WORDSIZE (__SIZEOF_LONG__ * 8) #endif static inline size_t hash_bits(size_t h, int bits)
On 6/9/20 11:16 PM, Arnaldo Carvalho de Melo wrote: > Em Tue, Jun 09, 2020 at 10:37:48PM +0200, Daniel Borkmann escreveu: >> Hey Arnaldo, >> >> On 6/9/20 5:34 PM, Arnaldo Carvalho de Melo wrote: >>> Some systems, such as Android, don't have a define for __WORDSIZE, do it >>> in terms of __SIZEOF_LONG__, as done in perf since 2012: >>> >>> http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 >>> >>> For reference: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html >>> >>> I build tested it here and Andrii did some Travis CI build tests too. >>> >>> Acked-by: Andrii Nakryiko <andriin@fb.com> >>> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> >> >> Diff missing? > > Oh well, sorry about that, EBADCOFFEE or something: > > From: Arnaldo Carvalho de Melo <acme@kernel.org> > > Some systems, such as Android, don't have a define for __WORDSIZE, do it > in terms of __SIZEOF_LONG__, as done in perf since 2012: > > http://git.kernel.org/torvalds/c/3f34f6c0233ae055b5 > > For reference: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html > > I build tested it here and Andrii did some Travis CI build tests too. > > Acked-by: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > [...] Applied, thanks!
--- tools/lib/bpf/hashmap.h 2020-06-05 13:25:27.822079838 -0300 +++ tools/perf/util/hashmap.h 2020-06-05 13:25:27.838079794 -0300 @@ -10,10 +10,9 @@ #include <stdbool.h> #include <stddef.h> -#ifdef __GLIBC__ -#include <bits/wordsize.h> -#else -#include <bits/reg.h> +#include <limits.h> +#ifndef __WORDSIZE +#define __WORDSIZE (__SIZEOF_LONG__ * 8) #endif static inline size_t hash_bits(size_t h, int bits)