On Thu, Mar 25, 2021 at 5:56 PM John Fastabend <john.fastabend@xxxxxxxxx> wrote:
>
> Andrii Nakryiko wrote:
> > On Mon, Mar 22, 2021 at 10:31 PM John Fastabend
> > <john.fastabend@xxxxxxxxx> wrote:
> > >
> > > Rafael David Tinoco wrote:
> > > > Unfortunately some distros don't have their kernel version defined
> > > > accurately in <linux/version.h> due to different long term support
> > > > reasons.
> > > >
> > > > It is important to have a way to override the bpf kern_version
> > > > attribute during runtime: some old kernels might still check for
> > > > kern_version attribute during bpf_prog_load().
> > > >
> > > > Signed-off-by: Rafael David Tinoco <rafaeldtinoco@xxxxxxxxxx>
> > > > ---
> > > > tools/lib/bpf/libbpf.c | 10 ++++++++++
> > > > tools/lib/bpf/libbpf.h | 1 +
> > > > tools/lib/bpf/libbpf.map | 1 +
> > > > 3 files changed, 12 insertions(+)
> > > >
> > >
> > > Hi Andrii and Rafael,
> > >
> > > Did you consider making kernel version an attribute of the load
> > > API, bpf_prog_load_xattr()? This feels slightly more natural
> > > to me, to tell the API the kernel you need at load time.
> >
> > Um... kern_version is already part of bpf_load_program_attr, used by
> > bpf_load_program_xattr. What am I missing? But you can't use that with
> > bpf_object APIs.
>
> Aha I mistyped. It looks like I have a patch floating around on my
> stack to add it to bpf_object_load_attr.
Oh, you meant this one. I'm actually trying to move away from having
load() to take options at all. If you check BPF skeletons, their load
doesn't even accept options. Adding getters/setter is better in one
major way:
- it's more flexible approach and allows to have both per-object
setters/options and per-program ones. bpf_object_load_attr provides
only per-object options, which are often inadequate (see recent
bpf_program__set_attach_target() and bpf_program__set_autoload(),
which are just impossible to sanely do with per-object options)
- even though we now have the whole forward/backwards compatible
OPTS "framework" within libbpf, I think it's less pleasant to use than
setters. We have to do options on load, because we don't have any
object before open happens (if we had separate new() and open() that
wouldn't be the case), so there is a need to specify things before
bpf_object is instantiated. bpf_object__load() doesn't have this
problem, because we have entire bpf_object and bpf_map/bpf_program to
tweak before we perform load.
- adding new APIs is inherently forward compatible. And backwards
compatibility is the same between OPTS and new API methods: you need
to make sure to use recent enough libbpf version that has options/API
you need.
So in short, I'm against adding load-time options, because there are
better and more flexible alternatives.
>
>
> > >
> > > Although, I don't use the skeleton pieces so maybe it would be
> > > awkward for that usage.
> >
> > Yes, low-level APIs are separate. This is for cases where you have
> > struct bpf_program abstractions, which are loaded by
> > bpf_object__load(). We could set it at per-program level, but they
> > should be all the same, so bpf_object__set_kversion() makes more sense
> > and is more convenient to use. And there is already a getter for that,
> > so it complements that nicely.
>
> +1
>
> >
> > >
> > > Sorry, missed v1,v2 so didn't reply sooner.
> > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 058b643cbcb1..3ac3d8dced7f 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -8269,6 +8269,16 @@ int bpf_object__btf_fd(const struct bpf_object *obj)
> > > > return obj->btf ? btf__fd(obj->btf) : -1;
> > > > }
> > > >
> > > > +int bpf_object__set_kversion(struct bpf_object *obj, __u32 kern_version)
> > > > +{
> > > > + if (obj->loaded)
> > > > + return -EINVAL;
> > > > +
> > > > + obj->kern_version = kern_version;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > >
> > > Having a test to read uname and feed it into libbpf using
> > > above to be sure we don't break this in the future would be
> > > nice.
> >
> > kern_version has been ignored by kernel for a long time. So there is
> > no way to test this in selftests/bpf. We could use libbpf CI's old
> > kernel setup to validate, but I don't think it's worth it. It's
> > extremely unlikely this will ever change or break (and it's a legacy
> > stuff we move away from anyways, so it's born sort of obsolete).
>
> +1
>
> For the patch, thanks for the details Andrii, thanks for the patch
> Rafael it will be useful here.
>
> Acked-by: John Fastabend <john.fastabend@xxxxxxxxx>
