| Message ID | 20200828193603.335512-5-sdf@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Allow storage of flexible metadata information for eBPF programs | expand |
Stanislav Fomichev <sdf@google.com> writes: > This is a low-level function (hence in bpf.c) to find out the metadata > map id for the provided program fd. > It will be used in the next commits from bpftool. > > Cc: Toke Høiland-Jørgensen <toke@redhat.com> > Cc: YiFei Zhu <zhuyifei1999@gmail.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > tools/lib/bpf/bpf.c | 74 ++++++++++++++++++++++++++++++++++++++++ > tools/lib/bpf/bpf.h | 1 + > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 76 insertions(+) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index 5f6c5676cc45..01c0ede1625d 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd, > > return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr)); > } > + > +int bpf_prog_find_metadata(int prog_fd) > +{ > + struct bpf_prog_info prog_info = {}; > + struct bpf_map_info map_info; > + __u32 prog_info_len; > + __u32 map_info_len; > + int saved_errno; > + __u32 *map_ids; > + int nr_maps; > + int map_fd; > + int ret; > + int i; > + > + prog_info_len = sizeof(prog_info); > + > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); > + if (ret) > + return ret; > + > + if (!prog_info.nr_map_ids) > + return -1; > + > + map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32)); > + if (!map_ids) > + return -1; > + > + nr_maps = prog_info.nr_map_ids; > + memset(&prog_info, 0, sizeof(prog_info)); > + prog_info.nr_map_ids = nr_maps; > + prog_info.map_ids = ptr_to_u64(map_ids); > + prog_info_len = sizeof(prog_info); > + > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); > + if (ret) > + goto free_map_ids; > + > + ret = -1; > + for (i = 0; i < prog_info.nr_map_ids; i++) { > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > + if (map_fd < 0) { > + ret = -1; > + goto free_map_ids; > + } > + > + memset(&map_info, 0, sizeof(map_info)); > + map_info_len = sizeof(map_info); > + ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); > + saved_errno = errno; > + close(map_fd); > + errno = saved_errno; > + if (ret) > + goto free_map_ids; If you get to this point on the last entry in the loop, ret will be 0, and any of the continue statements below will end the loop, causing the whole function to return 0. While this is not technically a valid ID, it still seems odd that the function returns -1 on all error conditions except this one. Also, it would be good to be able to unambiguously distinguish between "this program has no metadata associated" and "something went wrong while querying the kernel for metadata (e.g., permission error)". So something that amounts to a -ENOENT return; I guess turning all return values into negative error codes would do that (and also do away with the need for the saved_errno dance above), but id does clash a bit with the convention in the rest of the file (where all the other functions just return -1 and set errno)... -Toke
On 08/28, Toke H�iland-J�rgensen wrote: > Stanislav Fomichev <sdf@google.com> writes: > > This is a low-level function (hence in bpf.c) to find out the metadata > > map id for the provided program fd. > > It will be used in the next commits from bpftool. > > > > Cc: Toke H�iland-J�rgensen <toke@redhat.com> > > Cc: YiFei Zhu <zhuyifei1999@gmail.com> > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > tools/lib/bpf/bpf.c | 74 ++++++++++++++++++++++++++++++++++++++++ > > tools/lib/bpf/bpf.h | 1 + > > tools/lib/bpf/libbpf.map | 1 + > > 3 files changed, 76 insertions(+) > > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > > index 5f6c5676cc45..01c0ede1625d 100644 > > --- a/tools/lib/bpf/bpf.c > > +++ b/tools/lib/bpf/bpf.c > > @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd, > > > > return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr)); > > } > > + > > +int bpf_prog_find_metadata(int prog_fd) > > +{ > > + struct bpf_prog_info prog_info = {}; > > + struct bpf_map_info map_info; > > + __u32 prog_info_len; > > + __u32 map_info_len; > > + int saved_errno; > > + __u32 *map_ids; > > + int nr_maps; > > + int map_fd; > > + int ret; > > + int i; > > + > > + prog_info_len = sizeof(prog_info); > > + > > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); > > + if (ret) > > + return ret; > > + > > + if (!prog_info.nr_map_ids) > > + return -1; > > + > > + map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32)); > > + if (!map_ids) > > + return -1; > > + > > + nr_maps = prog_info.nr_map_ids; > > + memset(&prog_info, 0, sizeof(prog_info)); > > + prog_info.nr_map_ids = nr_maps; > > + prog_info.map_ids = ptr_to_u64(map_ids); > > + prog_info_len = sizeof(prog_info); > > + > > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); > > + if (ret) > > + goto free_map_ids; > > + > > + ret = -1; > > + for (i = 0; i < prog_info.nr_map_ids; i++) { > > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > > + if (map_fd < 0) { > > + ret = -1; > > + goto free_map_ids; > > + } > > + > > + memset(&map_info, 0, sizeof(map_info)); > > + map_info_len = sizeof(map_info); > > + ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); > > + saved_errno = errno; > > + close(map_fd); > > + errno = saved_errno; > > + if (ret) > > + goto free_map_ids; > If you get to this point on the last entry in the loop, ret will be 0, > and any of the continue statements below will end the loop, causing the > whole function to return 0. While this is not technically a valid ID, it > still seems odd that the function returns -1 on all error conditions > except this one. > Also, it would be good to be able to unambiguously distinguish between > "this program has no metadata associated" and "something went wrong > while querying the kernel for metadata (e.g., permission error)". So > something that amounts to a -ENOENT return; I guess turning all return > values into negative error codes would do that (and also do away with > the need for the saved_errno dance above), but id does clash a bit with > the convention in the rest of the file (where all the other functions > just return -1 and set errno)... Good point. I think I can change the function signature to: int bpf_prog_find_metadata(int prog_fd, int *map_id) And explicitly return map_id via argument. Then the ret can be used as -1/0 error and I can set errno appropriately where it makes sense. This will better match the convention we have in this file. Will respin v4 later this week to give people opportunity to look at the other patches in the series. Thanks!
On Mon, Aug 31, 2020 at 08:40:01AM -0700, sdf@google.com wrote: > On 08/28, Toke H�iland-J�rgensen wrote: > > Stanislav Fomichev <sdf@google.com> writes: > > > > This is a low-level function (hence in bpf.c) to find out the metadata > > > map id for the provided program fd. > > > It will be used in the next commits from bpftool. > > > > > > Cc: Toke H�iland-J�rgensen <toke@redhat.com> > > > Cc: YiFei Zhu <zhuyifei1999@gmail.com> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > tools/lib/bpf/bpf.c | 74 ++++++++++++++++++++++++++++++++++++++++ > > > tools/lib/bpf/bpf.h | 1 + > > > tools/lib/bpf/libbpf.map | 1 + > > > 3 files changed, 76 insertions(+) > > > > > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > > > index 5f6c5676cc45..01c0ede1625d 100644 > > > --- a/tools/lib/bpf/bpf.c > > > +++ b/tools/lib/bpf/bpf.c > > > @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd, > > > > > > return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr)); > > > } > > > + > > > +int bpf_prog_find_metadata(int prog_fd) > > > +{ > > > + struct bpf_prog_info prog_info = {}; > > > + struct bpf_map_info map_info; > > > + __u32 prog_info_len; > > > + __u32 map_info_len; > > > + int saved_errno; > > > + __u32 *map_ids; > > > + int nr_maps; > > > + int map_fd; > > > + int ret; > > > + int i; > > > + > > > + prog_info_len = sizeof(prog_info); > > > + > > > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); > > > + if (ret) > > > + return ret; > > > + > > > + if (!prog_info.nr_map_ids) > > > + return -1; > > > + > > > + map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32)); > > > + if (!map_ids) > > > + return -1; > > > + > > > + nr_maps = prog_info.nr_map_ids; > > > + memset(&prog_info, 0, sizeof(prog_info)); > > > + prog_info.nr_map_ids = nr_maps; > > > + prog_info.map_ids = ptr_to_u64(map_ids); > > > + prog_info_len = sizeof(prog_info); > > > + > > > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); > > > + if (ret) > > > + goto free_map_ids; > > > + > > > + ret = -1; > > > + for (i = 0; i < prog_info.nr_map_ids; i++) { > > > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); > > > + if (map_fd < 0) { > > > + ret = -1; > > > + goto free_map_ids; > > > + } > > > + > > > + memset(&map_info, 0, sizeof(map_info)); > > > + map_info_len = sizeof(map_info); > > > + ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); > > > + saved_errno = errno; > > > + close(map_fd); > > > + errno = saved_errno; > > > + if (ret) > > > + goto free_map_ids; > > > If you get to this point on the last entry in the loop, ret will be 0, > > and any of the continue statements below will end the loop, causing the > > whole function to return 0. While this is not technically a valid ID, it > > still seems odd that the function returns -1 on all error conditions > > except this one. > > > Also, it would be good to be able to unambiguously distinguish between > > "this program has no metadata associated" and "something went wrong > > while querying the kernel for metadata (e.g., permission error)". So > > something that amounts to a -ENOENT return; I guess turning all return > > values into negative error codes would do that (and also do away with > > the need for the saved_errno dance above), but id does clash a bit with > > the convention in the rest of the file (where all the other functions > > just return -1 and set errno)... > Good point. I think I can change the function signature to: > > int bpf_prog_find_metadata(int prog_fd, int *map_id) > > And explicitly return map_id via argument. Then the ret can be used as > -1/0 error and I can set errno appropriately where it makes sense. > This will better match the convention we have in this file. I don't feel great about this libbpf api. bpftool already does bpf_obj_get_info_by_fd() for progs and for maps. This extra step and extra set of syscalls is redundant work. I think it's better to be done as part of bpftool. It doesn't quite fit as generic api.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Mon, Aug 31, 2020 at 08:40:01AM -0700, sdf@google.com wrote: >> On 08/28, Toke H�iland-J�rgensen wrote: >> > Stanislav Fomichev <sdf@google.com> writes: >> >> > > This is a low-level function (hence in bpf.c) to find out the metadata >> > > map id for the provided program fd. >> > > It will be used in the next commits from bpftool. >> > > >> > > Cc: Toke H�iland-J�rgensen <toke@redhat.com> >> > > Cc: YiFei Zhu <zhuyifei1999@gmail.com> >> > > Signed-off-by: Stanislav Fomichev <sdf@google.com> >> > > --- >> > > tools/lib/bpf/bpf.c | 74 ++++++++++++++++++++++++++++++++++++++++ >> > > tools/lib/bpf/bpf.h | 1 + >> > > tools/lib/bpf/libbpf.map | 1 + >> > > 3 files changed, 76 insertions(+) >> > > >> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> > > index 5f6c5676cc45..01c0ede1625d 100644 >> > > --- a/tools/lib/bpf/bpf.c >> > > +++ b/tools/lib/bpf/bpf.c >> > > @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd, >> > > >> > > return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr)); >> > > } >> > > + >> > > +int bpf_prog_find_metadata(int prog_fd) >> > > +{ >> > > + struct bpf_prog_info prog_info = {}; >> > > + struct bpf_map_info map_info; >> > > + __u32 prog_info_len; >> > > + __u32 map_info_len; >> > > + int saved_errno; >> > > + __u32 *map_ids; >> > > + int nr_maps; >> > > + int map_fd; >> > > + int ret; >> > > + int i; >> > > + >> > > + prog_info_len = sizeof(prog_info); >> > > + >> > > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); >> > > + if (ret) >> > > + return ret; >> > > + >> > > + if (!prog_info.nr_map_ids) >> > > + return -1; >> > > + >> > > + map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32)); >> > > + if (!map_ids) >> > > + return -1; >> > > + >> > > + nr_maps = prog_info.nr_map_ids; >> > > + memset(&prog_info, 0, sizeof(prog_info)); >> > > + prog_info.nr_map_ids = nr_maps; >> > > + prog_info.map_ids = ptr_to_u64(map_ids); >> > > + prog_info_len = sizeof(prog_info); >> > > + >> > > + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); >> > > + if (ret) >> > > + goto free_map_ids; >> > > + >> > > + ret = -1; >> > > + for (i = 0; i < prog_info.nr_map_ids; i++) { >> > > + map_fd = bpf_map_get_fd_by_id(map_ids[i]); >> > > + if (map_fd < 0) { >> > > + ret = -1; >> > > + goto free_map_ids; >> > > + } >> > > + >> > > + memset(&map_info, 0, sizeof(map_info)); >> > > + map_info_len = sizeof(map_info); >> > > + ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); >> > > + saved_errno = errno; >> > > + close(map_fd); >> > > + errno = saved_errno; >> > > + if (ret) >> > > + goto free_map_ids; >> >> > If you get to this point on the last entry in the loop, ret will be 0, >> > and any of the continue statements below will end the loop, causing the >> > whole function to return 0. While this is not technically a valid ID, it >> > still seems odd that the function returns -1 on all error conditions >> > except this one. >> >> > Also, it would be good to be able to unambiguously distinguish between >> > "this program has no metadata associated" and "something went wrong >> > while querying the kernel for metadata (e.g., permission error)". So >> > something that amounts to a -ENOENT return; I guess turning all return >> > values into negative error codes would do that (and also do away with >> > the need for the saved_errno dance above), but id does clash a bit with >> > the convention in the rest of the file (where all the other functions >> > just return -1 and set errno)... >> Good point. I think I can change the function signature to: >> >> int bpf_prog_find_metadata(int prog_fd, int *map_id) >> >> And explicitly return map_id via argument. Then the ret can be used as >> -1/0 error and I can set errno appropriately where it makes sense. >> This will better match the convention we have in this file. > > I don't feel great about this libbpf api. bpftool already does > bpf_obj_get_info_by_fd() for progs and for maps. > This extra step and extra set of syscalls is redundant work. > I think it's better to be done as part of bpftool. > It doesn't quite fit as generic api. Why not? We are establishing a convention for how to store (and read) metadata from a program; by having an API to get this, we make sure that every application that wants to access this metadata agrees on how to do so. If we don't have it, people will have to go look at bpftool code, and we'll end up with copied code snippets, which seems less than ideal. -Toke
On Wed, Sep 02, 2020 at 11:43:26AM +0200, Toke Høiland-Jørgensen wrote: > > > > I don't feel great about this libbpf api. bpftool already does > > bpf_obj_get_info_by_fd() for progs and for maps. > > This extra step and extra set of syscalls is redundant work. > > I think it's better to be done as part of bpftool. > > It doesn't quite fit as generic api. > > Why not? It's a helper function on top of already provided api and implemented in the most brute force and inefficient way. bpftool implementation of the same will be more efficient. > so. If we don't have it, people will have to go look at bpftool code, > and we'll end up with copied code snippets, which seems less than ideal. I'd like to see the real use case first before hypothesising.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Wed, Sep 02, 2020 at 11:43:26AM +0200, Toke Høiland-Jørgensen wrote: >> > >> > I don't feel great about this libbpf api. bpftool already does >> > bpf_obj_get_info_by_fd() for progs and for maps. >> > This extra step and extra set of syscalls is redundant work. >> > I think it's better to be done as part of bpftool. >> > It doesn't quite fit as generic api. >> >> Why not? > > It's a helper function on top of already provided api and implemented > in the most brute force and inefficient way. > bpftool implementation of the same will be more efficient. Right, certainly wouldn't mind something more efficient. But to me, the inefficiency is outweighed by convenience of having this canonical reference for 'this is the metadata map'. >> so. If we don't have it, people will have to go look at bpftool code, >> and we'll end up with copied code snippets, which seems less than ideal. > > I'd like to see the real use case first before hypothesising. For me, that would be incorporating support for this into libxdp/xdp-tools; which was the reason I asked for this to be split into a separate API in the first place. But okay, not going to keep arguing about this, I can copy-paste code as well as the next person. -Toke
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index 5f6c5676cc45..01c0ede1625d 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd, return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr)); } + +int bpf_prog_find_metadata(int prog_fd) +{ + struct bpf_prog_info prog_info = {}; + struct bpf_map_info map_info; + __u32 prog_info_len; + __u32 map_info_len; + int saved_errno; + __u32 *map_ids; + int nr_maps; + int map_fd; + int ret; + int i; + + prog_info_len = sizeof(prog_info); + + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); + if (ret) + return ret; + + if (!prog_info.nr_map_ids) + return -1; + + map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32)); + if (!map_ids) + return -1; + + nr_maps = prog_info.nr_map_ids; + memset(&prog_info, 0, sizeof(prog_info)); + prog_info.nr_map_ids = nr_maps; + prog_info.map_ids = ptr_to_u64(map_ids); + prog_info_len = sizeof(prog_info); + + ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len); + if (ret) + goto free_map_ids; + + ret = -1; + for (i = 0; i < prog_info.nr_map_ids; i++) { + map_fd = bpf_map_get_fd_by_id(map_ids[i]); + if (map_fd < 0) { + ret = -1; + goto free_map_ids; + } + + memset(&map_info, 0, sizeof(map_info)); + map_info_len = sizeof(map_info); + ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len); + saved_errno = errno; + close(map_fd); + errno = saved_errno; + if (ret) + goto free_map_ids; + + if (map_info.type != BPF_MAP_TYPE_ARRAY) + continue; + if (map_info.key_size != sizeof(int)) + continue; + if (map_info.max_entries != 1) + continue; + if (!map_info.btf_value_type_id) + continue; + if (!strstr(map_info.name, ".metadata")) + continue; + + ret = map_ids[i]; + break; + } + + +free_map_ids: + free(map_ids); + return ret; +} diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h index 8c1ac4b42f90..8982ffa7cfd2 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -251,6 +251,7 @@ struct bpf_prog_bind_opts { LIBBPF_API int bpf_prog_bind_map(int prog_fd, int map_fd, const struct bpf_prog_bind_opts *opts); +LIBBPF_API int bpf_prog_find_metadata(int prog_fd); #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 529b99c0c2c3..b7a40f543b2b 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -307,4 +307,5 @@ LIBBPF_0.2.0 { perf_buffer__buffer_fd; perf_buffer__epoll_fd; perf_buffer__consume_buffer; + bpf_prog_find_metadata; } LIBBPF_0.1.0;
This is a low-level function (hence in bpf.c) to find out the metadata map id for the provided program fd. It will be used in the next commits from bpftool. Cc: Toke Høiland-Jørgensen <toke@redhat.com> Cc: YiFei Zhu <zhuyifei1999@gmail.com> Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/lib/bpf/bpf.c | 74 ++++++++++++++++++++++++++++++++++++++++ tools/lib/bpf/bpf.h | 1 + tools/lib/bpf/libbpf.map | 1 + 3 files changed, 76 insertions(+)