* [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute
@ 2019-10-22 15:04 Toke Høiland-Jørgensen
2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-10-22 15:04 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song,
Jesper Dangaard Brouer, David Miller, netdev, bpf
This series adds support to libbpf for reading 'pinning' settings from BTF-based
map definitions. It introduces new variants of the bpf_object__(un)pin_maps()
functions which accepts options with the new options struct definition style.
These allow the caller to configure pinning options to use both old-style
"forced pinning" where all defined maps are pinned, and the new pinning that is
based on the BTF attribute.
The actual semantics of the pinning is similar to what iproute2 allows today,
and the eventual goal is to move the iproute2 implementation to be based on
libbpf and the functions introduced in this series.
A follow-up series will add support to libbpf for automatic map pinning on
program load (with optional reuse of existing maps if they are already pinned).
However, the functions introduced in this series can be used standalone, and so
I decided to break things up to ease review.
---
Toke Høiland-Jørgensen (3):
libbpf: Store map pin path in struct bpf_map
libbpf: Support configurable pinning of maps from BTF annotations
libbpf: Add pin option to automount BPF filesystem before pinning
tools/lib/bpf/bpf_helpers.h | 8 ++
tools/lib/bpf/libbpf.c | 189 ++++++++++++++++++++++++++++++++++++-------
tools/lib/bpf/libbpf.h | 36 ++++++++
tools/lib/bpf/libbpf.map | 4 +
4 files changed, 208 insertions(+), 29 deletions(-)
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 15:04 [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen @ 2019-10-22 15:04 ` Toke Høiland-Jørgensen 2019-10-22 17:37 ` Andrii Nakryiko 2019-10-22 15:04 ` [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations Toke Høiland-Jørgensen 2019-10-22 15:04 ` [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning Toke Høiland-Jørgensen 2 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 15:04 UTC (permalink / raw) To: Daniel Borkmann Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, netdev, bpf From: Toke Høiland-Jørgensen <toke@redhat.com> When pinning a map, store the pin path in struct bpf_map so it can be re-used later for un-pinning. This simplifies the later addition of per-map pin paths. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- tools/lib/bpf/libbpf.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index cccfd9355134..b4fdd8ee3bbd 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -226,6 +226,7 @@ struct bpf_map { void *priv; bpf_map_clear_priv_t clear_priv; enum libbpf_map_type libbpf_type; + char *pin_path; }; struct bpf_secdata { @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) if (err) goto err_close_new_fd; free(map->name); + zfree(&map->pin_path); map->fd = new_fd; map->name = new_name; @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) return -errno; } + map->pin_path = strdup(path); pr_debug("pinned map '%s'\n", path); return 0; @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) { int err; + if (!path) + path = map->pin_path; + err = check_path(path); if (err) return err; @@ -4044,6 +4050,7 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) if (err != 0) return -errno; pr_debug("unpinned map '%s'\n", path); + zfree(&map->pin_path); return 0; } @@ -4088,17 +4095,10 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) err_unpin_maps: while ((map = bpf_map__prev(map, obj))) { - char buf[PATH_MAX]; - int len; - - len = snprintf(buf, PATH_MAX, "%s/%s", path, - bpf_map__name(map)); - if (len < 0) - continue; - else if (len >= PATH_MAX) + if (!map->pin_path) continue; - bpf_map__unpin(map, buf); + bpf_map__unpin(map, NULL); } return err; @@ -4248,6 +4248,7 @@ void bpf_object__close(struct bpf_object *obj) for (i = 0; i < obj->nr_maps; i++) { zfree(&obj->maps[i].name); + zfree(&obj->maps[i].pin_path); if (obj->maps[i].clear_priv) obj->maps[i].clear_priv(&obj->maps[i], obj->maps[i].priv); ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen @ 2019-10-22 17:37 ` Andrii Nakryiko 2019-10-22 18:13 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-22 17:37 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > When pinning a map, store the pin path in struct bpf_map so it can be > re-used later for un-pinning. This simplifies the later addition of per-map > pin paths. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > tools/lib/bpf/libbpf.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index cccfd9355134..b4fdd8ee3bbd 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -226,6 +226,7 @@ struct bpf_map { > void *priv; > bpf_map_clear_priv_t clear_priv; > enum libbpf_map_type libbpf_type; > + char *pin_path; > }; > > struct bpf_secdata { > @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) > if (err) > goto err_close_new_fd; > free(map->name); > + zfree(&map->pin_path); > While you are touching this function, can you please also fix error handling in it? We should store -errno locally on error, before we call close() which might change errno. > map->fd = new_fd; > map->name = new_name; > @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) > return -errno; > } > > + map->pin_path = strdup(path); if (!map->pin_path) { err = -errno; goto err_close_new_fd; } > pr_debug("pinned map '%s'\n", path); > > return 0; > @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > { > int err; > > + if (!path) > + path = map->pin_path; This semantics is kind of weird. Given we now remember pin_path, should we instead check that user-provided path is actually correct and matches what we stored? Alternatively, bpf_map__unpin() w/o path argument looks like a cleaner API. > + > err = check_path(path); > if (err) > return err; > @@ -4044,6 +4050,7 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > if (err != 0) > return -errno; > pr_debug("unpinned map '%s'\n", path); > + zfree(&map->pin_path); > > return 0; > } > @@ -4088,17 +4095,10 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > > err_unpin_maps: > while ((map = bpf_map__prev(map, obj))) { > - char buf[PATH_MAX]; > - int len; > - > - len = snprintf(buf, PATH_MAX, "%s/%s", path, > - bpf_map__name(map)); > - if (len < 0) > - continue; > - else if (len >= PATH_MAX) > + if (!map->pin_path) > continue; > > - bpf_map__unpin(map, buf); > + bpf_map__unpin(map, NULL); > } > > return err; > @@ -4248,6 +4248,7 @@ void bpf_object__close(struct bpf_object *obj) > > for (i = 0; i < obj->nr_maps; i++) { > zfree(&obj->maps[i].name); > + zfree(&obj->maps[i].pin_path); > if (obj->maps[i].clear_priv) > obj->maps[i].clear_priv(&obj->maps[i], > obj->maps[i].priv); > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 17:37 ` Andrii Nakryiko @ 2019-10-22 18:13 ` Toke Høiland-Jørgensen 2019-10-22 18:23 ` Andrii Nakryiko 0 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 18:13 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> When pinning a map, store the pin path in struct bpf_map so it can be >> re-used later for un-pinning. This simplifies the later addition of per-map >> pin paths. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- >> tools/lib/bpf/libbpf.c | 19 ++++++++++--------- >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index cccfd9355134..b4fdd8ee3bbd 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -226,6 +226,7 @@ struct bpf_map { >> void *priv; >> bpf_map_clear_priv_t clear_priv; >> enum libbpf_map_type libbpf_type; >> + char *pin_path; >> }; >> >> struct bpf_secdata { >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) >> if (err) >> goto err_close_new_fd; >> free(map->name); >> + zfree(&map->pin_path); >> > > While you are touching this function, can you please also fix error > handling in it? We should store -errno locally on error, before we > call close() which might change errno. Didn't actually look much at the surrounding function, TBH. I do expect that I will need to go poke into this for the follow-on "automatic reuse of pinned maps" series anyway. But sure, I can do a bit of cleanup in a standalone patch first :) >> map->fd = new_fd; >> map->name = new_name; >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) >> return -errno; >> } >> >> + map->pin_path = strdup(path); > > if (!map->pin_path) { > err = -errno; > goto err_close_new_fd; > } Right. >> pr_debug("pinned map '%s'\n", path); >> >> return 0; >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) >> { >> int err; >> >> + if (!path) >> + path = map->pin_path; > > This semantics is kind of weird. Given we now remember pin_path, > should we instead check that user-provided path is actually correct > and matches what we stored? Alternatively, bpf_map__unpin() w/o path > argument looks like a cleaner API. Yeah, I guess the function without a path argument would make the most sense. However, we can't really change the API of bpf_map__unpin() (unless you're proposing a symbol-versioned new version?). Dunno if it's worth it to include a new, somewhat oddly-named, function to achieve this? For the internal libbpf uses at least it's easy enough for the caller to just go bpf_map__unpin(map, map->pin_path), so I could also just drop this change? WDYT? -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 18:13 ` Toke Høiland-Jørgensen @ 2019-10-22 18:23 ` Andrii Nakryiko 2019-10-22 18:45 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-22 18:23 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> From: Toke Høiland-Jørgensen <toke@redhat.com> > >> > >> When pinning a map, store the pin path in struct bpf_map so it can be > >> re-used later for un-pinning. This simplifies the later addition of per-map > >> pin paths. > >> > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> --- > >> tools/lib/bpf/libbpf.c | 19 ++++++++++--------- > >> 1 file changed, 10 insertions(+), 9 deletions(-) > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index cccfd9355134..b4fdd8ee3bbd 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -226,6 +226,7 @@ struct bpf_map { > >> void *priv; > >> bpf_map_clear_priv_t clear_priv; > >> enum libbpf_map_type libbpf_type; > >> + char *pin_path; > >> }; > >> > >> struct bpf_secdata { > >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) > >> if (err) > >> goto err_close_new_fd; > >> free(map->name); > >> + zfree(&map->pin_path); > >> > > > > While you are touching this function, can you please also fix error > > handling in it? We should store -errno locally on error, before we > > call close() which might change errno. > > Didn't actually look much at the surrounding function, TBH. I do expect > that I will need to go poke into this for the follow-on "automatic reuse > of pinned maps" series anyway. But sure, I can do a bit of cleanup in a > standalone patch first :) > > >> map->fd = new_fd; > >> map->name = new_name; > >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) > >> return -errno; > >> } > >> > >> + map->pin_path = strdup(path); > > > > if (!map->pin_path) { > > err = -errno; > > goto err_close_new_fd; > > } > > Right. > > >> pr_debug("pinned map '%s'\n", path); > >> > >> return 0; > >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > >> { > >> int err; > >> > >> + if (!path) > >> + path = map->pin_path; > > > > This semantics is kind of weird. Given we now remember pin_path, > > should we instead check that user-provided path is actually correct > > and matches what we stored? Alternatively, bpf_map__unpin() w/o path > > argument looks like a cleaner API. > > Yeah, I guess the function without a path argument would make the most > sense. However, we can't really change the API of bpf_map__unpin() > (unless you're proposing a symbol-versioned new version?). Dunno if it's > worth it to include a new, somewhat oddly-named, function to achieve > this? For the internal libbpf uses at least it's easy enough for the > caller to just go bpf_map__unpin(map, map->pin_path), so I could also > just drop this change? WDYT? I'd probably do strcmp(map->pin_path, path), if path is specified. This will support existing use cases, will allow NULL if we don't want to bother remembering pin_path, will prevent weird use case of pinning to one path, but unpinning another one. Ideally, all this pinning will just be done declaratively and will happen automatically, so users won't even have to know about this API :) > > -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 18:23 ` Andrii Nakryiko @ 2019-10-22 18:45 ` Toke Høiland-Jørgensen 2019-10-22 18:49 ` Andrii Nakryiko 0 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 18:45 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Oct 22, 2019 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> >> >> When pinning a map, store the pin path in struct bpf_map so it can be >> >> re-used later for un-pinning. This simplifies the later addition of per-map >> >> pin paths. >> >> >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> >> --- >> >> tools/lib/bpf/libbpf.c | 19 ++++++++++--------- >> >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >> >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> >> index cccfd9355134..b4fdd8ee3bbd 100644 >> >> --- a/tools/lib/bpf/libbpf.c >> >> +++ b/tools/lib/bpf/libbpf.c >> >> @@ -226,6 +226,7 @@ struct bpf_map { >> >> void *priv; >> >> bpf_map_clear_priv_t clear_priv; >> >> enum libbpf_map_type libbpf_type; >> >> + char *pin_path; >> >> }; >> >> >> >> struct bpf_secdata { >> >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) >> >> if (err) >> >> goto err_close_new_fd; >> >> free(map->name); >> >> + zfree(&map->pin_path); >> >> >> > >> > While you are touching this function, can you please also fix error >> > handling in it? We should store -errno locally on error, before we >> > call close() which might change errno. >> >> Didn't actually look much at the surrounding function, TBH. I do expect >> that I will need to go poke into this for the follow-on "automatic reuse >> of pinned maps" series anyway. But sure, I can do a bit of cleanup in a >> standalone patch first :) >> >> >> map->fd = new_fd; >> >> map->name = new_name; >> >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) >> >> return -errno; >> >> } >> >> >> >> + map->pin_path = strdup(path); >> > >> > if (!map->pin_path) { >> > err = -errno; >> > goto err_close_new_fd; >> > } >> >> Right. >> >> >> pr_debug("pinned map '%s'\n", path); >> >> >> >> return 0; >> >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) >> >> { >> >> int err; >> >> >> >> + if (!path) >> >> + path = map->pin_path; >> > >> > This semantics is kind of weird. Given we now remember pin_path, >> > should we instead check that user-provided path is actually correct >> > and matches what we stored? Alternatively, bpf_map__unpin() w/o path >> > argument looks like a cleaner API. >> >> Yeah, I guess the function without a path argument would make the most >> sense. However, we can't really change the API of bpf_map__unpin() >> (unless you're proposing a symbol-versioned new version?). Dunno if it's >> worth it to include a new, somewhat oddly-named, function to achieve >> this? For the internal libbpf uses at least it's easy enough for the >> caller to just go bpf_map__unpin(map, map->pin_path), so I could also >> just drop this change? WDYT? > > I'd probably do strcmp(map->pin_path, path), if path is specified. > This will support existing use cases, will allow NULL if we don't want > to bother remembering pin_path, will prevent weird use case of pinning > to one path, but unpinning another one. So something like if (path && map->pin_path && strcmp(path, map->pin_path)) return -EINVAL else if (!path) path = map->pin_path; ? > Ideally, all this pinning will just be done declaratively and will > happen automatically, so users won't even have to know about this API > :) Yeah, that's where I'm hoping to get to. But, well, the pin/unpin functions already exist so we do need to keep them working... -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 18:45 ` Toke Høiland-Jørgensen @ 2019-10-22 18:49 ` Andrii Nakryiko 2019-10-22 19:06 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-22 18:49 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 11:45 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Oct 22, 2019 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> > >> > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> > >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> > >> >> > >> >> When pinning a map, store the pin path in struct bpf_map so it can be > >> >> re-used later for un-pinning. This simplifies the later addition of per-map > >> >> pin paths. > >> >> > >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> >> --- > >> >> tools/lib/bpf/libbpf.c | 19 ++++++++++--------- > >> >> 1 file changed, 10 insertions(+), 9 deletions(-) > >> >> > >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> >> index cccfd9355134..b4fdd8ee3bbd 100644 > >> >> --- a/tools/lib/bpf/libbpf.c > >> >> +++ b/tools/lib/bpf/libbpf.c > >> >> @@ -226,6 +226,7 @@ struct bpf_map { > >> >> void *priv; > >> >> bpf_map_clear_priv_t clear_priv; > >> >> enum libbpf_map_type libbpf_type; > >> >> + char *pin_path; > >> >> }; > >> >> > >> >> struct bpf_secdata { > >> >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) > >> >> if (err) > >> >> goto err_close_new_fd; > >> >> free(map->name); > >> >> + zfree(&map->pin_path); > >> >> > >> > > >> > While you are touching this function, can you please also fix error > >> > handling in it? We should store -errno locally on error, before we > >> > call close() which might change errno. > >> > >> Didn't actually look much at the surrounding function, TBH. I do expect > >> that I will need to go poke into this for the follow-on "automatic reuse > >> of pinned maps" series anyway. But sure, I can do a bit of cleanup in a > >> standalone patch first :) > >> > >> >> map->fd = new_fd; > >> >> map->name = new_name; > >> >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) > >> >> return -errno; > >> >> } > >> >> > >> >> + map->pin_path = strdup(path); > >> > > >> > if (!map->pin_path) { > >> > err = -errno; > >> > goto err_close_new_fd; > >> > } > >> > >> Right. > >> > >> >> pr_debug("pinned map '%s'\n", path); > >> >> > >> >> return 0; > >> >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > >> >> { > >> >> int err; > >> >> > >> >> + if (!path) > >> >> + path = map->pin_path; > >> > > >> > This semantics is kind of weird. Given we now remember pin_path, > >> > should we instead check that user-provided path is actually correct > >> > and matches what we stored? Alternatively, bpf_map__unpin() w/o path > >> > argument looks like a cleaner API. > >> > >> Yeah, I guess the function without a path argument would make the most > >> sense. However, we can't really change the API of bpf_map__unpin() > >> (unless you're proposing a symbol-versioned new version?). Dunno if it's > >> worth it to include a new, somewhat oddly-named, function to achieve > >> this? For the internal libbpf uses at least it's easy enough for the > >> caller to just go bpf_map__unpin(map, map->pin_path), so I could also > >> just drop this change? WDYT? > > > > I'd probably do strcmp(map->pin_path, path), if path is specified. > > This will support existing use cases, will allow NULL if we don't want > > to bother remembering pin_path, will prevent weird use case of pinning > > to one path, but unpinning another one. > > So something like > > if (path && map->pin_path && strcmp(path, map->pin_path)) can we unpin not pinned map? sounds like an error condition? so: if (!map->pin_path) return -EWHATAREYOUDOING; if (path && strcmp(path, map->pin_path)) return -EHUH; path = map->pin_path; /* or just use map->ping_path explicitly */ ... proceed ... > return -EINVAL > else if (!path) > path = map->pin_path; > > ? > > > Ideally, all this pinning will just be done declaratively and will > > happen automatically, so users won't even have to know about this API > > :) > > Yeah, that's where I'm hoping to get to. But, well, the pin/unpin > functions already exist so we do need to keep them working... > > -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 18:49 ` Andrii Nakryiko @ 2019-10-22 19:06 ` Toke Høiland-Jørgensen 2019-10-23 4:43 ` Andrii Nakryiko 0 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 19:06 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Oct 22, 2019 at 11:45 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Tue, Oct 22, 2019 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> >> >> > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> >> >> >> >> When pinning a map, store the pin path in struct bpf_map so it can be >> >> >> re-used later for un-pinning. This simplifies the later addition of per-map >> >> >> pin paths. >> >> >> >> >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> >> >> --- >> >> >> tools/lib/bpf/libbpf.c | 19 ++++++++++--------- >> >> >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >> >> >> >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> >> >> index cccfd9355134..b4fdd8ee3bbd 100644 >> >> >> --- a/tools/lib/bpf/libbpf.c >> >> >> +++ b/tools/lib/bpf/libbpf.c >> >> >> @@ -226,6 +226,7 @@ struct bpf_map { >> >> >> void *priv; >> >> >> bpf_map_clear_priv_t clear_priv; >> >> >> enum libbpf_map_type libbpf_type; >> >> >> + char *pin_path; >> >> >> }; >> >> >> >> >> >> struct bpf_secdata { >> >> >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) >> >> >> if (err) >> >> >> goto err_close_new_fd; >> >> >> free(map->name); >> >> >> + zfree(&map->pin_path); >> >> >> >> >> > >> >> > While you are touching this function, can you please also fix error >> >> > handling in it? We should store -errno locally on error, before we >> >> > call close() which might change errno. >> >> >> >> Didn't actually look much at the surrounding function, TBH. I do expect >> >> that I will need to go poke into this for the follow-on "automatic reuse >> >> of pinned maps" series anyway. But sure, I can do a bit of cleanup in a >> >> standalone patch first :) >> >> >> >> >> map->fd = new_fd; >> >> >> map->name = new_name; >> >> >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) >> >> >> return -errno; >> >> >> } >> >> >> >> >> >> + map->pin_path = strdup(path); >> >> > >> >> > if (!map->pin_path) { >> >> > err = -errno; >> >> > goto err_close_new_fd; >> >> > } >> >> >> >> Right. >> >> >> >> >> pr_debug("pinned map '%s'\n", path); >> >> >> >> >> >> return 0; >> >> >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) >> >> >> { >> >> >> int err; >> >> >> >> >> >> + if (!path) >> >> >> + path = map->pin_path; >> >> > >> >> > This semantics is kind of weird. Given we now remember pin_path, >> >> > should we instead check that user-provided path is actually correct >> >> > and matches what we stored? Alternatively, bpf_map__unpin() w/o path >> >> > argument looks like a cleaner API. >> >> >> >> Yeah, I guess the function without a path argument would make the most >> >> sense. However, we can't really change the API of bpf_map__unpin() >> >> (unless you're proposing a symbol-versioned new version?). Dunno if it's >> >> worth it to include a new, somewhat oddly-named, function to achieve >> >> this? For the internal libbpf uses at least it's easy enough for the >> >> caller to just go bpf_map__unpin(map, map->pin_path), so I could also >> >> just drop this change? WDYT? >> > >> > I'd probably do strcmp(map->pin_path, path), if path is specified. >> > This will support existing use cases, will allow NULL if we don't want >> > to bother remembering pin_path, will prevent weird use case of pinning >> > to one path, but unpinning another one. >> >> So something like >> >> if (path && map->pin_path && strcmp(path, map->pin_path)) > > can we unpin not pinned map? sounds like an error condition? See my other comment about programs exiting. For an example, see the XDP tutorial (just pretend for a moment that that TODO comment was actually there :)): https://github.com/xdp-project/xdp-tutorial/blob/master/basic04-pinning-maps/xdp_loader.c#L135 -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map 2019-10-22 19:06 ` Toke Høiland-Jørgensen @ 2019-10-23 4:43 ` Andrii Nakryiko 0 siblings, 0 replies; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-23 4:43 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 12:06 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Oct 22, 2019 at 11:45 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> > >> > On Tue, Oct 22, 2019 at 11:13 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> > >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >> >> > >> >> > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> >> >> > >> >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> > >> >> >> > >> >> >> When pinning a map, store the pin path in struct bpf_map so it can be > >> >> >> re-used later for un-pinning. This simplifies the later addition of per-map > >> >> >> pin paths. > >> >> >> > >> >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> >> >> --- > >> >> >> tools/lib/bpf/libbpf.c | 19 ++++++++++--------- > >> >> >> 1 file changed, 10 insertions(+), 9 deletions(-) > >> >> >> > >> >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> >> >> index cccfd9355134..b4fdd8ee3bbd 100644 > >> >> >> --- a/tools/lib/bpf/libbpf.c > >> >> >> +++ b/tools/lib/bpf/libbpf.c > >> >> >> @@ -226,6 +226,7 @@ struct bpf_map { > >> >> >> void *priv; > >> >> >> bpf_map_clear_priv_t clear_priv; > >> >> >> enum libbpf_map_type libbpf_type; > >> >> >> + char *pin_path; > >> >> >> }; > >> >> >> > >> >> >> struct bpf_secdata { > >> >> >> @@ -1929,6 +1930,7 @@ int bpf_map__reuse_fd(struct bpf_map *map, int fd) > >> >> >> if (err) > >> >> >> goto err_close_new_fd; > >> >> >> free(map->name); > >> >> >> + zfree(&map->pin_path); > >> >> >> > >> >> > > >> >> > While you are touching this function, can you please also fix error > >> >> > handling in it? We should store -errno locally on error, before we > >> >> > call close() which might change errno. > >> >> > >> >> Didn't actually look much at the surrounding function, TBH. I do expect > >> >> that I will need to go poke into this for the follow-on "automatic reuse > >> >> of pinned maps" series anyway. But sure, I can do a bit of cleanup in a > >> >> standalone patch first :) > >> >> > >> >> >> map->fd = new_fd; > >> >> >> map->name = new_name; > >> >> >> @@ -4022,6 +4024,7 @@ int bpf_map__pin(struct bpf_map *map, const char *path) > >> >> >> return -errno; > >> >> >> } > >> >> >> > >> >> >> + map->pin_path = strdup(path); > >> >> > > >> >> > if (!map->pin_path) { > >> >> > err = -errno; > >> >> > goto err_close_new_fd; > >> >> > } > >> >> > >> >> Right. > >> >> > >> >> >> pr_debug("pinned map '%s'\n", path); > >> >> >> > >> >> >> return 0; > >> >> >> @@ -4031,6 +4034,9 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > >> >> >> { > >> >> >> int err; > >> >> >> > >> >> >> + if (!path) > >> >> >> + path = map->pin_path; > >> >> > > >> >> > This semantics is kind of weird. Given we now remember pin_path, > >> >> > should we instead check that user-provided path is actually correct > >> >> > and matches what we stored? Alternatively, bpf_map__unpin() w/o path > >> >> > argument looks like a cleaner API. > >> >> > >> >> Yeah, I guess the function without a path argument would make the most > >> >> sense. However, we can't really change the API of bpf_map__unpin() > >> >> (unless you're proposing a symbol-versioned new version?). Dunno if it's > >> >> worth it to include a new, somewhat oddly-named, function to achieve > >> >> this? For the internal libbpf uses at least it's easy enough for the > >> >> caller to just go bpf_map__unpin(map, map->pin_path), so I could also > >> >> just drop this change? WDYT? > >> > > >> > I'd probably do strcmp(map->pin_path, path), if path is specified. > >> > This will support existing use cases, will allow NULL if we don't want > >> > to bother remembering pin_path, will prevent weird use case of pinning > >> > to one path, but unpinning another one. > >> > >> So something like > >> > >> if (path && map->pin_path && strcmp(path, map->pin_path)) > > > > can we unpin not pinned map? sounds like an error condition? > > See my other comment about programs exiting. For an example, see the XDP > tutorial (just pretend for a moment that that TODO comment was actually > there :)): replied about re-pinning/sharing, that should come from map definition anyways and should be handled by map sharing/reuse code, so I think we should be good there. > > https://github.com/xdp-project/xdp-tutorial/blob/master/basic04-pinning-maps/xdp_loader.c#L135 For the clean up use case, if we pinned map, we have its pin_path stored, and can unpin. For rare case where we want unconditionally "unpin" map, why app can't just delete the file, if app is so smart as to know pin path of some other app's map ;) > > -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-22 15:04 [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen 2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen @ 2019-10-22 15:04 ` Toke Høiland-Jørgensen 2019-10-22 18:20 ` Andrii Nakryiko 2019-10-22 15:04 ` [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning Toke Høiland-Jørgensen 2 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 15:04 UTC (permalink / raw) To: Daniel Borkmann Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, netdev, bpf From: Toke Høiland-Jørgensen <toke@redhat.com> This adds support to libbpf for setting map pinning information as part of the BTF map declaration. We introduce a new pair of functions to pin and unpin maps based on this setting, as well as a getter and setter function for the pin information that callers can use after map load. The pin_type supports two modes: LOCAL pinning, which requires the caller to set a pin path using bpf_object_pin_opts, and a global mode, where the path can still be overridden, but defaults to /sys/fs/bpf. This is inspired by the two modes supported by the iproute2 map definitions. In particular, it should be possible to express the current iproute2 operating mode in terms of the options introduced here. The new pin functions will skip any maps that do not have a pinning type set, unless the 'override_type' option is set, in which case all maps will be pinning using the pin type set in that option. This also makes it possible to express the old pin_maps and unpin_maps functions in terms of the new option-based functions. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- tools/lib/bpf/bpf_helpers.h | 8 +++ tools/lib/bpf/libbpf.c | 123 ++++++++++++++++++++++++++++++++++++------- tools/lib/bpf/libbpf.h | 33 ++++++++++++ tools/lib/bpf/libbpf.map | 4 + 4 files changed, 148 insertions(+), 20 deletions(-) diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h index 2203595f38c3..a23cf55d41b1 100644 --- a/tools/lib/bpf/bpf_helpers.h +++ b/tools/lib/bpf/bpf_helpers.h @@ -38,4 +38,12 @@ struct bpf_map_def { unsigned int map_flags; }; +enum libbpf_pin_type { + LIBBPF_PIN_NONE, + /* PIN_LOCAL: pin maps by name in path specified by caller */ + LIBBPF_PIN_LOCAL, + /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */ + LIBBPF_PIN_GLOBAL, +}; + #endif diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index b4fdd8ee3bbd..aea3916de341 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -226,6 +226,7 @@ struct bpf_map { void *priv; bpf_map_clear_priv_t clear_priv; enum libbpf_map_type libbpf_type; + enum libbpf_pin_type pinning; char *pin_path; }; @@ -1270,6 +1271,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, } map->def.value_size = sz; map->btf_value_type_id = t->type; + } else if (strcmp(name, "pinning") == 0) { + __u32 val; + + if (!get_map_field_int(map_name, obj->btf, def, m, + &val)) + return -EINVAL; + pr_debug("map '%s': found pinning = %u.\n", + map_name, val); + + if (val && val != LIBBPF_PIN_LOCAL && + val != LIBBPF_PIN_GLOBAL) { + pr_warning("map '%s': invalid pinning value %u.\n", + map_name, val); + return -EINVAL; + } + map->pinning = val; } else { if (strict) { pr_warning("map '%s': unknown field '%s'.\n", @@ -4055,10 +4072,51 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) return 0; } -int bpf_object__pin_maps(struct bpf_object *obj, const char *path) +static int get_pin_path(char *buf, size_t buf_len, + struct bpf_map *map, struct bpf_object_pin_opts *opts, + bool mkdir) +{ + enum libbpf_pin_type type; + const char *path; + int err, len; + + type = OPTS_GET(opts, override_type, 0) ?: map->pinning; + + if (type == LIBBPF_PIN_GLOBAL) { + path = OPTS_GET(opts, path_global, NULL); + if (!path) + path = "/sys/fs/bpf"; + } else if (type == LIBBPF_PIN_LOCAL) { + path = OPTS_GET(opts, path_local, NULL); + if (!path) { + pr_warning("map '%s' set pinning to PIN_LOCAL, " + "but no local path provided. Skipping.\n", + bpf_map__name(map)); + return 0; + } + } else { + return 0; + } + + if (mkdir) { + err = make_dir(path); + if (err) + return err; + } + + len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map)); + if (len < 0) + return -EINVAL; + else if (len >= buf_len) + return -ENAMETOOLONG; + return len; +} + +int bpf_object__pin_maps_opts(struct bpf_object *obj, + struct bpf_object_pin_opts *opts) { struct bpf_map *map; - int err; + int err, len; if (!obj) return -ENOENT; @@ -4068,21 +4126,17 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) return -ENOENT; } - err = make_dir(path); - if (err) - return err; + if (!OPTS_VALID(opts, bpf_object_pin_opts)) + return -EINVAL; bpf_object__for_each_map(map, obj) { char buf[PATH_MAX]; - int len; - len = snprintf(buf, PATH_MAX, "%s/%s", path, - bpf_map__name(map)); - if (len < 0) { - err = -EINVAL; - goto err_unpin_maps; - } else if (len >= PATH_MAX) { - err = -ENAMETOOLONG; + len = get_pin_path(buf, PATH_MAX, map, opts, true); + if (len == 0) { + continue; + } else if (len < 0) { + err = len; goto err_unpin_maps; } @@ -4104,7 +4158,16 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) return err; } -int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) +int bpf_object__pin_maps(struct bpf_object *obj, const char *path) +{ + LIBBPF_OPTS(bpf_object_pin_opts, opts, + .path_global = path, + .override_type = LIBBPF_PIN_GLOBAL); + return bpf_object__pin_maps_opts(obj, &opts); +} + +int bpf_object__unpin_maps_opts(struct bpf_object *obj, + struct bpf_object_pin_opts *opts) { struct bpf_map *map; int err; @@ -4112,16 +4175,18 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) if (!obj) return -ENOENT; + if (!OPTS_VALID(opts, bpf_object_pin_opts)) + return -EINVAL; + bpf_object__for_each_map(map, obj) { char buf[PATH_MAX]; int len; - len = snprintf(buf, PATH_MAX, "%s/%s", path, - bpf_map__name(map)); - if (len < 0) - return -EINVAL; - else if (len >= PATH_MAX) - return -ENAMETOOLONG; + len = get_pin_path(buf, PATH_MAX, map, opts, false); + if (len == 0) + continue; + else if (len < 0) + return len; err = bpf_map__unpin(map, buf); if (err) @@ -4131,6 +4196,14 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) return 0; } +int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) +{ + LIBBPF_OPTS(bpf_object_pin_opts, opts, + .path_global = path, + .override_type = LIBBPF_PIN_GLOBAL); + return bpf_object__unpin_maps_opts(obj, &opts); +} + int bpf_object__pin_programs(struct bpf_object *obj, const char *path) { struct bpf_program *prog; @@ -4782,6 +4855,16 @@ void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex) map->map_ifindex = ifindex; } +void bpf_map__set_pinning(struct bpf_map *map, enum libbpf_pin_type pinning) +{ + map->pinning = pinning; +} + +enum libbpf_pin_type bpf_map__get_pinning(struct bpf_map *map) +{ + return map->pinning; +} + int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd) { if (!bpf_map_type__is_map_in_map(map->def.type)) { diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 53ce212764e0..2131eeafb18d 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -119,9 +119,40 @@ int bpf_object__section_size(const struct bpf_object *obj, const char *name, __u32 *size); int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, __u32 *off); + +enum libbpf_pin_type { + LIBBPF_PIN_NONE, + /* PIN_LOCAL: pin maps by name in path specified by caller */ + LIBBPF_PIN_LOCAL, + /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */ + LIBBPF_PIN_GLOBAL, +}; + +struct bpf_object_pin_opts { + /* size of this struct, for forward/backward compatiblity */ + size_t sz; + + /* Paths to pin maps setting PIN_GLOBAL and PIN_LOCAL auto-pin option. + * The global path defaults to /sys/fs/bpf, while the local path has + * no default (so the option must be set if that pin type is used). + */ + const char *path_global; + const char *path_local; + + /* If set, the pin type specified in map definitions will be ignored, + * and this type used for all maps instead. + */ + enum libbpf_pin_type override_type; +}; +#define bpf_object_pin_opts__last_field override_type + LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, const char *path); +LIBBPF_API int bpf_object__pin_maps_opts(struct bpf_object *obj, + struct bpf_object_pin_opts *opts); +LIBBPF_API int bpf_object__unpin_maps_opts(struct bpf_object *obj, + struct bpf_object_pin_opts *opts); LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, const char *path); LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, @@ -377,6 +408,8 @@ LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map); LIBBPF_API void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex); LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path); LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path); +LIBBPF_API enum libbpf_pin_type bpf_map__get_pinning(struct bpf_map *map); +LIBBPF_API void bpf_map__set_pinning(struct bpf_map *map, enum libbpf_pin_type); LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd); diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 4d241fd92dd4..d0aacb3e14fb 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -195,4 +195,8 @@ LIBBPF_0.0.6 { global: bpf_object__open_file; bpf_object__open_mem; + bpf_object__pin_maps_opts; + bpf_object__unpin_maps_opts; + bpf_map__get_pinning; + bpf_map__set_pinning; } LIBBPF_0.0.5; ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-22 15:04 ` [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations Toke Høiland-Jørgensen @ 2019-10-22 18:20 ` Andrii Nakryiko 2019-10-22 18:57 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-22 18:20 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > This adds support to libbpf for setting map pinning information as part of > the BTF map declaration. We introduce a new pair of functions to pin and > unpin maps based on this setting, as well as a getter and setter function > for the pin information that callers can use after map load. > > The pin_type supports two modes: LOCAL pinning, which requires the caller > to set a pin path using bpf_object_pin_opts, and a global mode, where the > path can still be overridden, but defaults to /sys/fs/bpf. This is inspired > by the two modes supported by the iproute2 map definitions. In particular, > it should be possible to express the current iproute2 operating mode in > terms of the options introduced here. > > The new pin functions will skip any maps that do not have a pinning type > set, unless the 'override_type' option is set, in which case all maps will > be pinning using the pin type set in that option. This also makes it > possible to express the old pin_maps and unpin_maps functions in terms of > the new option-based functions. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- So few high-level thoughts. 1. I'd start with just NONE and GLOBAL as two pinning modes. It might be worth-while to name GLOBAL something different just to specify that it is just pinning, either to default /sys/fs/bpf root or some other user-provided root path. 1a. LOCAL seems to behave exactly like GLOBAL, just uses separate option for a path. So we effectively have two GLOBAL modes, one with default (but overrideable) /sys/fs/bpf, another with user-provided mandatory path. The distinction seem rather small and arbitrary. What's the use case? 2. When is pin type override useful? Either specify it once declaratively in map definition, or just do pinning programmatically? 3. I think we should make pinning path override into bpf_object_open_opts and keep bpf_object__pin_maps simple. We are probably going to make map pinning/sharing automatic anyway, so that will need to happen as part of either open or load operation. 4. Once pinned, map knows its pinned path, just use that, I don't see any reasonable use case where you'd want to override path just for unpinning. Does it make sense? > tools/lib/bpf/bpf_helpers.h | 8 +++ > tools/lib/bpf/libbpf.c | 123 ++++++++++++++++++++++++++++++++++++------- > tools/lib/bpf/libbpf.h | 33 ++++++++++++ > tools/lib/bpf/libbpf.map | 4 + > 4 files changed, 148 insertions(+), 20 deletions(-) > > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > index 2203595f38c3..a23cf55d41b1 100644 > --- a/tools/lib/bpf/bpf_helpers.h > +++ b/tools/lib/bpf/bpf_helpers.h > @@ -38,4 +38,12 @@ struct bpf_map_def { > unsigned int map_flags; > }; > > +enum libbpf_pin_type { > + LIBBPF_PIN_NONE, > + /* PIN_LOCAL: pin maps by name in path specified by caller */ > + LIBBPF_PIN_LOCAL, Daniel mentioned in previous discussions that LOCAL mode is never used. I'd like to avoid supporting unnecessary stuff. Is it really useful? > + /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */ > + LIBBPF_PIN_GLOBAL, > +}; > + > #endif > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index b4fdd8ee3bbd..aea3916de341 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -226,6 +226,7 @@ struct bpf_map { > void *priv; > bpf_map_clear_priv_t clear_priv; > enum libbpf_map_type libbpf_type; > + enum libbpf_pin_type pinning; > char *pin_path; > }; > > @@ -1270,6 +1271,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > } > map->def.value_size = sz; > map->btf_value_type_id = t->type; > + } else if (strcmp(name, "pinning") == 0) { > + __u32 val; > + > + if (!get_map_field_int(map_name, obj->btf, def, m, > + &val)) > + return -EINVAL; > + pr_debug("map '%s': found pinning = %u.\n", > + map_name, val); > + > + if (val && val != LIBBPF_PIN_LOCAL && > + val != LIBBPF_PIN_GLOBAL) { let's write out LIBBPF_PIN_NONE explicitly, instead of just `val`? > + pr_warning("map '%s': invalid pinning value %u.\n", > + map_name, val); > + return -EINVAL; > + } > + map->pinning = val; > } else { > if (strict) { > pr_warning("map '%s': unknown field '%s'.\n", > @@ -4055,10 +4072,51 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > return 0; > } > > -int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > +static int get_pin_path(char *buf, size_t buf_len, > + struct bpf_map *map, struct bpf_object_pin_opts *opts, > + bool mkdir) > +{ > + enum libbpf_pin_type type; > + const char *path; > + int err, len; > + > + type = OPTS_GET(opts, override_type, 0) ?: map->pinning; > + > + if (type == LIBBPF_PIN_GLOBAL) { > + path = OPTS_GET(opts, path_global, NULL); > + if (!path) > + path = "/sys/fs/bpf"; > + } else if (type == LIBBPF_PIN_LOCAL) { > + path = OPTS_GET(opts, path_local, NULL); > + if (!path) { > + pr_warning("map '%s' set pinning to PIN_LOCAL, " > + "but no local path provided. Skipping.\n", > + bpf_map__name(map)); > + return 0; > + } > + } else { > + return 0; > + } > + > + if (mkdir) { > + err = make_dir(path); > + if (err) > + return err; > + } > + > + len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map)); > + if (len < 0) > + return -EINVAL; > + else if (len >= buf_len) > + return -ENAMETOOLONG; > + return len; > +} > + > +int bpf_object__pin_maps_opts(struct bpf_object *obj, > + struct bpf_object_pin_opts *opts) > { > struct bpf_map *map; > - int err; > + int err, len; > > if (!obj) > return -ENOENT; > @@ -4068,21 +4126,17 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > return -ENOENT; > } > > - err = make_dir(path); > - if (err) > - return err; > + if (!OPTS_VALID(opts, bpf_object_pin_opts)) > + return -EINVAL; > > bpf_object__for_each_map(map, obj) { > char buf[PATH_MAX]; > - int len; > > - len = snprintf(buf, PATH_MAX, "%s/%s", path, > - bpf_map__name(map)); > - if (len < 0) { > - err = -EINVAL; > - goto err_unpin_maps; > - } else if (len >= PATH_MAX) { > - err = -ENAMETOOLONG; > + len = get_pin_path(buf, PATH_MAX, map, opts, true); > + if (len == 0) { > + continue; > + } else if (len < 0) { > + err = len; > goto err_unpin_maps; > } > > @@ -4104,7 +4158,16 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > return err; > } > > -int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > +int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > +{ > + LIBBPF_OPTS(bpf_object_pin_opts, opts, > + .path_global = path, > + .override_type = LIBBPF_PIN_GLOBAL); style nit: extra line between declaration and statements > + return bpf_object__pin_maps_opts(obj, &opts); > +} > + > +int bpf_object__unpin_maps_opts(struct bpf_object *obj, > + struct bpf_object_pin_opts *opts) > { > struct bpf_map *map; > int err; > @@ -4112,16 +4175,18 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > if (!obj) > return -ENOENT; > > + if (!OPTS_VALID(opts, bpf_object_pin_opts)) > + return -EINVAL; specifying pin options for unpin operation looks cumbersome. We know the pinned path, just use that and keep unpinning simple? > + > bpf_object__for_each_map(map, obj) { > char buf[PATH_MAX]; > int len; > > - len = snprintf(buf, PATH_MAX, "%s/%s", path, > - bpf_map__name(map)); > - if (len < 0) > - return -EINVAL; > - else if (len >= PATH_MAX) > - return -ENAMETOOLONG; > + len = get_pin_path(buf, PATH_MAX, map, opts, false); > + if (len == 0) > + continue; > + else if (len < 0) > + return len; this whole logic should be replaced with just map->pin_path or am I missing some important use case? > > err = bpf_map__unpin(map, buf); > if (err) > @@ -4131,6 +4196,14 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > return 0; > } > > +int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > +{ > + LIBBPF_OPTS(bpf_object_pin_opts, opts, > + .path_global = path, > + .override_type = LIBBPF_PIN_GLOBAL); same styling nit > + return bpf_object__unpin_maps_opts(obj, &opts); > +} > + > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > { > struct bpf_program *prog; > @@ -4782,6 +4855,16 @@ void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex) > map->map_ifindex = ifindex; > } > > +void bpf_map__set_pinning(struct bpf_map *map, enum libbpf_pin_type pinning) > +{ > + map->pinning = pinning; > +} > + > +enum libbpf_pin_type bpf_map__get_pinning(struct bpf_map *map) > +{ > + return map->pinning; > +} > + > int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd) > { > if (!bpf_map_type__is_map_in_map(map->def.type)) { > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 53ce212764e0..2131eeafb18d 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -119,9 +119,40 @@ int bpf_object__section_size(const struct bpf_object *obj, const char *name, > __u32 *size); > int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > __u32 *off); > + > +enum libbpf_pin_type { > + LIBBPF_PIN_NONE, > + /* PIN_LOCAL: pin maps by name in path specified by caller */ > + LIBBPF_PIN_LOCAL, > + /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */ > + LIBBPF_PIN_GLOBAL, > +}; > + > +struct bpf_object_pin_opts { > + /* size of this struct, for forward/backward compatiblity */ > + size_t sz; > + > + /* Paths to pin maps setting PIN_GLOBAL and PIN_LOCAL auto-pin option. > + * The global path defaults to /sys/fs/bpf, while the local path has > + * no default (so the option must be set if that pin type is used). > + */ > + const char *path_global; > + const char *path_local; > + > + /* If set, the pin type specified in map definitions will be ignored, > + * and this type used for all maps instead. > + */ > + enum libbpf_pin_type override_type; > +}; > +#define bpf_object_pin_opts__last_field override_type > + > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > const char *path); > +LIBBPF_API int bpf_object__pin_maps_opts(struct bpf_object *obj, > + struct bpf_object_pin_opts *opts); > +LIBBPF_API int bpf_object__unpin_maps_opts(struct bpf_object *obj, > + struct bpf_object_pin_opts *opts); > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > const char *path); > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > @@ -377,6 +408,8 @@ LIBBPF_API bool bpf_map__is_internal(const struct bpf_map *map); > LIBBPF_API void bpf_map__set_ifindex(struct bpf_map *map, __u32 ifindex); > LIBBPF_API int bpf_map__pin(struct bpf_map *map, const char *path); > LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path); > +LIBBPF_API enum libbpf_pin_type bpf_map__get_pinning(struct bpf_map *map); > +LIBBPF_API void bpf_map__set_pinning(struct bpf_map *map, enum libbpf_pin_type); > > LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd); > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index 4d241fd92dd4..d0aacb3e14fb 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -195,4 +195,8 @@ LIBBPF_0.0.6 { > global: > bpf_object__open_file; > bpf_object__open_mem; > + bpf_object__pin_maps_opts; > + bpf_object__unpin_maps_opts; > + bpf_map__get_pinning; > + bpf_map__set_pinning; > } LIBBPF_0.0.5; > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-22 18:20 ` Andrii Nakryiko @ 2019-10-22 18:57 ` Toke Høiland-Jørgensen 2019-10-23 4:40 ` Andrii Nakryiko 0 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 18:57 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> This adds support to libbpf for setting map pinning information as part of >> the BTF map declaration. We introduce a new pair of functions to pin and >> unpin maps based on this setting, as well as a getter and setter function >> for the pin information that callers can use after map load. >> >> The pin_type supports two modes: LOCAL pinning, which requires the caller >> to set a pin path using bpf_object_pin_opts, and a global mode, where the >> path can still be overridden, but defaults to /sys/fs/bpf. This is inspired >> by the two modes supported by the iproute2 map definitions. In particular, >> it should be possible to express the current iproute2 operating mode in >> terms of the options introduced here. >> >> The new pin functions will skip any maps that do not have a pinning type >> set, unless the 'override_type' option is set, in which case all maps will >> be pinning using the pin type set in that option. This also makes it >> possible to express the old pin_maps and unpin_maps functions in terms of >> the new option-based functions. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- > > So few high-level thoughts. > > 1. I'd start with just NONE and GLOBAL as two pinning modes. It might > be worth-while to name GLOBAL something different just to specify that > it is just pinning, either to default /sys/fs/bpf root or some other > user-provided root path. > 1a. LOCAL seems to behave exactly like GLOBAL, just uses separate > option for a path. So we effectively have two GLOBAL modes, one with > default (but overrideable) /sys/fs/bpf, another with user-provided > mandatory path. The distinction seem rather small and arbitrary. > What's the use case? Supporting iproute2, mostly :) Don't terribly mind dropping LOCAL, though; I don't have any particular use case in mind for it myself. > 2. When is pin type override useful? Either specify it once > declaratively in map definition, or just do pinning programmatically? Dunno if it's really useful, actually. > 3. I think we should make pinning path override into > bpf_object_open_opts and keep bpf_object__pin_maps simple. We are > probably going to make map pinning/sharing automatic anyway, so that > will need to happen as part of either open or load operation. I actually started with just writing automatic map pinning logic for open(), but found myself re-implementing most of the logic in map_pin(). So figured I might as well expose it to that as well. For open/load I think the logic should be that we parse the pinning attribute on open and set map->pin_path from that. Then load() looks at pin_path and does the reuse/create dance. That way, an application can set its own pin_paths between open and load to support legacy formats (like iproute2 needs to). > 4. Once pinned, map knows its pinned path, just use that, I don't see > any reasonable use case where you'd want to override path just for > unpinning. Well, unpinning may need to re-construct the pin path. E.g., applications that exit after loading and are re-run after unloading, such as iproute2, probably want to be able to unpin maps. Unfortunately I don't think there is a way to get the pin path(s) of an object from the kernel, though, is there? That would be kinda neat for implementing something like `ip link set dev eth0 xdp off unpin`. > Does it make sense? > >> tools/lib/bpf/bpf_helpers.h | 8 +++ >> tools/lib/bpf/libbpf.c | 123 ++++++++++++++++++++++++++++++++++++------- >> tools/lib/bpf/libbpf.h | 33 ++++++++++++ >> tools/lib/bpf/libbpf.map | 4 + >> 4 files changed, 148 insertions(+), 20 deletions(-) >> >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h >> index 2203595f38c3..a23cf55d41b1 100644 >> --- a/tools/lib/bpf/bpf_helpers.h >> +++ b/tools/lib/bpf/bpf_helpers.h >> @@ -38,4 +38,12 @@ struct bpf_map_def { >> unsigned int map_flags; >> }; >> >> +enum libbpf_pin_type { >> + LIBBPF_PIN_NONE, >> + /* PIN_LOCAL: pin maps by name in path specified by caller */ >> + LIBBPF_PIN_LOCAL, > > Daniel mentioned in previous discussions that LOCAL mode is never > used. I'd like to avoid supporting unnecessary stuff. Is it really > useful? Oh, he did? In that case, let's definitely get rid of it :) >> + /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */ >> + LIBBPF_PIN_GLOBAL, >> +}; >> + >> #endif >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index b4fdd8ee3bbd..aea3916de341 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -226,6 +226,7 @@ struct bpf_map { >> void *priv; >> bpf_map_clear_priv_t clear_priv; >> enum libbpf_map_type libbpf_type; >> + enum libbpf_pin_type pinning; >> char *pin_path; >> }; >> >> @@ -1270,6 +1271,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, >> } >> map->def.value_size = sz; >> map->btf_value_type_id = t->type; >> + } else if (strcmp(name, "pinning") == 0) { >> + __u32 val; >> + >> + if (!get_map_field_int(map_name, obj->btf, def, m, >> + &val)) >> + return -EINVAL; >> + pr_debug("map '%s': found pinning = %u.\n", >> + map_name, val); >> + >> + if (val && val != LIBBPF_PIN_LOCAL && >> + val != LIBBPF_PIN_GLOBAL) { > > let's write out LIBBPF_PIN_NONE explicitly, instead of just `val`? OK. >> + pr_warning("map '%s': invalid pinning value %u.\n", >> + map_name, val); >> + return -EINVAL; >> + } >> + map->pinning = val; >> } else { >> if (strict) { >> pr_warning("map '%s': unknown field '%s'.\n", >> @@ -4055,10 +4072,51 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) >> return 0; >> } >> >> -int bpf_object__pin_maps(struct bpf_object *obj, const char *path) >> +static int get_pin_path(char *buf, size_t buf_len, >> + struct bpf_map *map, struct bpf_object_pin_opts *opts, >> + bool mkdir) >> +{ >> + enum libbpf_pin_type type; >> + const char *path; >> + int err, len; >> + >> + type = OPTS_GET(opts, override_type, 0) ?: map->pinning; >> + >> + if (type == LIBBPF_PIN_GLOBAL) { >> + path = OPTS_GET(opts, path_global, NULL); >> + if (!path) >> + path = "/sys/fs/bpf"; >> + } else if (type == LIBBPF_PIN_LOCAL) { >> + path = OPTS_GET(opts, path_local, NULL); >> + if (!path) { >> + pr_warning("map '%s' set pinning to PIN_LOCAL, " >> + "but no local path provided. Skipping.\n", >> + bpf_map__name(map)); >> + return 0; >> + } >> + } else { >> + return 0; >> + } >> + >> + if (mkdir) { >> + err = make_dir(path); >> + if (err) >> + return err; >> + } >> + >> + len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map)); >> + if (len < 0) >> + return -EINVAL; >> + else if (len >= buf_len) >> + return -ENAMETOOLONG; >> + return len; >> +} >> + >> +int bpf_object__pin_maps_opts(struct bpf_object *obj, >> + struct bpf_object_pin_opts *opts) >> { >> struct bpf_map *map; >> - int err; >> + int err, len; >> >> if (!obj) >> return -ENOENT; >> @@ -4068,21 +4126,17 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) >> return -ENOENT; >> } >> >> - err = make_dir(path); >> - if (err) >> - return err; >> + if (!OPTS_VALID(opts, bpf_object_pin_opts)) >> + return -EINVAL; >> >> bpf_object__for_each_map(map, obj) { >> char buf[PATH_MAX]; >> - int len; >> >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, >> - bpf_map__name(map)); >> - if (len < 0) { >> - err = -EINVAL; >> - goto err_unpin_maps; >> - } else if (len >= PATH_MAX) { >> - err = -ENAMETOOLONG; >> + len = get_pin_path(buf, PATH_MAX, map, opts, true); >> + if (len == 0) { >> + continue; >> + } else if (len < 0) { >> + err = len; >> goto err_unpin_maps; >> } >> >> @@ -4104,7 +4158,16 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) >> return err; >> } >> >> -int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) >> +int bpf_object__pin_maps(struct bpf_object *obj, const char *path) >> +{ >> + LIBBPF_OPTS(bpf_object_pin_opts, opts, >> + .path_global = path, >> + .override_type = LIBBPF_PIN_GLOBAL); > > style nit: extra line between declaration and statements > >> + return bpf_object__pin_maps_opts(obj, &opts); >> +} >> + >> +int bpf_object__unpin_maps_opts(struct bpf_object *obj, >> + struct bpf_object_pin_opts *opts) >> { >> struct bpf_map *map; >> int err; >> @@ -4112,16 +4175,18 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) >> if (!obj) >> return -ENOENT; >> >> + if (!OPTS_VALID(opts, bpf_object_pin_opts)) >> + return -EINVAL; > > specifying pin options for unpin operation looks cumbersome. We know > the pinned path, just use that and keep unpinning simple? You are right, but see above re: recreating pin paths on re-run. -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-22 18:57 ` Toke Høiland-Jørgensen @ 2019-10-23 4:40 ` Andrii Nakryiko 2019-10-23 8:53 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-23 4:40 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 11:57 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> From: Toke Høiland-Jørgensen <toke@redhat.com> > >> > >> This adds support to libbpf for setting map pinning information as part of > >> the BTF map declaration. We introduce a new pair of functions to pin and > >> unpin maps based on this setting, as well as a getter and setter function > >> for the pin information that callers can use after map load. > >> > >> The pin_type supports two modes: LOCAL pinning, which requires the caller > >> to set a pin path using bpf_object_pin_opts, and a global mode, where the > >> path can still be overridden, but defaults to /sys/fs/bpf. This is inspired > >> by the two modes supported by the iproute2 map definitions. In particular, > >> it should be possible to express the current iproute2 operating mode in > >> terms of the options introduced here. > >> > >> The new pin functions will skip any maps that do not have a pinning type > >> set, unless the 'override_type' option is set, in which case all maps will > >> be pinning using the pin type set in that option. This also makes it > >> possible to express the old pin_maps and unpin_maps functions in terms of > >> the new option-based functions. > >> > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> --- > > > > So few high-level thoughts. > > > > 1. I'd start with just NONE and GLOBAL as two pinning modes. It might > > be worth-while to name GLOBAL something different just to specify that > > it is just pinning, either to default /sys/fs/bpf root or some other > > user-provided root path. > > 1a. LOCAL seems to behave exactly like GLOBAL, just uses separate > > option for a path. So we effectively have two GLOBAL modes, one with > > default (but overrideable) /sys/fs/bpf, another with user-provided > > mandatory path. The distinction seem rather small and arbitrary. > > What's the use case? > > Supporting iproute2, mostly :) > > Don't terribly mind dropping LOCAL, though; I don't have any particular > use case in mind for it myself. > > > 2. When is pin type override useful? Either specify it once > > declaratively in map definition, or just do pinning programmatically? > > Dunno if it's really useful, actually. Ok then, let's add minimal amount of new stuff that satisfies known use cases. If we need more, thankfully, BTF-based stuff is easily extendable. > > > 3. I think we should make pinning path override into > > bpf_object_open_opts and keep bpf_object__pin_maps simple. We are > > probably going to make map pinning/sharing automatic anyway, so that > > will need to happen as part of either open or load operation. > > I actually started with just writing automatic map pinning logic for > open(), but found myself re-implementing most of the logic in map_pin(). > So figured I might as well expose it to that as well. > > For open/load I think the logic should be that we parse the pinning > attribute on open and set map->pin_path from that. Then load() looks at > pin_path and does the reuse/create dance. That way, an application can > set its own pin_paths between open and load to support legacy formats > (like iproute2 needs to). Yeah, makes sense. That's impression I got from reading the code as well. > > > 4. Once pinned, map knows its pinned path, just use that, I don't see > > any reasonable use case where you'd want to override path just for > > unpinning. > > Well, unpinning may need to re-construct the pin path. E.g., > applications that exit after loading and are re-run after unloading, > such as iproute2, probably want to be able to unpin maps. Unfortunately > I don't think there is a way to get the pin path(s) of an object from > the kernel, though, is there? That would be kinda neat for implementing > something like `ip link set dev eth0 xdp off unpin`. Hm... It seems to me that if application exits and another instance starts, it should generate pin path using the same logic, then check if map is already pinned. Then based on settings, either reuse or unpin first. Either way, pin_path needs to be calculated from map attributes, not "guessed" by application. > > > Does it make sense? > > > >> tools/lib/bpf/bpf_helpers.h | 8 +++ > >> tools/lib/bpf/libbpf.c | 123 ++++++++++++++++++++++++++++++++++++------- > >> tools/lib/bpf/libbpf.h | 33 ++++++++++++ > >> tools/lib/bpf/libbpf.map | 4 + > >> 4 files changed, 148 insertions(+), 20 deletions(-) > >> > >> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h > >> index 2203595f38c3..a23cf55d41b1 100644 > >> --- a/tools/lib/bpf/bpf_helpers.h > >> +++ b/tools/lib/bpf/bpf_helpers.h > >> @@ -38,4 +38,12 @@ struct bpf_map_def { > >> unsigned int map_flags; > >> }; > >> > >> +enum libbpf_pin_type { > >> + LIBBPF_PIN_NONE, > >> + /* PIN_LOCAL: pin maps by name in path specified by caller */ > >> + LIBBPF_PIN_LOCAL, > > > > Daniel mentioned in previous discussions that LOCAL mode is never > > used. I'd like to avoid supporting unnecessary stuff. Is it really > > useful? > > Oh, he did? In that case, let's definitely get rid of it :) > > >> + /* PIN_GLOBAL: pin maps by name in global path (/sys/fs/bpf by default) */ > >> + LIBBPF_PIN_GLOBAL, > >> +}; > >> + > >> #endif > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index b4fdd8ee3bbd..aea3916de341 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -226,6 +226,7 @@ struct bpf_map { > >> void *priv; > >> bpf_map_clear_priv_t clear_priv; > >> enum libbpf_map_type libbpf_type; > >> + enum libbpf_pin_type pinning; > >> char *pin_path; > >> }; > >> > >> @@ -1270,6 +1271,22 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj, > >> } > >> map->def.value_size = sz; > >> map->btf_value_type_id = t->type; > >> + } else if (strcmp(name, "pinning") == 0) { > >> + __u32 val; > >> + > >> + if (!get_map_field_int(map_name, obj->btf, def, m, > >> + &val)) > >> + return -EINVAL; > >> + pr_debug("map '%s': found pinning = %u.\n", > >> + map_name, val); > >> + > >> + if (val && val != LIBBPF_PIN_LOCAL && > >> + val != LIBBPF_PIN_GLOBAL) { > > > > let's write out LIBBPF_PIN_NONE explicitly, instead of just `val`? > > OK. > > >> + pr_warning("map '%s': invalid pinning value %u.\n", > >> + map_name, val); > >> + return -EINVAL; > >> + } > >> + map->pinning = val; > >> } else { > >> if (strict) { > >> pr_warning("map '%s': unknown field '%s'.\n", > >> @@ -4055,10 +4072,51 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > >> return 0; > >> } > >> > >> -int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > >> +static int get_pin_path(char *buf, size_t buf_len, > >> + struct bpf_map *map, struct bpf_object_pin_opts *opts, > >> + bool mkdir) > >> +{ > >> + enum libbpf_pin_type type; > >> + const char *path; > >> + int err, len; > >> + > >> + type = OPTS_GET(opts, override_type, 0) ?: map->pinning; > >> + > >> + if (type == LIBBPF_PIN_GLOBAL) { > >> + path = OPTS_GET(opts, path_global, NULL); > >> + if (!path) > >> + path = "/sys/fs/bpf"; > >> + } else if (type == LIBBPF_PIN_LOCAL) { > >> + path = OPTS_GET(opts, path_local, NULL); > >> + if (!path) { > >> + pr_warning("map '%s' set pinning to PIN_LOCAL, " > >> + "but no local path provided. Skipping.\n", > >> + bpf_map__name(map)); > >> + return 0; > >> + } > >> + } else { > >> + return 0; > >> + } > >> + > >> + if (mkdir) { > >> + err = make_dir(path); > >> + if (err) > >> + return err; > >> + } > >> + > >> + len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map)); > >> + if (len < 0) > >> + return -EINVAL; > >> + else if (len >= buf_len) > >> + return -ENAMETOOLONG; > >> + return len; > >> +} > >> + > >> +int bpf_object__pin_maps_opts(struct bpf_object *obj, > >> + struct bpf_object_pin_opts *opts) > >> { > >> struct bpf_map *map; > >> - int err; > >> + int err, len; > >> > >> if (!obj) > >> return -ENOENT; > >> @@ -4068,21 +4126,17 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > >> return -ENOENT; > >> } > >> > >> - err = make_dir(path); > >> - if (err) > >> - return err; > >> + if (!OPTS_VALID(opts, bpf_object_pin_opts)) > >> + return -EINVAL; > >> > >> bpf_object__for_each_map(map, obj) { > >> char buf[PATH_MAX]; > >> - int len; > >> > >> - len = snprintf(buf, PATH_MAX, "%s/%s", path, > >> - bpf_map__name(map)); > >> - if (len < 0) { > >> - err = -EINVAL; > >> - goto err_unpin_maps; > >> - } else if (len >= PATH_MAX) { > >> - err = -ENAMETOOLONG; > >> + len = get_pin_path(buf, PATH_MAX, map, opts, true); > >> + if (len == 0) { > >> + continue; > >> + } else if (len < 0) { > >> + err = len; > >> goto err_unpin_maps; > >> } > >> > >> @@ -4104,7 +4158,16 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > >> return err; > >> } > >> > >> -int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > >> +int bpf_object__pin_maps(struct bpf_object *obj, const char *path) > >> +{ > >> + LIBBPF_OPTS(bpf_object_pin_opts, opts, > >> + .path_global = path, > >> + .override_type = LIBBPF_PIN_GLOBAL); > > > > style nit: extra line between declaration and statements > > > >> + return bpf_object__pin_maps_opts(obj, &opts); > >> +} > >> + > >> +int bpf_object__unpin_maps_opts(struct bpf_object *obj, > >> + struct bpf_object_pin_opts *opts) > >> { > >> struct bpf_map *map; > >> int err; > >> @@ -4112,16 +4175,18 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > >> if (!obj) > >> return -ENOENT; > >> > >> + if (!OPTS_VALID(opts, bpf_object_pin_opts)) > >> + return -EINVAL; > > > > specifying pin options for unpin operation looks cumbersome. We know > > the pinned path, just use that and keep unpinning simple? > > You are right, but see above re: recreating pin paths on re-run.> > > -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-23 4:40 ` Andrii Nakryiko @ 2019-10-23 8:53 ` Toke Høiland-Jørgensen 2019-10-23 12:30 ` Daniel Borkmann 0 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-23 8:53 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> > 4. Once pinned, map knows its pinned path, just use that, I don't see >> > any reasonable use case where you'd want to override path just for >> > unpinning. >> >> Well, unpinning may need to re-construct the pin path. E.g., >> applications that exit after loading and are re-run after unloading, >> such as iproute2, probably want to be able to unpin maps. Unfortunately >> I don't think there is a way to get the pin path(s) of an object from >> the kernel, though, is there? That would be kinda neat for implementing >> something like `ip link set dev eth0 xdp off unpin`. > > Hm... It seems to me that if application exits and another instance > starts, it should generate pin path using the same logic, then check > if map is already pinned. Then based on settings, either reuse or > unpin first. Either way, pin_path needs to be calculated from map > attributes, not "guessed" by application. Yeah, ideally. However, the bpf object file may not be available (it's not for iproute2, for instance). I'm not sure there's really anything we *can* do about that, though, other than have the application guess. Unless we add more state to the kernel. Would it make sense to store the fact that a map was auto-pinned as a flag in the kernel map info? That way, an application could read that flag along with the name and go looking in /sys/fs/bpf. Hmm, but I guess it could do that anyway; so maybe what we need is just a "try to find all pinned maps of this program" function? That could then to something like: - Get the maps IDs and names of all maps attached to that program from the kernel. - Look for each map name in /sys/fs/bpf - If a pinned map with the same name exists, compare the IDs, and unlink if they match I don't suppose it'll be possible to do all that in a race-free manner, but that would go for any use of unlink() unless I'm missing something? -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-23 8:53 ` Toke Høiland-Jørgensen @ 2019-10-23 12:30 ` Daniel Borkmann 2019-10-23 13:05 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Daniel Borkmann @ 2019-10-23 12:30 UTC (permalink / raw) To: Toke Høiland-Jørgensen, Andrii Nakryiko Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On 10/23/19 10:53 AM, Toke Høiland-Jørgensen wrote: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > >>>> 4. Once pinned, map knows its pinned path, just use that, I don't see >>>> any reasonable use case where you'd want to override path just for >>>> unpinning. >>> >>> Well, unpinning may need to re-construct the pin path. E.g., >>> applications that exit after loading and are re-run after unloading, >>> such as iproute2, probably want to be able to unpin maps. Unfortunately >>> I don't think there is a way to get the pin path(s) of an object from >>> the kernel, though, is there? That would be kinda neat for implementing >>> something like `ip link set dev eth0 xdp off unpin`. Yep, there is no way to get the pinned path(s) of an object from the kernel, there could be multiple bpf fs mounts (note that /sys/fs/bpf is just one/default location but not limited to that) where the map is pinned, even in different mount namespaces (e.g. container A has its own private mount ns and container B as well, both independent from each other, but in both could be pinned map X under some path), there could be various hard links etc, so the only way would be to query *all* system-wide bpf fs mounts for that given map id. >> Hm... It seems to me that if application exits and another instance >> starts, it should generate pin path using the same logic, then check >> if map is already pinned. Then based on settings, either reuse or >> unpin first. Either way, pin_path needs to be calculated from map >> attributes, not "guessed" by application. > > Yeah, ideally. However, the bpf object file may not be available (it's > not for iproute2, for instance). I'm not sure there's really anything we > *can* do about that, though, other than have the application guess. > Unless we add more state to the kernel. > > Would it make sense to store the fact that a map was auto-pinned as a > flag in the kernel map info? That way, an application could read that > flag along with the name and go looking in /sys/fs/bpf. Don't think it makes sense, see above. I'm not 100% certain I'm following the use case. You are worried about the case where an application should be able to unpin the map before loading a new one so it doesn't get reused? We have similar use-case in Cilium: iproute2 bails out if map properties mismatch from the pinned map and the map in the object file. In some cases like tail call maps where they can be fully reloaded and state wouldn't need to be preserved across Cilium agents restarts, we simply move the currently active and pinned tail call map to a temp location in the BPF fs instance, reload the new program into the kernel and upon success just delete the old tail call map so kernel can recycle its resources, but in case the new program failed to load, we move the old map back to the old location thus the data path is kept alive and can be re-upgraded at a later point in time. Most of the map management in terms of cleanup or reuse is handled by the Cilium agent; iproute2 is mostly acting dumb on purpose in that it first probes whether the map name is present in BPF fs, if so, it retrieves the fd and reuses it, and it not, it creates a new one and pins it to the location. > Hmm, but I guess it could do that anyway; so maybe what we need is just > a "try to find all pinned maps of this program" function? That could > then to something like: > > - Get the maps IDs and names of all maps attached to that program from > the kernel. > > - Look for each map name in /sys/fs/bpf > > - If a pinned map with the same name exists, compare the IDs, and unlink > if they match > > I don't suppose it'll be possible to do all that in a race-free manner, > but that would go for any use of unlink() unless I'm missing something? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations 2019-10-23 12:30 ` Daniel Borkmann @ 2019-10-23 13:05 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-23 13:05 UTC (permalink / raw) To: Daniel Borkmann, Andrii Nakryiko Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf > You are worried about the case where an application should be able to > unpin the map before loading a new one so it doesn't get reused? No, I'm worried about the opposite: Someone running (the equivalent of) 'ip link set dev eth0 xdp off', and then wondering why all resources aren't freed. I do believe that the common case could be solved by a logic similar to the one I proposed, though: >> Hmm, but I guess it could do that anyway; so maybe what we need is just >> a "try to find all pinned maps of this program" function? That could >> then to something like: >> >> - Get the maps IDs and names of all maps attached to that program from >> the kernel. >> >> - Look for each map name in /sys/fs/bpf >> >> - If a pinned map with the same name exists, compare the IDs, and unlink >> if they match >> >> I don't suppose it'll be possible to do all that in a race-free manner, >> but that would go for any use of unlink() unless I'm missing something? ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning 2019-10-22 15:04 [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen 2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen 2019-10-22 15:04 ` [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations Toke Høiland-Jørgensen @ 2019-10-22 15:04 ` Toke Høiland-Jørgensen 2019-10-22 18:28 ` Andrii Nakryiko 2 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 15:04 UTC (permalink / raw) To: Daniel Borkmann Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, netdev, bpf From: Toke Høiland-Jørgensen <toke@redhat.com> While the current map pinning functions will check whether the pin path is contained on a BPF filesystem, it does not offer any options to mount the file system if it doesn't exist. Since we now have pinning options, add a new one to automount a BPF filesystem at the pinning path if that is not already pointing at a bpffs. The mounting logic itself is copied from the iproute2 BPF helper functions. Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ tools/lib/bpf/libbpf.h | 5 ++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index aea3916de341..f527224bb211 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -37,6 +37,7 @@ #include <sys/epoll.h> #include <sys/ioctl.h> #include <sys/mman.h> +#include <sys/mount.h> #include <sys/stat.h> #include <sys/types.h> #include <sys/vfs.h> @@ -4072,6 +4073,35 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) return 0; } +static int mount_bpf_fs(const char *target) +{ + bool bind_done = false; + + while (mount("", target, "none", MS_PRIVATE | MS_REC, NULL)) { + if (errno != EINVAL || bind_done) { + pr_warning("mount --make-private %s failed: %s\n", + target, strerror(errno)); + return -1; + } + + if (mount(target, target, "none", MS_BIND, NULL)) { + pr_warning("mount --bind %s %s failed: %s\n", + target, target, strerror(errno)); + return -1; + } + + bind_done = true; + } + + if (mount("bpf", target, "bpf", 0, "mode=0700")) { + fprintf(stderr, "mount -t bpf bpf %s failed: %s\n", + target, strerror(errno)); + return -1; + } + + return 0; +} + static int get_pin_path(char *buf, size_t buf_len, struct bpf_map *map, struct bpf_object_pin_opts *opts, bool mkdir) @@ -4102,6 +4132,23 @@ static int get_pin_path(char *buf, size_t buf_len, err = make_dir(path); if (err) return err; + + if (OPTS_GET(opts, mount_bpf_fs, false)) { + struct statfs st_fs; + char *cp; + + if (statfs(path, &st_fs)) { + char errmsg[STRERR_BUFSIZE]; + + cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg)); + pr_warning("failed to statfs %s: %s\n", path, cp); + return -errno; + } + if (st_fs.f_type != BPF_FS_MAGIC && + mount_bpf_fs(path)) { + return -EINVAL; + } + } } len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map)); diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 2131eeafb18d..76b9a6cc7063 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -143,8 +143,11 @@ struct bpf_object_pin_opts { * and this type used for all maps instead. */ enum libbpf_pin_type override_type; + + /* Whether to attempt to mount a BPF FS if it's not already mounted */ + bool mount_bpf_fs; }; -#define bpf_object_pin_opts__last_field override_type +#define bpf_object_pin_opts__last_field mount_bpf_fs LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning 2019-10-22 15:04 ` [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning Toke Høiland-Jørgensen @ 2019-10-22 18:28 ` Andrii Nakryiko 2019-10-22 19:04 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-22 18:28 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > From: Toke Høiland-Jørgensen <toke@redhat.com> > > While the current map pinning functions will check whether the pin path is > contained on a BPF filesystem, it does not offer any options to mount the > file system if it doesn't exist. Since we now have pinning options, add a > new one to automount a BPF filesystem at the pinning path if that is not Next thing we'll be adding extra options to mount BPF FS... Can we leave the task of auto-mounting BPF FS to tools/applications? > already pointing at a bpffs. > > The mounting logic itself is copied from the iproute2 BPF helper functions. > > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > tools/lib/bpf/libbpf.h | 5 ++++- > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index aea3916de341..f527224bb211 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -37,6 +37,7 @@ > #include <sys/epoll.h> > #include <sys/ioctl.h> > #include <sys/mman.h> > +#include <sys/mount.h> > #include <sys/stat.h> > #include <sys/types.h> > #include <sys/vfs.h> > @@ -4072,6 +4073,35 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > return 0; > } > > +static int mount_bpf_fs(const char *target) > +{ > + bool bind_done = false; > + > + while (mount("", target, "none", MS_PRIVATE | MS_REC, NULL)) { what does this loop do? we need some comments explaining what's going on here (or better yet just drop this entirely and let bpftool/iproute2 do the mounting). > + if (errno != EINVAL || bind_done) { > + pr_warning("mount --make-private %s failed: %s\n", > + target, strerror(errno)); > + return -1; > + } > + > + if (mount(target, target, "none", MS_BIND, NULL)) { > + pr_warning("mount --bind %s %s failed: %s\n", > + target, target, strerror(errno)); > + return -1; > + } > + > + bind_done = true; > + } > + > + if (mount("bpf", target, "bpf", 0, "mode=0700")) { > + fprintf(stderr, "mount -t bpf bpf %s failed: %s\n", > + target, strerror(errno)); > + return -1; > + } > + > + return 0; > +} > + > static int get_pin_path(char *buf, size_t buf_len, > struct bpf_map *map, struct bpf_object_pin_opts *opts, > bool mkdir) > @@ -4102,6 +4132,23 @@ static int get_pin_path(char *buf, size_t buf_len, Nothing in `get_pin_path` indicates that it's going to do an entire FS mount, please split this out of get_pin_path. > err = make_dir(path); > if (err) > return err; > + > + if (OPTS_GET(opts, mount_bpf_fs, false)) { > + struct statfs st_fs; > + char *cp; > + > + if (statfs(path, &st_fs)) { > + char errmsg[STRERR_BUFSIZE]; > + > + cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg)); > + pr_warning("failed to statfs %s: %s\n", path, cp); > + return -errno; > + } > + if (st_fs.f_type != BPF_FS_MAGIC && > + mount_bpf_fs(path)) { > + return -EINVAL; > + } > + } > } > > len = snprintf(buf, buf_len, "%s/%s", path, bpf_map__name(map)); > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index 2131eeafb18d..76b9a6cc7063 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -143,8 +143,11 @@ struct bpf_object_pin_opts { > * and this type used for all maps instead. > */ > enum libbpf_pin_type override_type; > + > + /* Whether to attempt to mount a BPF FS if it's not already mounted */ > + bool mount_bpf_fs; > }; > -#define bpf_object_pin_opts__last_field override_type > +#define bpf_object_pin_opts__last_field mount_bpf_fs > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning 2019-10-22 18:28 ` Andrii Nakryiko @ 2019-10-22 19:04 ` Toke Høiland-Jørgensen 2019-10-23 4:58 ` Andrii Nakryiko 0 siblings, 1 reply; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-22 19:04 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> While the current map pinning functions will check whether the pin path is >> contained on a BPF filesystem, it does not offer any options to mount the >> file system if it doesn't exist. Since we now have pinning options, add a >> new one to automount a BPF filesystem at the pinning path if that is not > > Next thing we'll be adding extra options to mount BPF FS... Can we > leave the task of auto-mounting BPF FS to tools/applications? Well, there was a reason I put this into a separate patch: I wasn't sure it really fit here. My reasoning is the following: If we end up with a default auto-pinning that works really well, people are going to just use that. And end up really confused when bpffs is not mounted. And it seems kinda silly to make every application re-implement the same mount check and logic. Or to put it another way: If we agree that the reasonable default thing is to just pin things in /sys/fs/bpf, let's make it as easy as possible for applications to do that right. >> already pointing at a bpffs. >> >> The mounting logic itself is copied from the iproute2 BPF helper functions. >> >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> >> --- >> tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ >> tools/lib/bpf/libbpf.h | 5 ++++- >> 2 files changed, 51 insertions(+), 1 deletion(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index aea3916de341..f527224bb211 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -37,6 +37,7 @@ >> #include <sys/epoll.h> >> #include <sys/ioctl.h> >> #include <sys/mman.h> >> +#include <sys/mount.h> >> #include <sys/stat.h> >> #include <sys/types.h> >> #include <sys/vfs.h> >> @@ -4072,6 +4073,35 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) >> return 0; >> } >> >> +static int mount_bpf_fs(const char *target) >> +{ >> + bool bind_done = false; >> + >> + while (mount("", target, "none", MS_PRIVATE | MS_REC, NULL)) { > > what does this loop do? we need some comments explaining what's going > on here Well, as it says in the commit message I stole this from iproute2. I think the "--make-private, --bind" dance is there to make sure we don't mess up some other mount points at this path. Which seems like a good idea, and one of those things that most people probably won't think about when just writing an application that wants to mount the fs; which is another reason to put this into libbpf :) >> + if (errno != EINVAL || bind_done) { >> + pr_warning("mount --make-private %s failed: %s\n", >> + target, strerror(errno)); >> + return -1; >> + } >> + >> + if (mount(target, target, "none", MS_BIND, NULL)) { >> + pr_warning("mount --bind %s %s failed: %s\n", >> + target, target, strerror(errno)); >> + return -1; >> + } >> + >> + bind_done = true; >> + } >> + >> + if (mount("bpf", target, "bpf", 0, "mode=0700")) { >> + fprintf(stderr, "mount -t bpf bpf %s failed: %s\n", >> + target, strerror(errno)); >> + return -1; >> + } >> + >> + return 0; >> +} >> + >> static int get_pin_path(char *buf, size_t buf_len, >> struct bpf_map *map, struct bpf_object_pin_opts *opts, >> bool mkdir) >> @@ -4102,6 +4132,23 @@ static int get_pin_path(char *buf, size_t buf_len, > > Nothing in `get_pin_path` indicates that it's going to do an entire FS > mount, please split this out of get_pin_path. Regardless of the arguments above, that is certainly a fair point ;) -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning 2019-10-22 19:04 ` Toke Høiland-Jørgensen @ 2019-10-23 4:58 ` Andrii Nakryiko 2019-10-23 8:58 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 21+ messages in thread From: Andrii Nakryiko @ 2019-10-23 4:58 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf On Tue, Oct 22, 2019 at 12:04 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> From: Toke Høiland-Jørgensen <toke@redhat.com> > >> > >> While the current map pinning functions will check whether the pin path is > >> contained on a BPF filesystem, it does not offer any options to mount the > >> file system if it doesn't exist. Since we now have pinning options, add a > >> new one to automount a BPF filesystem at the pinning path if that is not > > > > Next thing we'll be adding extra options to mount BPF FS... Can we > > leave the task of auto-mounting BPF FS to tools/applications? > > Well, there was a reason I put this into a separate patch: I wasn't sure > it really fit here. My reasoning is the following: If we end up with a > default auto-pinning that works really well, people are going to just > use that. And end up really confused when bpffs is not mounted. And it > seems kinda silly to make every application re-implement the same mount > check and logic. > > Or to put it another way: If we agree that the reasonable default thing > is to just pin things in /sys/fs/bpf, let's make it as easy as possible > for applications to do that right. > This reminds me the setrlimit() issue, though. And we decided that library shouldn't be manipulating global resources on behalf of users. I think this is a similar one. > >> already pointing at a bpffs. > >> > >> The mounting logic itself is copied from the iproute2 BPF helper functions. > >> > >> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > >> --- > >> tools/lib/bpf/libbpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ > >> tools/lib/bpf/libbpf.h | 5 ++++- > >> 2 files changed, 51 insertions(+), 1 deletion(-) > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index aea3916de341..f527224bb211 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -37,6 +37,7 @@ > >> #include <sys/epoll.h> > >> #include <sys/ioctl.h> > >> #include <sys/mman.h> > >> +#include <sys/mount.h> > >> #include <sys/stat.h> > >> #include <sys/types.h> > >> #include <sys/vfs.h> > >> @@ -4072,6 +4073,35 @@ int bpf_map__unpin(struct bpf_map *map, const char *path) > >> return 0; > >> } > >> > >> +static int mount_bpf_fs(const char *target) > >> +{ > >> + bool bind_done = false; > >> + > >> + while (mount("", target, "none", MS_PRIVATE | MS_REC, NULL)) { > > > > what does this loop do? we need some comments explaining what's going > > on here > > Well, as it says in the commit message I stole this from iproute2. I > think the "--make-private, --bind" dance is there to make sure we don't > mess up some other mount points at this path. Which seems like a good > idea, and one of those things that most people probably won't think > about when just writing an application that wants to mount the fs; which > is another reason to put this into libbpf :) I think this is exactly a reason to not do this and rely on applications to know and set up their environment properly. All these races, accidentally stomping on someone else's FS, etc, that sounds like a really good excuse to not do this in libbpf. Definitely not until we get a real experience, driven by production use cases, on how to go about that correctly. It might be added as a helper, but I think application has to call it explicitly. > > >> + if (errno != EINVAL || bind_done) { > >> + pr_warning("mount --make-private %s failed: %s\n", > >> + target, strerror(errno)); > >> + return -1; > >> + } > >> + > >> + if (mount(target, target, "none", MS_BIND, NULL)) { > >> + pr_warning("mount --bind %s %s failed: %s\n", > >> + target, target, strerror(errno)); > >> + return -1; > >> + } > >> + > >> + bind_done = true; > >> + } > >> + > >> + if (mount("bpf", target, "bpf", 0, "mode=0700")) { > >> + fprintf(stderr, "mount -t bpf bpf %s failed: %s\n", > >> + target, strerror(errno)); > >> + return -1; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static int get_pin_path(char *buf, size_t buf_len, > >> struct bpf_map *map, struct bpf_object_pin_opts *opts, > >> bool mkdir) > >> @@ -4102,6 +4132,23 @@ static int get_pin_path(char *buf, size_t buf_len, > > > > Nothing in `get_pin_path` indicates that it's going to do an entire FS > > mount, please split this out of get_pin_path. > > Regardless of the arguments above, that is certainly a fair point ;) > > -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning 2019-10-23 4:58 ` Andrii Nakryiko @ 2019-10-23 8:58 ` Toke Høiland-Jørgensen 0 siblings, 0 replies; 21+ messages in thread From: Toke Høiland-Jørgensen @ 2019-10-23 8:58 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Jesper Dangaard Brouer, David Miller, Networking, bpf Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Tue, Oct 22, 2019 at 12:04 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: >> >> > On Tue, Oct 22, 2019 at 9:08 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> >> >> From: Toke Høiland-Jørgensen <toke@redhat.com> >> >> >> >> While the current map pinning functions will check whether the pin path is >> >> contained on a BPF filesystem, it does not offer any options to mount the >> >> file system if it doesn't exist. Since we now have pinning options, add a >> >> new one to automount a BPF filesystem at the pinning path if that is not >> > >> > Next thing we'll be adding extra options to mount BPF FS... Can we >> > leave the task of auto-mounting BPF FS to tools/applications? >> >> Well, there was a reason I put this into a separate patch: I wasn't sure >> it really fit here. My reasoning is the following: If we end up with a >> default auto-pinning that works really well, people are going to just >> use that. And end up really confused when bpffs is not mounted. And it >> seems kinda silly to make every application re-implement the same mount >> check and logic. >> >> Or to put it another way: If we agree that the reasonable default thing >> is to just pin things in /sys/fs/bpf, let's make it as easy as possible >> for applications to do that right. >> > > This reminds me the setrlimit() issue, though. Heh, yeah. I personally consider the rlimit issue one of the top usability issues with BPF :/ > And we decided that library shouldn't be manipulating global resources > on behalf of users. I think this is a similar one. Hmm, that's a fair point, actually. I do get twitchy watching most applications just blindly setting rlimit to unlimited before they try to load BPF programs... I think I'll just drop this patch for now :) -Toke ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2019-10-23 13:05 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-22 15:04 [PATCH bpf-next 0/3] libbpf: Support pinning of maps using 'pinning' BTF attribute Toke Høiland-Jørgensen 2019-10-22 15:04 ` [PATCH bpf-next 1/3] libbpf: Store map pin path in struct bpf_map Toke Høiland-Jørgensen 2019-10-22 17:37 ` Andrii Nakryiko 2019-10-22 18:13 ` Toke Høiland-Jørgensen 2019-10-22 18:23 ` Andrii Nakryiko 2019-10-22 18:45 ` Toke Høiland-Jørgensen 2019-10-22 18:49 ` Andrii Nakryiko 2019-10-22 19:06 ` Toke Høiland-Jørgensen 2019-10-23 4:43 ` Andrii Nakryiko 2019-10-22 15:04 ` [PATCH bpf-next 2/3] libbpf: Support configurable pinning of maps from BTF annotations Toke Høiland-Jørgensen 2019-10-22 18:20 ` Andrii Nakryiko 2019-10-22 18:57 ` Toke Høiland-Jørgensen 2019-10-23 4:40 ` Andrii Nakryiko 2019-10-23 8:53 ` Toke Høiland-Jørgensen 2019-10-23 12:30 ` Daniel Borkmann 2019-10-23 13:05 ` Toke Høiland-Jørgensen 2019-10-22 15:04 ` [PATCH bpf-next 3/3] libbpf: Add pin option to automount BPF filesystem before pinning Toke Høiland-Jørgensen 2019-10-22 18:28 ` Andrii Nakryiko 2019-10-22 19:04 ` Toke Høiland-Jørgensen 2019-10-23 4:58 ` Andrii Nakryiko 2019-10-23 8:58 ` Toke Høiland-Jørgensen
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).