| Message ID | 20200802042127.2119901-1-yhs@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: change uapi for bpf iterator map elements | expand |
On Sat, Aug 1, 2020 at 9:22 PM Yonghong Song <yhs@fb.com> wrote: > > Previous commit adjusted kernel uapi for map > element bpf iterator. This patch adjusted libbpf API > due to uapi change. > > Signed-off-by: Yonghong Song <yhs@fb.com> > --- > tools/lib/bpf/bpf.c | 4 +++- > tools/lib/bpf/bpf.h | 5 +++-- > tools/lib/bpf/libbpf.c | 7 +++++-- > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index eab14c97c15d..c75a84398d51 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -598,7 +598,9 @@ int bpf_link_create(int prog_fd, int target_fd, > attr.link_create.prog_fd = prog_fd; > attr.link_create.target_fd = target_fd; > attr.link_create.attach_type = attach_type; > - attr.link_create.flags = OPTS_GET(opts, flags, 0); > + attr.link_create.iter_info = > + ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); > + attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); > > return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); > } > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h > index 28855fd5b5f4..c9895f191305 100644 > --- a/tools/lib/bpf/bpf.h > +++ b/tools/lib/bpf/bpf.h > @@ -170,9 +170,10 @@ LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd, > > struct bpf_link_create_opts { > size_t sz; /* size of this struct for forward/backward compatibility */ > - __u32 flags; I'd actually keep flags in link_create_ops, as it's part of the kernel UAPI anyways, we won't have to add it later. Just pass it through into bpf_attr. > + union bpf_iter_link_info *iter_info; > + __u32 iter_info_len; > }; > -#define bpf_link_create_opts__last_field flags > +#define bpf_link_create_opts__last_field iter_info_len > > LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, > enum bpf_attach_type attach_type, > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 7be04e45d29c..dc8fabf9d30d 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -8298,6 +8298,7 @@ bpf_program__attach_iter(struct bpf_program *prog, > const struct bpf_iter_attach_opts *opts) > { > DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_create_opts); > + union bpf_iter_link_info linfo; > char errmsg[STRERR_BUFSIZE]; > struct bpf_link *link; > int prog_fd, link_fd; > @@ -8307,8 +8308,10 @@ bpf_program__attach_iter(struct bpf_program *prog, > return ERR_PTR(-EINVAL); > > if (OPTS_HAS(opts, map_fd)) { > - target_fd = opts->map_fd; > - link_create_opts.flags = BPF_ITER_LINK_MAP_FD; > + memset(&linfo, 0, sizeof(linfo)); > + linfo.map.map_fd = opts->map_fd; > + link_create_opts.iter_info = &linfo; > + link_create_opts.iter_info_len = sizeof(linfo); Maybe instead of having map_fd directly in bpf_iter_attach_opts, let's just accept bpf_iter_link_info and its len directly from the user? Right now kernel UAPI and libbpf API for customizing iterator attachment differ. It would be simpler to keep them in sync and we won't have to discuss how to evolve bpf_iter_attach_opts as we add more customization for different types of iterators. Thoughts? > } > > prog_fd = bpf_program__fd(prog); > -- > 2.24.1 >
On 8/2/20 6:35 PM, Andrii Nakryiko wrote: > On Sat, Aug 1, 2020 at 9:22 PM Yonghong Song <yhs@fb.com> wrote: >> >> Previous commit adjusted kernel uapi for map >> element bpf iterator. This patch adjusted libbpf API >> due to uapi change. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> >> --- >> tools/lib/bpf/bpf.c | 4 +++- >> tools/lib/bpf/bpf.h | 5 +++-- >> tools/lib/bpf/libbpf.c | 7 +++++-- >> 3 files changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> index eab14c97c15d..c75a84398d51 100644 >> --- a/tools/lib/bpf/bpf.c >> +++ b/tools/lib/bpf/bpf.c >> @@ -598,7 +598,9 @@ int bpf_link_create(int prog_fd, int target_fd, >> attr.link_create.prog_fd = prog_fd; >> attr.link_create.target_fd = target_fd; >> attr.link_create.attach_type = attach_type; >> - attr.link_create.flags = OPTS_GET(opts, flags, 0); >> + attr.link_create.iter_info = >> + ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); >> + attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); >> >> return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); >> } >> diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h >> index 28855fd5b5f4..c9895f191305 100644 >> --- a/tools/lib/bpf/bpf.h >> +++ b/tools/lib/bpf/bpf.h >> @@ -170,9 +170,10 @@ LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd, >> >> struct bpf_link_create_opts { >> size_t sz; /* size of this struct for forward/backward compatibility */ >> - __u32 flags; > > I'd actually keep flags in link_create_ops, as it's part of the kernel > UAPI anyways, we won't have to add it later. Just pass it through into > bpf_attr. Okay. Will keep it. > >> + union bpf_iter_link_info *iter_info; >> + __u32 iter_info_len; >> }; >> -#define bpf_link_create_opts__last_field flags >> +#define bpf_link_create_opts__last_field iter_info_len >> >> LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, >> enum bpf_attach_type attach_type, >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 7be04e45d29c..dc8fabf9d30d 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -8298,6 +8298,7 @@ bpf_program__attach_iter(struct bpf_program *prog, >> const struct bpf_iter_attach_opts *opts) >> { >> DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_create_opts); >> + union bpf_iter_link_info linfo; >> char errmsg[STRERR_BUFSIZE]; >> struct bpf_link *link; >> int prog_fd, link_fd; >> @@ -8307,8 +8308,10 @@ bpf_program__attach_iter(struct bpf_program *prog, >> return ERR_PTR(-EINVAL); >> >> if (OPTS_HAS(opts, map_fd)) { >> - target_fd = opts->map_fd; >> - link_create_opts.flags = BPF_ITER_LINK_MAP_FD; >> + memset(&linfo, 0, sizeof(linfo)); >> + linfo.map.map_fd = opts->map_fd; >> + link_create_opts.iter_info = &linfo; >> + link_create_opts.iter_info_len = sizeof(linfo); > > Maybe instead of having map_fd directly in bpf_iter_attach_opts, let's > just accept bpf_iter_link_info and its len directly from the user? > Right now kernel UAPI and libbpf API for customizing iterator > attachment differ. It would be simpler to keep them in sync and we > won't have to discuss how to evolve bpf_iter_attach_opts as we add > more customization for different types of iterators. Thoughts? Good suggestion. Previously we don't have a structure to encapsulate map_fd so map_fd is added to the bpf_iter_attach_opts. Indeed, we can directly add bpf_iter_link_info to link_iter_attach_opts, and this will free libbpf from handling any future additions in bpf_iter_link_info. > >> } >> >> prog_fd = bpf_program__fd(prog); >> -- >> 2.24.1 >>
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index eab14c97c15d..c75a84398d51 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -598,7 +598,9 @@ int bpf_link_create(int prog_fd, int target_fd, attr.link_create.prog_fd = prog_fd; attr.link_create.target_fd = target_fd; attr.link_create.attach_type = attach_type; - attr.link_create.flags = OPTS_GET(opts, flags, 0); + attr.link_create.iter_info = + ptr_to_u64(OPTS_GET(opts, iter_info, (void *)0)); + attr.link_create.iter_info_len = OPTS_GET(opts, iter_info_len, 0); return sys_bpf(BPF_LINK_CREATE, &attr, sizeof(attr)); } diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h index 28855fd5b5f4..c9895f191305 100644 --- a/tools/lib/bpf/bpf.h +++ b/tools/lib/bpf/bpf.h @@ -170,9 +170,10 @@ LIBBPF_API int bpf_prog_detach2(int prog_fd, int attachable_fd, struct bpf_link_create_opts { size_t sz; /* size of this struct for forward/backward compatibility */ - __u32 flags; + union bpf_iter_link_info *iter_info; + __u32 iter_info_len; }; -#define bpf_link_create_opts__last_field flags +#define bpf_link_create_opts__last_field iter_info_len LIBBPF_API int bpf_link_create(int prog_fd, int target_fd, enum bpf_attach_type attach_type, diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 7be04e45d29c..dc8fabf9d30d 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -8298,6 +8298,7 @@ bpf_program__attach_iter(struct bpf_program *prog, const struct bpf_iter_attach_opts *opts) { DECLARE_LIBBPF_OPTS(bpf_link_create_opts, link_create_opts); + union bpf_iter_link_info linfo; char errmsg[STRERR_BUFSIZE]; struct bpf_link *link; int prog_fd, link_fd; @@ -8307,8 +8308,10 @@ bpf_program__attach_iter(struct bpf_program *prog, return ERR_PTR(-EINVAL); if (OPTS_HAS(opts, map_fd)) { - target_fd = opts->map_fd; - link_create_opts.flags = BPF_ITER_LINK_MAP_FD; + memset(&linfo, 0, sizeof(linfo)); + linfo.map.map_fd = opts->map_fd; + link_create_opts.iter_info = &linfo; + link_create_opts.iter_info_len = sizeof(linfo); } prog_fd = bpf_program__fd(prog);
Previous commit adjusted kernel uapi for map element bpf iterator. This patch adjusted libbpf API due to uapi change. Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/bpf.c | 4 +++- tools/lib/bpf/bpf.h | 5 +++-- tools/lib/bpf/libbpf.c | 7 +++++-- 3 files changed, 11 insertions(+), 5 deletions(-)