* [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr @ 2019-09-27 13:08 KP Singh 2019-09-27 16:06 ` Andrii Nakryiko 0 siblings, 1 reply; 5+ messages in thread From: KP Singh @ 2019-09-27 13:08 UTC (permalink / raw) To: linux-kernel, bpf Cc: Anton Protopopov, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Florent Revest From: KP Singh <kpsingh@google.com> Introduce struct bpf_object_open_buffer_attr and an API function, bpf_object__open_xattr, as the existing API, bpf_object__open_buffer, doesn't provide a way to specify neither the "needs_kver" nor the "flags" parameter to the internal call to the __bpf_object__open which makes it inconvenient for loading BPF objects that do not require a kernel version from a buffer. The flags attribute in the bpf_object_open_buffer_attr is set to MAPS_RELAX_COMPAT when used in bpf_object__open_buffer to maintain backward compatibility as this was added to load objects with non-compat map definitions in: commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map definitions") and bpf_object__open_buffer was called with this flag enabled (as a boolean true value). The existing "bpf_object__open_xattr" cannot be modified to maintain API compatibility. Reported-by: Anton Protopopov <a.s.protopopov@gmail.com> Signed-off-by: KP Singh <kpsingh@google.com> --- tools/lib/bpf/libbpf.c | 39 ++++++++++++++++++++++++++++----------- tools/lib/bpf/libbpf.h | 10 ++++++++++ tools/lib/bpf/libbpf.map | 5 +++++ 3 files changed, 43 insertions(+), 11 deletions(-) This patch is assimilates the feedback from: https://lore.kernel.org/bpf/20190815000330.12044-1-a.s.protopopov@gmail.com/ I have added a "Reported-by:" tag, but please feel free to update to "Co-developed-by" if it's more appropriate from an attribution perspective. diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 2b57d7ea7836..1f1f2e92832b 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -2752,25 +2752,42 @@ struct bpf_object *bpf_object__open(const char *path) return bpf_object__open_xattr(&attr); } -struct bpf_object *bpf_object__open_buffer(void *obj_buf, - size_t obj_buf_sz, - const char *name) +struct bpf_object * +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr) { char tmp_name[64]; /* param validation */ - if (!obj_buf || obj_buf_sz <= 0) - return NULL; + if (!attr || !attr->obj_buf || !(attr->obj_buf_sz <= 0)) + return ERR_PTR(-EINVAL); - if (!name) { + if (!attr->obj_name) { snprintf(tmp_name, sizeof(tmp_name), "%lx-%lx", - (unsigned long)obj_buf, - (unsigned long)obj_buf_sz); - name = tmp_name; + (unsigned long)attr->obj_buf, + (unsigned long)attr->obj_buf_sz); + attr->obj_name = tmp_name; } - pr_debug("loading object '%s' from buffer\n", name); + pr_debug("loading object '%s' from buffer\n", attr->obj_name); + + return __bpf_object__open(attr->obj_name, attr->obj_buf, + attr->obj_buf_sz, + bpf_prog_type__needs_kver(attr->prog_type), + attr->flags); +} + +struct bpf_object *bpf_object__open_buffer(void *obj_buf, + size_t obj_buf_sz, + const char *name) +{ + struct bpf_object_open_buffer_attr attr = { + .obj_name = name, + .obj_buf = obj_buf, + .obj_buf_sz = obj_buf_sz, + .prog_type = BPF_PROG_TYPE_UNSPEC, + .flags = MAPS_RELAX_COMPAT, + }; - return __bpf_object__open(name, obj_buf, obj_buf_sz, true, true); + return bpf_object__open_buffer_xattr(&attr); } int bpf_object__unload(struct bpf_object *obj) diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 5cbf459ece0b..ad0f5a263594 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -67,6 +67,14 @@ struct bpf_object_open_attr { enum bpf_prog_type prog_type; }; +struct bpf_object_open_buffer_attr { + const char *obj_name; + void *obj_buf; + size_t obj_buf_sz; + enum bpf_prog_type prog_type; + int flags; +}; + LIBBPF_API struct bpf_object *bpf_object__open(const char *path); LIBBPF_API struct bpf_object * bpf_object__open_xattr(struct bpf_object_open_attr *attr); @@ -75,6 +83,8 @@ struct bpf_object *__bpf_object__open_xattr(struct bpf_object_open_attr *attr, LIBBPF_API struct bpf_object *bpf_object__open_buffer(void *obj_buf, size_t obj_buf_sz, const char *name); +LIBBPF_API struct bpf_object * +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr); 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, diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index f9d316e873d8..4d5dcc58c73d 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -184,3 +184,8 @@ LIBBPF_0.0.4 { perf_buffer__new_raw; perf_buffer__poll; } LIBBPF_0.0.3; + +LIBBPF_0.0.5 { + global: + bpf_object__open_buffer_xattr; +} LIBBPF_0.0.4; -- 2.20.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr 2019-09-27 13:08 [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr KP Singh @ 2019-09-27 16:06 ` Andrii Nakryiko 2019-09-30 7:12 ` Toke Høiland-Jørgensen 0 siblings, 1 reply; 5+ messages in thread From: Andrii Nakryiko @ 2019-09-27 16:06 UTC (permalink / raw) To: KP Singh Cc: open list, bpf, Anton Protopopov, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Florent Revest On Fri, Sep 27, 2019 at 6:11 AM KP Singh <kpsingh@chromium.org> wrote: > > From: KP Singh <kpsingh@google.com> > > Introduce struct bpf_object_open_buffer_attr and an API function, > bpf_object__open_xattr, as the existing API, bpf_object__open_buffer, > doesn't provide a way to specify neither the "needs_kver" nor > the "flags" parameter to the internal call to the > __bpf_object__open which makes it inconvenient for loading BPF > objects that do not require a kernel version from a buffer. > > The flags attribute in the bpf_object_open_buffer_attr is set > to MAPS_RELAX_COMPAT when used in bpf_object__open_buffer to > maintain backward compatibility as this was added to load objects > with non-compat map definitions in: > > commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map > definitions") > > and bpf_object__open_buffer was called with this flag enabled (as a > boolean true value). > > The existing "bpf_object__open_xattr" cannot be modified to > maintain API compatibility. > > Reported-by: Anton Protopopov <a.s.protopopov@gmail.com> > Signed-off-by: KP Singh <kpsingh@google.com> > --- > tools/lib/bpf/libbpf.c | 39 ++++++++++++++++++++++++++++----------- > tools/lib/bpf/libbpf.h | 10 ++++++++++ > tools/lib/bpf/libbpf.map | 5 +++++ > 3 files changed, 43 insertions(+), 11 deletions(-) > > This patch is assimilates the feedback from: > > https://lore.kernel.org/bpf/20190815000330.12044-1-a.s.protopopov@gmail.com/ > > I have added a "Reported-by:" tag, but please feel free to update to > "Co-developed-by" if it's more appropriate from an attribution perspective. > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 2b57d7ea7836..1f1f2e92832b 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -2752,25 +2752,42 @@ struct bpf_object *bpf_object__open(const char *path) > return bpf_object__open_xattr(&attr); > } > > -struct bpf_object *bpf_object__open_buffer(void *obj_buf, > - size_t obj_buf_sz, > - const char *name) > +struct bpf_object * > +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr) I have few concerns w.r.t. adding API in this form and I'm going to use this specific case to discuss more general problem of API design, ABI compatibility, and extending APIs with extra optional arguments. 1. In general, I think it would be good for libbpf API usability to use the following pattern consistently (moving forward): T1 some_api_function(T2 mandatory_arg1, ..., TN mandatory_arg, struct something_opts *opts) So all the mandatory arguments that have to be provides are specified explicitly as function arguments. That makes it very clear what API expects to get always. opts (we use both opts and attrs, but opts seems better because its optional options :), on the other hand, is stuff that might be omitted, so if user doesn't care about tuning behavior of API and wants all-defaults behavior, then providing NULL here should just work. So in this case for bpf_object__open_buffer_xattr(), it could look like this: struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, struct bpf_object_open_opts* opts); 2. Now, we need to do something with adding new options without breaking ABIs. With all the existing extra attributes, when we need to add new field to that struct, that can break old code that's dynamically linked to newer versions of libbpf, because their attr/opts struct is too short for new code, so that could cause segment violation or can make libbpf read garbage for those newly added fields. There are basically three ways we can go about this: a. either provide the size of opts struct as an extra argument to each API that uses options, so: struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, struct bpf_object_open_opts* opts, size_t opts_sz); b. make it mandatory that every option struct has to have as a first field its size, so: struct bpf_object_open_opts { size_t sz; /* now we can keep adding attrs */ }; Now, when options are provided, we'll read first sizeof(size_t) bytes, validate it for sanity and then we'll know which fields are there or not. Both options have downside of user needing to do extra initialization, but it's not too bad in either case. Especially in case b), if user doesn't care about extra options, then no extra steps are necessary. In case a, we can pass NULL, 0 at the end, so also not a big deal. c. Alternatively, we can do symbol versioning similar how xsk.c started doing it, and handle those options struct size differences transparently. But that's a lot of extra boilerplate code in libbpf and I'd like to avoid that, if possible. 3. Now, the last minor complain is about flags field. It's super generic. Why not have a set of boolean fields in a struct, in this case to allow to specify strict/compat modes. Given we solve struct extensibility issue, adding new bool fields is not an issue at all, so the benefit of flags field are gone. The downside of flags field is that it's very opaque integer, you have to go and read sources to understand all the intended use cases and possible flags, which is certainly not a great user experience. I'm curious to hear opinions of people regarding these ABI issues and solutions. > { > char tmp_name[64]; > [...] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr 2019-09-27 16:06 ` Andrii Nakryiko @ 2019-09-30 7:12 ` Toke Høiland-Jørgensen 2019-09-30 13:24 ` KP Singh 2019-09-30 16:50 ` Andrii Nakryiko 0 siblings, 2 replies; 5+ messages in thread From: Toke Høiland-Jørgensen @ 2019-09-30 7:12 UTC (permalink / raw) To: Andrii Nakryiko, KP Singh Cc: open list, bpf, Anton Protopopov, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Florent Revest Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Fri, Sep 27, 2019 at 6:11 AM KP Singh <kpsingh@chromium.org> wrote: >> >> From: KP Singh <kpsingh@google.com> >> >> Introduce struct bpf_object_open_buffer_attr and an API function, >> bpf_object__open_xattr, as the existing API, bpf_object__open_buffer, >> doesn't provide a way to specify neither the "needs_kver" nor >> the "flags" parameter to the internal call to the >> __bpf_object__open which makes it inconvenient for loading BPF >> objects that do not require a kernel version from a buffer. >> >> The flags attribute in the bpf_object_open_buffer_attr is set >> to MAPS_RELAX_COMPAT when used in bpf_object__open_buffer to >> maintain backward compatibility as this was added to load objects >> with non-compat map definitions in: >> >> commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map >> definitions") >> >> and bpf_object__open_buffer was called with this flag enabled (as a >> boolean true value). >> >> The existing "bpf_object__open_xattr" cannot be modified to >> maintain API compatibility. >> >> Reported-by: Anton Protopopov <a.s.protopopov@gmail.com> >> Signed-off-by: KP Singh <kpsingh@google.com> >> --- >> tools/lib/bpf/libbpf.c | 39 ++++++++++++++++++++++++++++----------- >> tools/lib/bpf/libbpf.h | 10 ++++++++++ >> tools/lib/bpf/libbpf.map | 5 +++++ >> 3 files changed, 43 insertions(+), 11 deletions(-) >> >> This patch is assimilates the feedback from: >> >> https://lore.kernel.org/bpf/20190815000330.12044-1-a.s.protopopov@gmail.com/ >> >> I have added a "Reported-by:" tag, but please feel free to update to >> "Co-developed-by" if it's more appropriate from an attribution perspective. >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 2b57d7ea7836..1f1f2e92832b 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -2752,25 +2752,42 @@ struct bpf_object *bpf_object__open(const char *path) >> return bpf_object__open_xattr(&attr); >> } >> >> -struct bpf_object *bpf_object__open_buffer(void *obj_buf, >> - size_t obj_buf_sz, >> - const char *name) >> +struct bpf_object * >> +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr) > > I have few concerns w.r.t. adding API in this form and I'm going to > use this specific case to discuss more general problem of API design, > ABI compatibility, and extending APIs with extra optional arguments. > > 1. In general, I think it would be good for libbpf API usability to > use the following pattern consistently (moving forward): > > T1 some_api_function(T2 mandatory_arg1, ..., TN mandatory_arg, struct > something_opts *opts) > > So all the mandatory arguments that have to be provides are specified > explicitly as function arguments. That makes it very clear what API > expects to get always. > opts (we use both opts and attrs, but opts seems better because its > optional options :), on the other hand, is stuff that might be > omitted, so if user doesn't care about tuning behavior of API and > wants all-defaults behavior, then providing NULL here should just > work. > > So in this case for bpf_object__open_buffer_xattr(), it could look like this: > > struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, > struct bpf_object_open_opts* opts); I like this idea! Sensible defaults that can be selected by just passing NULL as opts is a laudable goal. > 2. Now, we need to do something with adding new options without > breaking ABIs. With all the existing extra attributes, when we need to > add new field to that struct, that can break old code that's > dynamically linked to newer versions of libbpf, because their > attr/opts struct is too short for new code, so that could cause > segment violation or can make libbpf read garbage for those newly > added fields. There are basically three ways we can go about this: > > a. either provide the size of opts struct as an extra argument to each > API that uses options, so: > struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, > struct bpf_object_open_opts* opts, size_t opts_sz); > > b. make it mandatory that every option struct has to have as a first > field its size, so: > > struct bpf_object_open_opts { > size_t sz; > /* now we can keep adding attrs */ > }; > > Now, when options are provided, we'll read first sizeof(size_t) bytes, > validate it for sanity and then we'll know which fields are there or > not. > > Both options have downside of user needing to do extra initialization, > but it's not too bad in either case. Especially in case b), if user > doesn't care about extra options, then no extra steps are necessary. > In case a, we can pass NULL, 0 at the end, so also not a big deal. > > c. Alternatively, we can do symbol versioning similar how xsk.c > started doing it, and handle those options struct size differences > transparently. But that's a lot of extra boilerplate code in libbpf > and I'd like to avoid that, if possible. My hunch is that we're kidding ourselves if we think we can avoid the symbol versioning. And besides, checking struct sizes needs boilerplate code as well, boilerplate that will fail at runtime instead of compile time if it's done wrong. So IMO we're better off just doing symbol version right from the beginning. > 3. Now, the last minor complain is about flags field. It's super > generic. Why not have a set of boolean fields in a struct, in this > case to allow to specify strict/compat modes. Given we solve struct > extensibility issue, adding new bool fields is not an issue at all, so > the benefit of flags field are gone. The downside of flags field is > that it's very opaque integer, you have to go and read sources to > understand all the intended use cases and possible flags, which is > certainly not a great user experience. This I agree with :) -Toke ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr 2019-09-30 7:12 ` Toke Høiland-Jørgensen @ 2019-09-30 13:24 ` KP Singh 2019-09-30 16:50 ` Andrii Nakryiko 1 sibling, 0 replies; 5+ messages in thread From: KP Singh @ 2019-09-30 13:24 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: Andrii Nakryiko, KP Singh, open list, bpf, Anton Protopopov, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Florent Revest Thanks for the feeback! I will be happy to update this patch once there is consensus about the design of the API for future additions. On 30-Sep 09:12, Toke Høiland-Jørgensen wrote: > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Fri, Sep 27, 2019 at 6:11 AM KP Singh <kpsingh@chromium.org> wrote: > >> > >> From: KP Singh <kpsingh@google.com> > >> > >> Introduce struct bpf_object_open_buffer_attr and an API function, > >> bpf_object__open_xattr, as the existing API, bpf_object__open_buffer, > >> doesn't provide a way to specify neither the "needs_kver" nor > >> the "flags" parameter to the internal call to the > >> __bpf_object__open which makes it inconvenient for loading BPF > >> objects that do not require a kernel version from a buffer. > >> > >> The flags attribute in the bpf_object_open_buffer_attr is set > >> to MAPS_RELAX_COMPAT when used in bpf_object__open_buffer to > >> maintain backward compatibility as this was added to load objects > >> with non-compat map definitions in: > >> > >> commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map > >> definitions") > >> > >> and bpf_object__open_buffer was called with this flag enabled (as a > >> boolean true value). > >> > >> The existing "bpf_object__open_xattr" cannot be modified to > >> maintain API compatibility. > >> > >> Reported-by: Anton Protopopov <a.s.protopopov@gmail.com> > >> Signed-off-by: KP Singh <kpsingh@google.com> > >> --- > >> tools/lib/bpf/libbpf.c | 39 ++++++++++++++++++++++++++++----------- > >> tools/lib/bpf/libbpf.h | 10 ++++++++++ > >> tools/lib/bpf/libbpf.map | 5 +++++ > >> 3 files changed, 43 insertions(+), 11 deletions(-) > >> > >> This patch is assimilates the feedback from: > >> > >> https://lore.kernel.org/bpf/20190815000330.12044-1-a.s.protopopov@gmail.com/ > >> > >> I have added a "Reported-by:" tag, but please feel free to update to > >> "Co-developed-by" if it's more appropriate from an attribution perspective. > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index 2b57d7ea7836..1f1f2e92832b 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -2752,25 +2752,42 @@ struct bpf_object *bpf_object__open(const char *path) > >> return bpf_object__open_xattr(&attr); > >> } > >> > >> -struct bpf_object *bpf_object__open_buffer(void *obj_buf, > >> - size_t obj_buf_sz, > >> - const char *name) > >> +struct bpf_object * > >> +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr) > > > > I have few concerns w.r.t. adding API in this form and I'm going to > > use this specific case to discuss more general problem of API design, > > ABI compatibility, and extending APIs with extra optional arguments. > > > > 1. In general, I think it would be good for libbpf API usability to > > use the following pattern consistently (moving forward): > > > > T1 some_api_function(T2 mandatory_arg1, ..., TN mandatory_arg, struct > > something_opts *opts) > > > > So all the mandatory arguments that have to be provides are specified > > explicitly as function arguments. That makes it very clear what API > > expects to get always. > > opts (we use both opts and attrs, but opts seems better because its > > optional options :), on the other hand, is stuff that might be > > omitted, so if user doesn't care about tuning behavior of API and > > wants all-defaults behavior, then providing NULL here should just > > work. > > > > So in this case for bpf_object__open_buffer_xattr(), it could look like this: > > > > struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, > > struct bpf_object_open_opts* opts); > > I like this idea! Sensible defaults that can be selected by just passing > NULL as opts is a laudable goal. > > > 2. Now, we need to do something with adding new options without > > breaking ABIs. With all the existing extra attributes, when we need to > > add new field to that struct, that can break old code that's > > dynamically linked to newer versions of libbpf, because their > > attr/opts struct is too short for new code, so that could cause > > segment violation or can make libbpf read garbage for those newly > > added fields. There are basically three ways we can go about this: > > > > a. either provide the size of opts struct as an extra argument to each > > API that uses options, so: > > struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, > > struct bpf_object_open_opts* opts, size_t opts_sz); > > > > b. make it mandatory that every option struct has to have as a first > > field its size, so: > > > > struct bpf_object_open_opts { > > size_t sz; > > /* now we can keep adding attrs */ > > }; > > > > Now, when options are provided, we'll read first sizeof(size_t) bytes, > > validate it for sanity and then we'll know which fields are there or > > not. > > > > Both options have downside of user needing to do extra initialization, > > but it's not too bad in either case. Especially in case b), if user > > doesn't care about extra options, then no extra steps are necessary. > > In case a, we can pass NULL, 0 at the end, so also not a big deal. > > > > c. Alternatively, we can do symbol versioning similar how xsk.c > > started doing it, and handle those options struct size differences > > transparently. But that's a lot of extra boilerplate code in libbpf > > and I'd like to avoid that, if possible. > > My hunch is that we're kidding ourselves if we think we can avoid the > symbol versioning. And besides, checking struct sizes needs boilerplate > code as well, boilerplate that will fail at runtime instead of compile > time if it's done wrong. > > So IMO we're better off just doing symbol version right from the > beginning. I see there is already a patch for introducing symbol versioning in libbpf: https://lore.kernel.org/bpf/20190928231916.3054271-1-yhs@fb.com/ - KP > > > 3. Now, the last minor complain is about flags field. It's super > > generic. Why not have a set of boolean fields in a struct, in this > > case to allow to specify strict/compat modes. Given we solve struct > > extensibility issue, adding new bool fields is not an issue at all, so > > the benefit of flags field are gone. The downside of flags field is > > that it's very opaque integer, you have to go and read sources to > > understand all the intended use cases and possible flags, which is > > certainly not a great user experience. > > This I agree with :) > > -Toke > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr 2019-09-30 7:12 ` Toke Høiland-Jørgensen 2019-09-30 13:24 ` KP Singh @ 2019-09-30 16:50 ` Andrii Nakryiko 1 sibling, 0 replies; 5+ messages in thread From: Andrii Nakryiko @ 2019-09-30 16:50 UTC (permalink / raw) To: Toke Høiland-Jørgensen Cc: KP Singh, open list, bpf, Anton Protopopov, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Florent Revest On Mon, Sep 30, 2019 at 12:12 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Fri, Sep 27, 2019 at 6:11 AM KP Singh <kpsingh@chromium.org> wrote: > >> > >> From: KP Singh <kpsingh@google.com> > >> > >> Introduce struct bpf_object_open_buffer_attr and an API function, > >> bpf_object__open_xattr, as the existing API, bpf_object__open_buffer, > >> doesn't provide a way to specify neither the "needs_kver" nor > >> the "flags" parameter to the internal call to the > >> __bpf_object__open which makes it inconvenient for loading BPF > >> objects that do not require a kernel version from a buffer. > >> > >> The flags attribute in the bpf_object_open_buffer_attr is set > >> to MAPS_RELAX_COMPAT when used in bpf_object__open_buffer to > >> maintain backward compatibility as this was added to load objects > >> with non-compat map definitions in: > >> > >> commit c034a177d3c8 ("bpf: bpftool, add flag to allow non-compat map > >> definitions") > >> > >> and bpf_object__open_buffer was called with this flag enabled (as a > >> boolean true value). > >> > >> The existing "bpf_object__open_xattr" cannot be modified to > >> maintain API compatibility. > >> > >> Reported-by: Anton Protopopov <a.s.protopopov@gmail.com> > >> Signed-off-by: KP Singh <kpsingh@google.com> > >> --- > >> tools/lib/bpf/libbpf.c | 39 ++++++++++++++++++++++++++++----------- > >> tools/lib/bpf/libbpf.h | 10 ++++++++++ > >> tools/lib/bpf/libbpf.map | 5 +++++ > >> 3 files changed, 43 insertions(+), 11 deletions(-) > >> > >> This patch is assimilates the feedback from: > >> > >> https://lore.kernel.org/bpf/20190815000330.12044-1-a.s.protopopov@gmail.com/ > >> > >> I have added a "Reported-by:" tag, but please feel free to update to > >> "Co-developed-by" if it's more appropriate from an attribution perspective. > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index 2b57d7ea7836..1f1f2e92832b 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -2752,25 +2752,42 @@ struct bpf_object *bpf_object__open(const char *path) > >> return bpf_object__open_xattr(&attr); > >> } > >> > >> -struct bpf_object *bpf_object__open_buffer(void *obj_buf, > >> - size_t obj_buf_sz, > >> - const char *name) > >> +struct bpf_object * > >> +bpf_object__open_buffer_xattr(struct bpf_object_open_buffer_attr *attr) > > > > I have few concerns w.r.t. adding API in this form and I'm going to > > use this specific case to discuss more general problem of API design, > > ABI compatibility, and extending APIs with extra optional arguments. > > > > 1. In general, I think it would be good for libbpf API usability to > > use the following pattern consistently (moving forward): > > > > T1 some_api_function(T2 mandatory_arg1, ..., TN mandatory_arg, struct > > something_opts *opts) > > > > So all the mandatory arguments that have to be provides are specified > > explicitly as function arguments. That makes it very clear what API > > expects to get always. > > opts (we use both opts and attrs, but opts seems better because its > > optional options :), on the other hand, is stuff that might be > > omitted, so if user doesn't care about tuning behavior of API and > > wants all-defaults behavior, then providing NULL here should just > > work. > > > > So in this case for bpf_object__open_buffer_xattr(), it could look like this: > > > > struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, > > struct bpf_object_open_opts* opts); > > I like this idea! Sensible defaults that can be selected by just passing > NULL as opts is a laudable goal. Cool, I just sent out RFC w/ what I had in mind and applied that to consistent open_mem/open_file APIs, let's discuss further there. > > > 2. Now, we need to do something with adding new options without > > breaking ABIs. With all the existing extra attributes, when we need to > > add new field to that struct, that can break old code that's > > dynamically linked to newer versions of libbpf, because their > > attr/opts struct is too short for new code, so that could cause > > segment violation or can make libbpf read garbage for those newly > > added fields. There are basically three ways we can go about this: > > > > a. either provide the size of opts struct as an extra argument to each > > API that uses options, so: > > struct bpf_object* bpf_object__open_buffer_opts(void *buf, size_t sz, > > struct bpf_object_open_opts* opts, size_t opts_sz); > > > > b. make it mandatory that every option struct has to have as a first > > field its size, so: > > > > struct bpf_object_open_opts { > > size_t sz; > > /* now we can keep adding attrs */ > > }; > > > > Now, when options are provided, we'll read first sizeof(size_t) bytes, > > validate it for sanity and then we'll know which fields are there or > > not. > > > > Both options have downside of user needing to do extra initialization, > > but it's not too bad in either case. Especially in case b), if user > > doesn't care about extra options, then no extra steps are necessary. > > In case a, we can pass NULL, 0 at the end, so also not a big deal. > > > > c. Alternatively, we can do symbol versioning similar how xsk.c > > started doing it, and handle those options struct size differences > > transparently. But that's a lot of extra boilerplate code in libbpf > > and I'd like to avoid that, if possible. > > My hunch is that we're kidding ourselves if we think we can avoid the > symbol versioning. And besides, checking struct sizes needs boilerplate > code as well, boilerplate that will fail at runtime instead of compile > time if it's done wrong. > > So IMO we're better off just doing symbol version right from the > beginning. I'm not against symbol versioning, don't get me wrong. Yonghong is already fixing one of the problems with symbol versioning and after that it should be pretty streamlined to add extra symbol versions. But I'd like to avoid create a new symbol version for every minor new optional field being added. On libbpf side this will cause lots of boilerplate code for each version to "converge" to correct and full struct definition and then call into a common implementation that expects full struct definition. With what I'm proposing in the RFC patch I sent out and CC'ed you on we should have new symbol version only when there is some nontrivial change only, which I think is a good thing to strive for. > > > 3. Now, the last minor complain is about flags field. It's super > > generic. Why not have a set of boolean fields in a struct, in this > > case to allow to specify strict/compat modes. Given we solve struct > > extensibility issue, adding new bool fields is not an issue at all, so > > the benefit of flags field are gone. The downside of flags field is > > that it's very opaque integer, you have to go and read sources to > > understand all the intended use cases and possible flags, which is > > certainly not a great user experience. > > This I agree with :) > > -Toke > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-30 16:50 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-27 13:08 [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr KP Singh 2019-09-27 16:06 ` Andrii Nakryiko 2019-09-30 7:12 ` Toke Høiland-Jørgensen 2019-09-30 13:24 ` KP Singh 2019-09-30 16:50 ` Andrii Nakryiko
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).