bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: KP Singh <kpsingh@chromium.org>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	open list <linux-kernel@vger.kernel.org>,
	bpf <bpf@vger.kernel.org>,
	Anton Protopopov <a.s.protopopov@gmail.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Florent Revest <revest@chromium.org>
Subject: Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr
Date: Mon, 30 Sep 2019 15:24:51 +0200	[thread overview]
Message-ID: <20190930132451.GA27081@google.com> (raw)
In-Reply-To: <87h84uxno9.fsf@toke.dk>

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
> 

  reply	other threads:[~2019-09-30 13:24 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-27 13:08 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 [this message]
2019-09-30 16:50     ` Andrii Nakryiko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190930132451.GA27081@google.com \
    --to=kpsingh@chromium.org \
    --cc=a.s.protopopov@gmail.com \
    --cc=andrii.nakryiko@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=daniel@iogearbox.net \
    --cc=kafai@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=revest@chromium.org \
    --cc=songliubraving@fb.com \
    --cc=toke@redhat.com \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH] tools: libbpf: Add bpf_object__open_buffer_xattr' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).