Re: [PATCH bpf-next] libbpf: Add bpf_object__unpin()

[Daniel Xu]

On Wed, Aug 23, 2023 at 10:19:10AM -0700, Andrii Nakryiko wrote:
> On Tue, Aug 22, 2023 at 10:44 PM Daniel Xu <dxu@xxxxxxxxx> wrote:
> >
> > For bpf_object__pin_programs() there is bpf_object__unpin_programs().
> > Likewise bpf_object__unpin_maps() for bpf_object__pin_maps().
> >
> > But no bpf_object__unpin() for bpf_object__pin(). Adding the former adds
> > symmetry to the API.
> >
> > It's also convenient for cleanup in application code. It's an API I
> > would've used if it was available for a repro I was writing earlier.
> >
> > Signed-off-by: Daniel Xu <dxu@xxxxxxxxx>
> > ---
> >  tools/lib/bpf/libbpf.c   | 15 +++++++++++++++
> >  tools/lib/bpf/libbpf.h   |  1 +
> >  tools/lib/bpf/libbpf.map |  1 +
> >  3 files changed, 17 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 4c3967d94b6d..96ff1aa4bf6a 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8376,6 +8376,21 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
> >         return 0;
> >  }
> >
> > +int bpf_object__unpin(struct bpf_object *obj, const char *path)
> > +{
> > +       int err;
> > +
> > +       err = bpf_object__unpin_programs(obj, path);
> > +       if (err)
> > +               return libbpf_err(err);
> > +
> > +       err = bpf_object__unpin_maps(obj, path);
> > +       if (err)
> > +               return libbpf_err(err);
> > +
> > +       return 0;
> > +}
> > +
> 
> pin APIs predate me, and I barely ever use them, but I wonder if
> people feel fine with the fact that if any single unpin fails, all the
> other programs/maps will not be unpinned? I also wonder if the best
> effort unpinning of everything (while propagating first/last error) is
> more practical? Looking at bpf_object__pin_programs, we try unpin
> everything, even if some unpins fail.
> 
> Any thoughts or preferences?

Yeah, I noticed bpf_object__pin_programs() tries to simulate some
transactionality. However, bpf_object__unpin_programs() and
bpf_object__unpin_maps() both do not try rollbacks and have already been
exposed as public API. So I thought it would be best to stay consistent.

I also figured it's unlikely only a single unpin() fails. For pin(), you
could have name collisions. But not for unpin(). I suppose the main
error case is if some 3rd party (or yourself) comes in and messes with
your objects in bpffs.

In general, though, there are other places where transactionality would
be a nice property. For example, if I have a TC prog that I want to
attach to, say, _all_ ethernet interfaces, I have to be careful about
rollbacks in the event of failure on a single iface.

It would be really nice if the kernel  had a general way to provide
atomicity w.r.t. multiple operations. But I suppose that's a hard
problem.

[...]

Thanks,
Daniel