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

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

[Daniel Xu]

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@dxuuu.xyz>
---
 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;
+}
+
 static void bpf_map__destroy(struct bpf_map *map)
 {
 	if (map->inner_map) {
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 2e3eb3614c40..0e52621cba43 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -266,6 +266,7 @@ LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
 LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
 					  const char *path);
 LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
+LIBBPF_API int bpf_object__unpin(struct bpf_object *object, const char *path);
 
 LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj);
 LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj);
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 841a2f9c6fef..abf8fea3988e 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -399,4 +399,5 @@ LIBBPF_1.3.0 {
 		bpf_program__attach_netfilter;
 		bpf_program__attach_tcx;
 		bpf_program__attach_uprobe_multi;
+		bpf_object__unpin;
 } LIBBPF_1.2.0;
-- 
2.41.0

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

[Song Liu]

On Tue, Aug 22, 2023 at 10:44 PM Daniel Xu <dxu@dxuuu.xyz> 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@dxuuu.xyz>

Reviewed-by: Song Liu <song@kernel.org>

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

[Jiri Olsa]

On Tue, Aug 22, 2023 at 11:44:25PM -0600, Daniel Xu 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@dxuuu.xyz>
> ---
>  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;
> +}
> +
>  static void bpf_map__destroy(struct bpf_map *map)
>  {
>  	if (map->inner_map) {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 2e3eb3614c40..0e52621cba43 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -266,6 +266,7 @@ LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
>  LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
>  					  const char *path);
>  LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
> +LIBBPF_API int bpf_object__unpin(struct bpf_object *object, const char *path);
>  
>  LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj);
>  LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 841a2f9c6fef..abf8fea3988e 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -399,4 +399,5 @@ LIBBPF_1.3.0 {
>  		bpf_program__attach_netfilter;
>  		bpf_program__attach_tcx;
>  		bpf_program__attach_uprobe_multi;
> +		bpf_object__unpin;

nit, I think we add in here in alphabetical order

jirka

>  } LIBBPF_1.2.0;
> -- 
> 2.41.0
> 

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

[Andrii Nakryiko]

On Tue, Aug 22, 2023 at 10:44 PM Daniel Xu <dxu@dxuuu.xyz> 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@dxuuu.xyz>
> ---
>  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?

>  static void bpf_map__destroy(struct bpf_map *map)
>  {
>         if (map->inner_map) {
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 2e3eb3614c40..0e52621cba43 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -266,6 +266,7 @@ LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj,
>  LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj,
>                                           const char *path);
>  LIBBPF_API int bpf_object__pin(struct bpf_object *object, const char *path);
> +LIBBPF_API int bpf_object__unpin(struct bpf_object *object, const char *path);
>
>  LIBBPF_API const char *bpf_object__name(const struct bpf_object *obj);
>  LIBBPF_API unsigned int bpf_object__kversion(const struct bpf_object *obj);
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 841a2f9c6fef..abf8fea3988e 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -399,4 +399,5 @@ LIBBPF_1.3.0 {
>                 bpf_program__attach_netfilter;
>                 bpf_program__attach_tcx;
>                 bpf_program__attach_uprobe_multi;
> +               bpf_object__unpin;
>  } LIBBPF_1.2.0;
> --
> 2.41.0
>

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

[Song Liu]

On Wed, Aug 23, 2023 at 10:19 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Aug 22, 2023 at 10:44 PM Daniel Xu <dxu@dxuuu.xyz> 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@dxuuu.xyz>
> > ---
> >  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.

I think the behavior of bpf_object__pin_programs() makes sense for
the error handling path, e.g. undo everything as much as possible.
bpf_object__unpin(), OTOH, is similar to bpf_object__unpin_programs()
and bpf_object__unpin_maps(), if something went wrong, we just give up.

>
> Any thoughts or preferences?

I think current version is ok, as it matches bpf_object__unpin_programs
and bpf_object__unpin_maps behavior. But I guess we can change all
3?

Thanks,
Song

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@dxuuu.xyz> 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@dxuuu.xyz>
> > ---
> >  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

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

[Andrii Nakryiko]

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