| Message ID | 157175668879.112621.10917994557478417780.stgit@toke.dk |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | libbpf: Support pinning of maps using 'pinning' BTF attribute | expand |
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); >
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
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
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
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
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
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
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);