| Message ID | aeb83832ae61bbf463e1b2e39c1e30c3b227f5a5.1692769396.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: Add bpf_object__unpin() | expand |
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>
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 >
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 >
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
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
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
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;
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(+)