* [PATCH 0/2] lib/bpf hashmap portability and fix @ 2020-05-06 20:52 Ian Rogers 2020-05-06 20:52 ` [PATCH 1/2] lib/bpf hashmap: increase portability Ian Rogers 2020-05-06 20:52 ` [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear Ian Rogers 0 siblings, 2 replies; 9+ messages in thread From: Ian Rogers @ 2020-05-06 20:52 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel Cc: Ian Rogers I'm experimenting with hashmap in perf for perf events within a metric expression. In doing this I encountered some issues with lib/bpf hashmap. Ignoring the perf change, I think these fixes are likely still worthwhile. Thanks! Ian Rogers (2): lib/bpf hashmap: increase portability lib/bpf hashmap: fixes to hashmap__clear tools/lib/bpf/hashmap.c | 6 ++++++ tools/lib/bpf/hashmap.h | 3 +-- 2 files changed, 7 insertions(+), 2 deletions(-) -- 2.26.2.526.g744177e7f7-goog ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] lib/bpf hashmap: increase portability 2020-05-06 20:52 [PATCH 0/2] lib/bpf hashmap portability and fix Ian Rogers @ 2020-05-06 20:52 ` Ian Rogers 2020-05-06 21:33 ` Andrii Nakryiko 2020-05-06 20:52 ` [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear Ian Rogers 1 sibling, 1 reply; 9+ messages in thread From: Ian Rogers @ 2020-05-06 20:52 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel Cc: Ian Rogers Don't include libbpf_internal.h as it is unused and has conflicting definitions, for example, with tools/perf/util/debug.h. Fix a non-glibc include path. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/lib/bpf/hashmap.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h index bae8879cdf58..d5ef212a55ba 100644 --- a/tools/lib/bpf/hashmap.h +++ b/tools/lib/bpf/hashmap.h @@ -13,9 +13,8 @@ #ifdef __GLIBC__ #include <bits/wordsize.h> #else -#include <bits/reg.h> +#include <linux/bitops.h> #endif -#include "libbpf_internal.h" static inline size_t hash_bits(size_t h, int bits) { -- 2.26.2.526.g744177e7f7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] lib/bpf hashmap: increase portability 2020-05-06 20:52 ` [PATCH 1/2] lib/bpf hashmap: increase portability Ian Rogers @ 2020-05-06 21:33 ` Andrii Nakryiko 2020-05-06 21:47 ` Ian Rogers 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2020-05-06 21:33 UTC (permalink / raw) To: Ian Rogers Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Networking, bpf, open list On Wed, May 6, 2020 at 1:54 PM Ian Rogers <irogers@google.com> wrote: > > Don't include libbpf_internal.h as it is unused and has conflicting > definitions, for example, with tools/perf/util/debug.h. > Fix a non-glibc include path. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/lib/bpf/hashmap.h | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > index bae8879cdf58..d5ef212a55ba 100644 > --- a/tools/lib/bpf/hashmap.h > +++ b/tools/lib/bpf/hashmap.h > @@ -13,9 +13,8 @@ > #ifdef __GLIBC__ > #include <bits/wordsize.h> > #else > -#include <bits/reg.h> > +#include <linux/bitops.h> why this change? It might be ok for libbpf built from kernel source, but it will break Github libbpf. > #endif > -#include "libbpf_internal.h" Dropping this seems ok, don't remember why I had it here in the first place. > > static inline size_t hash_bits(size_t h, int bits) > { > -- > 2.26.2.526.g744177e7f7-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] lib/bpf hashmap: increase portability 2020-05-06 21:33 ` Andrii Nakryiko @ 2020-05-06 21:47 ` Ian Rogers 2020-05-06 21:56 ` Andrii Nakryiko 0 siblings, 1 reply; 9+ messages in thread From: Ian Rogers @ 2020-05-06 21:47 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Networking, bpf, open list On Wed, May 6, 2020 at 2:33 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, May 6, 2020 at 1:54 PM Ian Rogers <irogers@google.com> wrote: > > > > Don't include libbpf_internal.h as it is unused and has conflicting > > definitions, for example, with tools/perf/util/debug.h. > > Fix a non-glibc include path. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/lib/bpf/hashmap.h | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > > index bae8879cdf58..d5ef212a55ba 100644 > > --- a/tools/lib/bpf/hashmap.h > > +++ b/tools/lib/bpf/hashmap.h > > @@ -13,9 +13,8 @@ > > #ifdef __GLIBC__ > > #include <bits/wordsize.h> > > #else > > -#include <bits/reg.h> > > +#include <linux/bitops.h> > > why this change? It might be ok for libbpf built from kernel source, > but it will break Github libbpf. Without this change my debian based machine wasn't able to build within the kernel tree. I see bits/wordsize.h on the machine. Perhaps the __WORDSIZE computation could just be based on __LP64__ to remove any #include? Thanks, Ian > > #endif > > -#include "libbpf_internal.h" > > Dropping this seems ok, don't remember why I had it here in the first place. > > > > > static inline size_t hash_bits(size_t h, int bits) > > { > > -- > > 2.26.2.526.g744177e7f7-goog > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] lib/bpf hashmap: increase portability 2020-05-06 21:47 ` Ian Rogers @ 2020-05-06 21:56 ` Andrii Nakryiko 2020-05-06 22:13 ` Ian Rogers 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2020-05-06 21:56 UTC (permalink / raw) To: Ian Rogers Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Networking, bpf, open list On Wed, May 6, 2020 at 2:47 PM Ian Rogers <irogers@google.com> wrote: > > On Wed, May 6, 2020 at 2:33 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, May 6, 2020 at 1:54 PM Ian Rogers <irogers@google.com> wrote: > > > > > > Don't include libbpf_internal.h as it is unused and has conflicting > > > definitions, for example, with tools/perf/util/debug.h. > > > Fix a non-glibc include path. > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > --- > > > tools/lib/bpf/hashmap.h | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > > > index bae8879cdf58..d5ef212a55ba 100644 > > > --- a/tools/lib/bpf/hashmap.h > > > +++ b/tools/lib/bpf/hashmap.h > > > @@ -13,9 +13,8 @@ > > > #ifdef __GLIBC__ > > > #include <bits/wordsize.h> > > > #else > > > -#include <bits/reg.h> > > > +#include <linux/bitops.h> > > > > why this change? It might be ok for libbpf built from kernel source, > > but it will break Github libbpf. > > Without this change my debian based machine wasn't able to build > within the kernel tree. I see bits/wordsize.h on the machine. Perhaps > the __WORDSIZE computation could just be based on __LP64__ to remove > any #include? It might work. Do you mind forking https://github.com/libbpf/libbpf and trying to execute travis CI tests with such change? It compiles across a range of distros and arches. You might need to set up Travis CI login, hope that's not a problem. Thanks! > > Thanks, > Ian > > > > #endif > > > -#include "libbpf_internal.h" > > > > Dropping this seems ok, don't remember why I had it here in the first place. > > > > > > > > static inline size_t hash_bits(size_t h, int bits) > > > { > > > -- > > > 2.26.2.526.g744177e7f7-goog > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] lib/bpf hashmap: increase portability 2020-05-06 21:56 ` Andrii Nakryiko @ 2020-05-06 22:13 ` Ian Rogers 0 siblings, 0 replies; 9+ messages in thread From: Ian Rogers @ 2020-05-06 22:13 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Networking, bpf, open list On Wed, May 6, 2020 at 2:56 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, May 6, 2020 at 2:47 PM Ian Rogers <irogers@google.com> wrote: > > > > On Wed, May 6, 2020 at 2:33 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Wed, May 6, 2020 at 1:54 PM Ian Rogers <irogers@google.com> wrote: > > > > > > > > Don't include libbpf_internal.h as it is unused and has conflicting > > > > definitions, for example, with tools/perf/util/debug.h. > > > > Fix a non-glibc include path. > > > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > --- > > > > tools/lib/bpf/hashmap.h | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/tools/lib/bpf/hashmap.h b/tools/lib/bpf/hashmap.h > > > > index bae8879cdf58..d5ef212a55ba 100644 > > > > --- a/tools/lib/bpf/hashmap.h > > > > +++ b/tools/lib/bpf/hashmap.h > > > > @@ -13,9 +13,8 @@ > > > > #ifdef __GLIBC__ > > > > #include <bits/wordsize.h> > > > > #else > > > > -#include <bits/reg.h> > > > > +#include <linux/bitops.h> > > > > > > why this change? It might be ok for libbpf built from kernel source, > > > but it will break Github libbpf. > > > > Without this change my debian based machine wasn't able to build > > within the kernel tree. I see bits/wordsize.h on the machine. Perhaps > > the __WORDSIZE computation could just be based on __LP64__ to remove > > any #include? > > It might work. Do you mind forking https://github.com/libbpf/libbpf > and trying to execute travis CI tests with such change? It compiles > across a range of distros and arches. You might need to set up Travis > CI login, hope that's not a problem. Thanks! I'll try to find time. Thanks, Ian > > > > Thanks, > > Ian > > > > > > #endif > > > > -#include "libbpf_internal.h" > > > > > > Dropping this seems ok, don't remember why I had it here in the first place. > > > > > > > > > > > static inline size_t hash_bits(size_t h, int bits) > > > > { > > > > -- > > > > 2.26.2.526.g744177e7f7-goog > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear 2020-05-06 20:52 [PATCH 0/2] lib/bpf hashmap portability and fix Ian Rogers 2020-05-06 20:52 ` [PATCH 1/2] lib/bpf hashmap: increase portability Ian Rogers @ 2020-05-06 20:52 ` Ian Rogers 2020-05-06 21:36 ` Andrii Nakryiko 1 sibling, 1 reply; 9+ messages in thread From: Ian Rogers @ 2020-05-06 20:52 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel Cc: Ian Rogers hashmap_find_entry assumes that if buckets is NULL then there are no entries. NULL the buckets in clear to ensure this. Free hashmap entries and not just the bucket array. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/lib/bpf/hashmap.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c index 54c30c802070..1a1bca1ff5cd 100644 --- a/tools/lib/bpf/hashmap.c +++ b/tools/lib/bpf/hashmap.c @@ -59,7 +59,13 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn, void hashmap__clear(struct hashmap *map) { + struct hashmap_entry *cur, *tmp; + size_t bkt; + + hashmap__for_each_entry_safe(map, cur, tmp, bkt) + free(cur); free(map->buckets); + map->buckets = NULL; map->cap = map->cap_bits = map->sz = 0; } -- 2.26.2.526.g744177e7f7-goog ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear 2020-05-06 20:52 ` [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear Ian Rogers @ 2020-05-06 21:36 ` Andrii Nakryiko 2020-05-06 21:53 ` Ian Rogers 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2020-05-06 21:36 UTC (permalink / raw) To: Ian Rogers Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Networking, bpf, open list On Wed, May 6, 2020 at 1:55 PM Ian Rogers <irogers@google.com> wrote: > > hashmap_find_entry assumes that if buckets is NULL then there are no > entries. NULL the buckets in clear to ensure this. > Free hashmap entries and not just the bucket array. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- This is already fixed in bpf-next ([0]). Seems to be 1-to-1 character by character :) [0] https://patchwork.ozlabs.org/project/netdev/patch/20200429012111.277390-5-andriin@fb.com/ > tools/lib/bpf/hashmap.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c > index 54c30c802070..1a1bca1ff5cd 100644 > --- a/tools/lib/bpf/hashmap.c > +++ b/tools/lib/bpf/hashmap.c > @@ -59,7 +59,13 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn, > > void hashmap__clear(struct hashmap *map) > { > + struct hashmap_entry *cur, *tmp; > + size_t bkt; > + > + hashmap__for_each_entry_safe(map, cur, tmp, bkt) > + free(cur); > free(map->buckets); > + map->buckets = NULL; > map->cap = map->cap_bits = map->sz = 0; > } > > -- > 2.26.2.526.g744177e7f7-goog > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear 2020-05-06 21:36 ` Andrii Nakryiko @ 2020-05-06 21:53 ` Ian Rogers 0 siblings, 0 replies; 9+ messages in thread From: Ian Rogers @ 2020-05-06 21:53 UTC (permalink / raw) To: Andrii Nakryiko Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, Networking, bpf, open list On Wed, May 6, 2020 at 2:36 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, May 6, 2020 at 1:55 PM Ian Rogers <irogers@google.com> wrote: > > > > hashmap_find_entry assumes that if buckets is NULL then there are no > > entries. NULL the buckets in clear to ensure this. > > Free hashmap entries and not just the bucket array. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > This is already fixed in bpf-next ([0]). Seems to be 1-to-1 character > by character :) > > [0] https://patchwork.ozlabs.org/project/netdev/patch/20200429012111.277390-5-andriin@fb.com/ Thanks! Ian > > tools/lib/bpf/hashmap.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/tools/lib/bpf/hashmap.c b/tools/lib/bpf/hashmap.c > > index 54c30c802070..1a1bca1ff5cd 100644 > > --- a/tools/lib/bpf/hashmap.c > > +++ b/tools/lib/bpf/hashmap.c > > @@ -59,7 +59,13 @@ struct hashmap *hashmap__new(hashmap_hash_fn hash_fn, > > > > void hashmap__clear(struct hashmap *map) > > { > > + struct hashmap_entry *cur, *tmp; > > + size_t bkt; > > + > > + hashmap__for_each_entry_safe(map, cur, tmp, bkt) > > + free(cur); > > free(map->buckets); > > + map->buckets = NULL; > > map->cap = map->cap_bits = map->sz = 0; > > } > > > > -- > > 2.26.2.526.g744177e7f7-goog > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-05-06 22:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-06 20:52 [PATCH 0/2] lib/bpf hashmap portability and fix Ian Rogers 2020-05-06 20:52 ` [PATCH 1/2] lib/bpf hashmap: increase portability Ian Rogers 2020-05-06 21:33 ` Andrii Nakryiko 2020-05-06 21:47 ` Ian Rogers 2020-05-06 21:56 ` Andrii Nakryiko 2020-05-06 22:13 ` Ian Rogers 2020-05-06 20:52 ` [PATCH 2/2] lib/bpf hashmap: fixes to hashmap__clear Ian Rogers 2020-05-06 21:36 ` Andrii Nakryiko 2020-05-06 21:53 ` Ian Rogers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).