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

[Andrii Nakryiko]

On Wed, Aug 23, 2023 at 11:43 AM Daniel Xu <dxu@xxxxxxxxx> wrote:
>
> 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.

yep, makes sense. I guess if I were to rely heavily on
pinning/unpinning, I always have an option to pin/unpin individually.
Ok, please address the other feedback and resubmit.

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