| Message ID | 20200929031845.751054-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [bpf-next] libbpf: export bpf_object__reuse_map() to libbpf api | expand |
On Mon, Sep 28, 2020 at 8:20 PM Hangbin Liu <liuhangbin@gmail.com> wrote: > > Besides bpf_map__reuse_fd(), which could let us reuse existing map fd. > bpf_object__reuse_map() could let us reuse existing pinned maps, which > is helpful. > > This functions could also be used when we add iproute2 libbpf support, > so we don't need to re-use or re-implement new functions like > bpf_obj_get()/bpf_map_selfcheck_pinned() in iproute2. > > Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> > --- > tools/lib/bpf/libbpf.c | 3 +-- > tools/lib/bpf/libbpf.h | 1 + > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 32dc444224d8..e835d7a3437f 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4033,8 +4033,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd) > map_info.map_flags == map->def.map_flags); > } > > -static int > -bpf_object__reuse_map(struct bpf_map *map) > +int bpf_object__reuse_map(struct bpf_map *map) > { > char *cp, errmsg[STRERR_BUFSIZE]; > int err, pin_fd; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index a750f67a23f6..4b9e615eb393 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -431,6 +431,7 @@ bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); > /* get/set map FD */ > LIBBPF_API int bpf_map__fd(const struct bpf_map *map); > LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); > +LIBBPF_API int bpf_object__reuse_map(struct bpf_map *map); It's internal function, which doesn't check that map->pin_path is set, for one thing. It shouldn't be exposed. libbpf exposes bpf_map__set_pin_path() to set pin_path for any map, and then during load time libbpf with "reuse map", if pin_path is not NULL. Doesn't that work for you? > /* get map definition */ > LIBBPF_API const struct bpf_map_def *bpf_map__def(const struct bpf_map *map); > /* get map name */ > -- > 2.25.4 >
On Mon, Sep 28, 2020 at 08:30:42PM -0700, Andrii Nakryiko wrote: > > @@ -431,6 +431,7 @@ bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); > > /* get/set map FD */ > > LIBBPF_API int bpf_map__fd(const struct bpf_map *map); > > LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); > > +LIBBPF_API int bpf_object__reuse_map(struct bpf_map *map); > > It's internal function, which doesn't check that map->pin_path is set, How about add a path check in bpf_object__reuse_map()? And off course users who use it should call bpf_map__set_pin_path() first. > for one thing. It shouldn't be exposed. libbpf exposes > bpf_map__set_pin_path() to set pin_path for any map, and then during > load time libbpf with "reuse map", if pin_path is not NULL. Doesn't > that work for you? Long story... When trying to add iproute2 libbpf support that I'm working on. We need to create iproute2 legacy map-in-map manually before libbpf load objects, because libbpf only support BTF type map-in-map(unless I missed something.). After creating legacy map-in-map and reuse the fd, if the map has legacy pining defined, only set the pin path would not enough as libbpf will skip pinning map if map->fd > 0 in bpf_object__create_maps(). See the following code bellow. bpf_map__set_pin_path() bpf_create_map_in_map() <- create inner or outer map bpf_map__reuse_fd(map, inner/outer_fd) bpf_object__load(obj) - bpf_object__load_xattr() - bpf_object__create_maps() - if (map->fd >= 0) continue <- this will skip pinning map So when handle legacy map-in-map + pin map, we need to create the map and pin maps manually at the same time. The code would looks like (err handler skipped). map_fd = bpf_obj_get(pathname); if (map_fd > 0) { bpf_map__set_pin_path(map, pathname); return bpf_object__reuse_map(map); <- here we need the reuse_map } bpf_create_map_in_map() bpf_map__reuse_fd(map, map_fd); bpf_map__pin(map, pathname); So I think this function is needed, what do you think? Thanks Hangbin
On Tue, Sep 29, 2020 at 2:42 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > On Mon, Sep 28, 2020 at 08:30:42PM -0700, Andrii Nakryiko wrote: > > > @@ -431,6 +431,7 @@ bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); > > > /* get/set map FD */ > > > LIBBPF_API int bpf_map__fd(const struct bpf_map *map); > > > LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); > > > +LIBBPF_API int bpf_object__reuse_map(struct bpf_map *map); > > > > It's internal function, which doesn't check that map->pin_path is set, > > How about add a path check in bpf_object__reuse_map()? > > And off course users who use it should call bpf_map__set_pin_path() first. > > > for one thing. It shouldn't be exposed. libbpf exposes > > bpf_map__set_pin_path() to set pin_path for any map, and then during > > load time libbpf with "reuse map", if pin_path is not NULL. Doesn't > > that work for you? > > Long story... > > When trying to add iproute2 libbpf support that I'm working on. We need to > create iproute2 legacy map-in-map manually before libbpf load objects, because > libbpf only support BTF type map-in-map(unless I missed something.). > > After creating legacy map-in-map and reuse the fd, if the map has legacy > pining defined, only set the pin path would not enough as libbpf will skip > pinning map if map->fd > 0 in bpf_object__create_maps(). See the following > code bellow. > > bpf_map__set_pin_path() > bpf_create_map_in_map() <- create inner or outer map > bpf_map__reuse_fd(map, inner/outer_fd) > bpf_object__load(obj) > - bpf_object__load_xattr() > - bpf_object__create_maps() > - if (map->fd >= 0) > continue <- this will skip pinning map so maybe that's the part that needs to be fixed?.. > > So when handle legacy map-in-map + pin map, we need to create the map > and pin maps manually at the same time. The code would looks like > (err handler skipped). > > map_fd = bpf_obj_get(pathname); > if (map_fd > 0) { > bpf_map__set_pin_path(map, pathname); > return bpf_object__reuse_map(map); <- here we need the reuse_map > } > bpf_create_map_in_map() > bpf_map__reuse_fd(map, map_fd); > bpf_map__pin(map, pathname); > > So I think this function is needed, what do you think? I'm still not sure. And to be honest your examples are still a bit too succinct for me to follow where the problem is exactly. Can you please elaborate a bit more? It might very well be that map pinning and FD reuse have buggy and convoluted logic, but let's try to fix that first, before we expose new APIs. > > Thanks > Hangbin
On Tue, Sep 29, 2020 at 04:03:45PM -0700, Andrii Nakryiko wrote: > > bpf_map__set_pin_path() > > bpf_create_map_in_map() <- create inner or outer map > > bpf_map__reuse_fd(map, inner/outer_fd) > > bpf_object__load(obj) > > - bpf_object__load_xattr() > > - bpf_object__create_maps() > > - if (map->fd >= 0) > > continue <- this will skip pinning map > > so maybe that's the part that needs to be fixed?.. Hmm...maybe, let me see > > I'm still not sure. And to be honest your examples are still a bit too > succinct for me to follow where the problem is exactly. Can you please > elaborate a bit more? Let's take iproute2 legacy map for example, if it's a map-in-map type with pin path defined. In user space we could do like: if (bpf_obj_get(pathname) < 0) { bpf_create_map_in_map(); bpf_map__reuse_fd(map, map_fd); } bpf_map__set_pin_path(map, pathname); bpf_object__load(obj) So in libbpf we need diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 32dc444224d8..5412aa7169db 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4215,7 +4215,7 @@ bpf_object__create_maps(struct bpf_object *obj) if (map->fd >= 0) { pr_debug("map '%s': skipping creation (preset fd=%d)\n", map->name, map->fd); - continue; + goto check_pin_path; } err = bpf_object__create_map(obj, map); @@ -4258,6 +4258,7 @@ bpf_object__create_maps(struct bpf_object *obj) map->init_slots_sz = 0; } +check_pin_path: if (map->pin_path && !map->pinned) { err = bpf_map__pin(map, NULL); if (err) { Do you think if this change be better? Thanks Hangbin
On Tue, Sep 29, 2020 at 7:34 PM Hangbin Liu <liuhangbin@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 04:03:45PM -0700, Andrii Nakryiko wrote: > > > bpf_map__set_pin_path() > > > bpf_create_map_in_map() <- create inner or outer map > > > bpf_map__reuse_fd(map, inner/outer_fd) > > > bpf_object__load(obj) > > > - bpf_object__load_xattr() > > > - bpf_object__create_maps() > > > - if (map->fd >= 0) > > > continue <- this will skip pinning map > > > > so maybe that's the part that needs to be fixed?.. > > Hmm...maybe, let me see > > > > > I'm still not sure. And to be honest your examples are still a bit too > > succinct for me to follow where the problem is exactly. Can you please > > elaborate a bit more? > > Let's take iproute2 legacy map for example, if it's a map-in-map type with > pin path defined. In user space we could do like: > > if (bpf_obj_get(pathname) < 0) { > bpf_create_map_in_map(); > bpf_map__reuse_fd(map, map_fd); > } > bpf_map__set_pin_path(map, pathname); > bpf_object__load(obj) > > So in libbpf we need > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 32dc444224d8..5412aa7169db 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4215,7 +4215,7 @@ bpf_object__create_maps(struct bpf_object *obj) > if (map->fd >= 0) { > pr_debug("map '%s': skipping creation (preset fd=%d)\n", > map->name, map->fd); > - continue; > + goto check_pin_path; > } > > err = bpf_object__create_map(obj, map); > @@ -4258,6 +4258,7 @@ bpf_object__create_maps(struct bpf_object *obj) > map->init_slots_sz = 0; > } > > +check_pin_path: > if (map->pin_path && !map->pinned) { > err = bpf_map__pin(map, NULL); > if (err) { > > > Do you think if this change be better? Yes, of course. Just don't do it through use of goto. Guard map creation with that if instead. > > Thanks > Hangbin
On Thu, 1 Oct 2020 at 02:30, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 32dc444224d8..5412aa7169db 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -4215,7 +4215,7 @@ bpf_object__create_maps(struct bpf_object *obj) > > if (map->fd >= 0) { > > pr_debug("map '%s': skipping creation (preset fd=%d)\n", > > map->name, map->fd); > > - continue; > > + goto check_pin_path; > > } > > > > err = bpf_object__create_map(obj, map); > > @@ -4258,6 +4258,7 @@ bpf_object__create_maps(struct bpf_object *obj) > > map->init_slots_sz = 0; > > } > > > > +check_pin_path: > > if (map->pin_path && !map->pinned) { > > err = bpf_map__pin(map, NULL); > > if (err) { > > > > > > Do you think if this change be better? > > Yes, of course. Just don't do it through use of goto. Guard map > creation with that if instead. Hi Andrii, Looks I missed something, Would you like to explain why we should not use goto? And for "guard map creation with the if", do you mean duplicate the if (map->pin_path && !map->pinned) in if (map->fd >= 0)? like diff --git a/src/libbpf.c b/src/libbpf.c index 3df1f4d..705abcb 100644 --- a/src/libbpf.c +++ b/src/libbpf.c @@ -4215,6 +4215,15 @@ bpf_object__create_maps(struct bpf_object *obj) if (map->fd >= 0) { pr_debug("map '%s': skipping creation (preset fd=%d)\n", map->name, map->fd); + if (map->pin_path && !map->pinned) { + err = bpf_map__pin(map, NULL); + if (err) { + pr_warn("map '%s': failed to auto-pin at '%s': %d\n", + map->name, map->pin_path, err); + zclose(map->fd); + goto err_out; + } + } continue; } (Sorry if the code format got corrupted as I replied in web gmail....) Thanks Hangbin
On Thu, Oct 1, 2020 at 4:34 AM Hangbin Liu <liuhangbin@gmail.com> wrote: > > On Thu, 1 Oct 2020 at 02:30, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index 32dc444224d8..5412aa7169db 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -4215,7 +4215,7 @@ bpf_object__create_maps(struct bpf_object *obj) > > > if (map->fd >= 0) { > > > pr_debug("map '%s': skipping creation (preset fd=%d)\n", > > > map->name, map->fd); > > > - continue; > > > + goto check_pin_path; > > > } > > > > > > err = bpf_object__create_map(obj, map); > > > @@ -4258,6 +4258,7 @@ bpf_object__create_maps(struct bpf_object *obj) > > > map->init_slots_sz = 0; > > > } > > > > > > +check_pin_path: > > > if (map->pin_path && !map->pinned) { > > > err = bpf_map__pin(map, NULL); > > > if (err) { > > > > > > > > > Do you think if this change be better? > > > > Yes, of course. Just don't do it through use of goto. Guard map > > creation with that if instead. > > Hi Andrii, > > Looks I missed something, Would you like to explain why we should not use goto? Because goto shouldn't be a default way of altering the control flow. > And for "guard map creation with the if", do you mean duplicate the > if (map->pin_path && !map->pinned) in if (map->fd >= 0)? like I mean something like: if (map->pin_path) { ... } if (map fd < 0) { bpf_object__create_map(..); if (bpf_map__is_internal(..)) { ... } if (map->init_slot_sz) { ...} } if (map->pin_path && !map->pinned) { ... } > > diff --git a/src/libbpf.c b/src/libbpf.c > index 3df1f4d..705abcb 100644 > --- a/src/libbpf.c > +++ b/src/libbpf.c > @@ -4215,6 +4215,15 @@ bpf_object__create_maps(struct bpf_object *obj) > if (map->fd >= 0) { > pr_debug("map '%s': skipping creation (preset fd=%d)\n", > map->name, map->fd); > + if (map->pin_path && !map->pinned) { > + err = bpf_map__pin(map, NULL); > + if (err) { > + pr_warn("map '%s': failed to > auto-pin at '%s': %d\n", > + map->name, map->pin_path, err); > + zclose(map->fd); > + goto err_out; > + } > + } > continue; > } > > (Sorry if the code format got corrupted as I replied in web gmail....) > > Thanks > Hangbin
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 32dc444224d8..e835d7a3437f 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4033,8 +4033,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd) map_info.map_flags == map->def.map_flags); } -static int -bpf_object__reuse_map(struct bpf_map *map) +int bpf_object__reuse_map(struct bpf_map *map) { char *cp, errmsg[STRERR_BUFSIZE]; int err, pin_fd; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index a750f67a23f6..4b9e615eb393 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -431,6 +431,7 @@ bpf_map__prev(const struct bpf_map *map, const struct bpf_object *obj); /* get/set map FD */ LIBBPF_API int bpf_map__fd(const struct bpf_map *map); LIBBPF_API int bpf_map__reuse_fd(struct bpf_map *map, int fd); +LIBBPF_API int bpf_object__reuse_map(struct bpf_map *map); /* get map definition */ LIBBPF_API const struct bpf_map_def *bpf_map__def(const struct bpf_map *map); /* get map name */
Besides bpf_map__reuse_fd(), which could let us reuse existing map fd. bpf_object__reuse_map() could let us reuse existing pinned maps, which is helpful. This functions could also be used when we add iproute2 libbpf support, so we don't need to re-use or re-implement new functions like bpf_obj_get()/bpf_map_selfcheck_pinned() in iproute2. Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- tools/lib/bpf/libbpf.c | 3 +-- tools/lib/bpf/libbpf.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)