* [PATCH bpf-next 0/2] libbpf: add an option to reuse maps when loading a program @ 2019-07-05 20:40 Anton Protopopov 2019-07-05 20:44 ` [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() Anton Protopopov 2019-07-05 20:44 ` [PATCH bpf-next 2/2] bpf, libbpf: add an option to reuse existing maps in bpf_prog_load_xattr Anton Protopopov 0 siblings, 2 replies; 9+ messages in thread From: Anton Protopopov @ 2019-07-05 20:40 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf, linux-kernel Cc: Anton Protopopov The following two patches add an option for users to reuse existing maps when loading a program using the bpf_prog_load_xattr function. A user can specify a directory containing pinned maps inside the bpf_prog_load_attr structure, and in this case the bpf_prog_load_xattr function will replace (bpf_map__reuse_fd) all maps defined in the object with file descriptors obtained from corresponding entries from the specified directory. Anton Protopopov (2): bpf, libbpf: add a new API bpf_object__reuse_maps() bpf, libbpf: add an option to reuse existing maps in bpf_prog_load_xattr tools/lib/bpf/libbpf.c | 42 ++++++++++++++++++++++++++++++++++++++++ tools/lib/bpf/libbpf.h | 3 +++ tools/lib/bpf/libbpf.map | 1 + 3 files changed, 46 insertions(+) -- 2.19.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-05 20:40 [PATCH bpf-next 0/2] libbpf: add an option to reuse maps when loading a program Anton Protopopov @ 2019-07-05 20:44 ` Anton Protopopov 2019-07-05 21:44 ` Daniel Borkmann 2019-07-05 20:44 ` [PATCH bpf-next 2/2] bpf, libbpf: add an option to reuse existing maps in bpf_prog_load_xattr Anton Protopopov 1 sibling, 1 reply; 9+ messages in thread From: Anton Protopopov @ 2019-07-05 20:44 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf, linux-kernel Cc: Anton Protopopov Add a new API bpf_object__reuse_maps() which can be used to replace all maps in an object by maps pinned to a directory provided in the path argument. Namely, each map M in the object will be replaced by a map pinned to path/M.name. Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ tools/lib/bpf/libbpf.h | 2 ++ tools/lib/bpf/libbpf.map | 1 + 3 files changed, 37 insertions(+) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 4907997289e9..84c9e8f7bfd3 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) return 0; } +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) +{ + struct bpf_map *map; + + if (!obj) + return -ENOENT; + + if (!path) + return -EINVAL; + + bpf_object__for_each_map(map, obj) { + int len, err; + int pinned_map_fd; + char buf[PATH_MAX]; + + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); + if (len < 0) { + return -EINVAL; + } else if (len >= PATH_MAX) { + return -ENAMETOOLONG; + } + + pinned_map_fd = bpf_obj_get(buf); + if (pinned_map_fd < 0) + return pinned_map_fd; + + err = bpf_map__reuse_fd(map, pinned_map_fd); + if (err) + return err; + } + + return 0; +} + int bpf_object__pin_programs(struct bpf_object *obj, const char *path) { struct bpf_program *prog; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index d639f47e3110..7fe465a1be76 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, const char *path); +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, + const char *path); LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, const char *path); LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map index 2c6d835620d2..66a30be6696c 100644 --- a/tools/lib/bpf/libbpf.map +++ b/tools/lib/bpf/libbpf.map @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { btf_dump__new; btf__parse_elf; bpf_object__load_xattr; + bpf_object__reuse_maps; libbpf_num_possible_cpus; } LIBBPF_0.0.3; -- 2.19.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-05 20:44 ` [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() Anton Protopopov @ 2019-07-05 21:44 ` Daniel Borkmann 2019-07-08 15:11 ` Anton Protopopov 2019-07-08 17:54 ` Andrii Nakryiko 0 siblings, 2 replies; 9+ messages in thread From: Daniel Borkmann @ 2019-07-05 21:44 UTC (permalink / raw) To: Anton Protopopov, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf, linux-kernel, andriin On 07/05/2019 10:44 PM, Anton Protopopov wrote: > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in > an object by maps pinned to a directory provided in the path argument. Namely, > each map M in the object will be replaced by a map pinned to path/M.name. > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> > --- > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ > tools/lib/bpf/libbpf.h | 2 ++ > tools/lib/bpf/libbpf.map | 1 + > 3 files changed, 37 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 4907997289e9..84c9e8f7bfd3 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > return 0; > } > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) > +{ > + struct bpf_map *map; > + > + if (!obj) > + return -ENOENT; > + > + if (!path) > + return -EINVAL; > + > + bpf_object__for_each_map(map, obj) { > + int len, err; > + int pinned_map_fd; > + char buf[PATH_MAX]; We'd need to skip the case of bpf_map__is_internal(map) since they are always recreated for the given object. > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > + if (len < 0) { > + return -EINVAL; > + } else if (len >= PATH_MAX) { > + return -ENAMETOOLONG; > + } > + > + pinned_map_fd = bpf_obj_get(buf); > + if (pinned_map_fd < 0) > + return pinned_map_fd; Should we rather have a new map definition attribute that tells to reuse the map if it's pinned in bpf fs, and if not, we create it and later on pin it? This is what iproute2 is doing and which we're making use of heavily. In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps too limiting for a generic API as new version of an object file may contain new maps which are not yet present in bpf fs at that point. > + err = bpf_map__reuse_fd(map, pinned_map_fd); > + if (err) > + return err; > + } > + > + return 0; > +} > + > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > { > struct bpf_program *prog; > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index d639f47e3110..7fe465a1be76 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > const char *path); > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, > + const char *path); > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > const char *path); > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > index 2c6d835620d2..66a30be6696c 100644 > --- a/tools/lib/bpf/libbpf.map > +++ b/tools/lib/bpf/libbpf.map > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { > btf_dump__new; > btf__parse_elf; > bpf_object__load_xattr; > + bpf_object__reuse_maps; > libbpf_num_possible_cpus; > } LIBBPF_0.0.3; > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-05 21:44 ` Daniel Borkmann @ 2019-07-08 15:11 ` Anton Protopopov 2019-07-08 17:54 ` Andrii Nakryiko 1 sibling, 0 replies; 9+ messages in thread From: Anton Protopopov @ 2019-07-08 15:11 UTC (permalink / raw) To: Daniel Borkmann Cc: Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf, linux-kernel, andriin пт, 5 июл. 2019 г. в 17:44, Daniel Borkmann <daniel@iogearbox.net>: > > On 07/05/2019 10:44 PM, Anton Protopopov wrote: > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in > > an object by maps pinned to a directory provided in the path argument. Namely, > > each map M in the object will be replaced by a map pinned to path/M.name. > > > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> > > --- > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ > > tools/lib/bpf/libbpf.h | 2 ++ > > tools/lib/bpf/libbpf.map | 1 + > > 3 files changed, 37 insertions(+) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 4907997289e9..84c9e8f7bfd3 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > > return 0; > > } > > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) > > +{ > > + struct bpf_map *map; > > + > > + if (!obj) > > + return -ENOENT; > > + > > + if (!path) > > + return -EINVAL; > > + > > + bpf_object__for_each_map(map, obj) { > > + int len, err; > > + int pinned_map_fd; > > + char buf[PATH_MAX]; > > We'd need to skip the case of bpf_map__is_internal(map) since they are always > recreated for the given object. > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > > + if (len < 0) { > > + return -EINVAL; > > + } else if (len >= PATH_MAX) { > > + return -ENAMETOOLONG; > > + } > > + > > + pinned_map_fd = bpf_obj_get(buf); > > + if (pinned_map_fd < 0) > > + return pinned_map_fd; > > Should we rather have a new map definition attribute that tells to reuse > the map if it's pinned in bpf fs, and if not, we create it and later on > pin it? This is what iproute2 is doing and which we're making use of heavily. What do you think about adding a new generic field, say load_flags, to the bpf_map_def structure and a particular flag, say LOAD_F_STICKY for this purpose? And it will be cleared for internal maps, so we will skip them as well. > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps > too limiting for a generic API as new version of an object file may contain > new maps which are not yet present in bpf fs at that point. How permissive should it be? Is it ok to just print a warning on any bpf_obj_get() failure? Or does it make sense to skip some specific error (ENOENT) and reject on other errors? > > > + err = bpf_map__reuse_fd(map, pinned_map_fd); > > + if (err) > > + return err; > > + } > > + > > + return 0; > > +} > > + > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > > { > > struct bpf_program *prog; > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index d639f47e3110..7fe465a1be76 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > > const char *path); > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, > > + const char *path); > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > > const char *path); > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > index 2c6d835620d2..66a30be6696c 100644 > > --- a/tools/lib/bpf/libbpf.map > > +++ b/tools/lib/bpf/libbpf.map > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { > > btf_dump__new; > > btf__parse_elf; > > bpf_object__load_xattr; > > + bpf_object__reuse_maps; > > libbpf_num_possible_cpus; > > } LIBBPF_0.0.3; > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-05 21:44 ` Daniel Borkmann 2019-07-08 15:11 ` Anton Protopopov @ 2019-07-08 17:54 ` Andrii Nakryiko 2019-07-08 20:37 ` Anton Protopopov 1 sibling, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2019-07-08 17:54 UTC (permalink / raw) To: Daniel Borkmann Cc: Anton Protopopov, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Networking, bpf, open list, Andrii Nakryiko On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 07/05/2019 10:44 PM, Anton Protopopov wrote: > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in > > an object by maps pinned to a directory provided in the path argument. Namely, > > each map M in the object will be replaced by a map pinned to path/M.name. > > > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> > > --- > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ > > tools/lib/bpf/libbpf.h | 2 ++ > > tools/lib/bpf/libbpf.map | 1 + > > 3 files changed, 37 insertions(+) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 4907997289e9..84c9e8f7bfd3 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > > return 0; > > } > > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) As is, bpf_object__reuse_maps() can be easily implemented by user applications, as it's only using public libbpf APIs, so I'm not 100% sure we need to add method like that to libbpf. > > +{ > > + struct bpf_map *map; > > + > > + if (!obj) > > + return -ENOENT; > > + > > + if (!path) > > + return -EINVAL; > > + > > + bpf_object__for_each_map(map, obj) { > > + int len, err; > > + int pinned_map_fd; > > + char buf[PATH_MAX]; > > We'd need to skip the case of bpf_map__is_internal(map) since they are always > recreated for the given object. > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > > + if (len < 0) { > > + return -EINVAL; > > + } else if (len >= PATH_MAX) { > > + return -ENAMETOOLONG; > > + } > > + > > + pinned_map_fd = bpf_obj_get(buf); > > + if (pinned_map_fd < 0) > > + return pinned_map_fd; > > Should we rather have a new map definition attribute that tells to reuse > the map if it's pinned in bpf fs, and if not, we create it and later on > pin it? This is what iproute2 is doing and which we're making use of heavily. I'd like something like that as well. This would play nicely with recently added BTF-defined maps as well. I think it should be not just pin/don't pin flag, but rather pinning strategy, to accommodate various typical strategies of handling maps that are already pinned. So something like this: 1. BPF_PIN_NOTHING - default, don't pin; 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail; 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be exact state of object's map; 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this is how Cilium is pinning PROG_ARRAY maps, if I understand correctly); 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values. This list is only for illustrative purposes, ideally people that have a lot of experience using pinning for real-world use cases would chime in on what strategies are useful and make sense. > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps > too limiting for a generic API as new version of an object file may contain > new maps which are not yet present in bpf fs at that point. > > > + err = bpf_map__reuse_fd(map, pinned_map_fd); > > + if (err) > > + return err; > > + } > > + > > + return 0; > > +} > > + > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > > { > > struct bpf_program *prog; > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > index d639f47e3110..7fe465a1be76 100644 > > --- a/tools/lib/bpf/libbpf.h > > +++ b/tools/lib/bpf/libbpf.h > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > > const char *path); > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, > > + const char *path); > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > > const char *path); > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > index 2c6d835620d2..66a30be6696c 100644 > > --- a/tools/lib/bpf/libbpf.map > > +++ b/tools/lib/bpf/libbpf.map > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { > > btf_dump__new; > > btf__parse_elf; > > bpf_object__load_xattr; > > + bpf_object__reuse_maps; > > libbpf_num_possible_cpus; > > } LIBBPF_0.0.3; > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-08 17:54 ` Andrii Nakryiko @ 2019-07-08 20:37 ` Anton Protopopov 2019-07-09 17:40 ` Andrii Nakryiko 0 siblings, 1 reply; 9+ messages in thread From: Anton Protopopov @ 2019-07-08 20:37 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Networking, bpf, open list, Andrii Nakryiko пн, 8 июл. 2019 г. в 13:54, Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > On 07/05/2019 10:44 PM, Anton Protopopov wrote: > > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in > > > an object by maps pinned to a directory provided in the path argument. Namely, > > > each map M in the object will be replaced by a map pinned to path/M.name. > > > > > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> > > > --- > > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ > > > tools/lib/bpf/libbpf.h | 2 ++ > > > tools/lib/bpf/libbpf.map | 1 + > > > 3 files changed, 37 insertions(+) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index 4907997289e9..84c9e8f7bfd3 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > > > return 0; > > > } > > > > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) > > As is, bpf_object__reuse_maps() can be easily implemented by user > applications, as it's only using public libbpf APIs, so I'm not 100% > sure we need to add method like that to libbpf. The bpf_object__reuse_maps() can definitely be implemented by user applications, however, to use it a user also needs to re-implement the bpf_prog_load_xattr funciton, so it seemed to me that adding this functionality to the library is a better way. > > > > +{ > > > + struct bpf_map *map; > > > + > > > + if (!obj) > > > + return -ENOENT; > > > + > > > + if (!path) > > > + return -EINVAL; > > > + > > > + bpf_object__for_each_map(map, obj) { > > > + int len, err; > > > + int pinned_map_fd; > > > + char buf[PATH_MAX]; > > > > We'd need to skip the case of bpf_map__is_internal(map) since they are always > > recreated for the given object. > > > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > > > + if (len < 0) { > > > + return -EINVAL; > > > + } else if (len >= PATH_MAX) { > > > + return -ENAMETOOLONG; > > > + } > > > + > > > + pinned_map_fd = bpf_obj_get(buf); > > > + if (pinned_map_fd < 0) > > > + return pinned_map_fd; > > > > Should we rather have a new map definition attribute that tells to reuse > > the map if it's pinned in bpf fs, and if not, we create it and later on > > pin it? This is what iproute2 is doing and which we're making use of heavily. > > I'd like something like that as well. This would play nicely with > recently added BTF-defined maps as well. > > I think it should be not just pin/don't pin flag, but rather pinning > strategy, to accommodate various typical strategies of handling maps > that are already pinned. So something like this: > > 1. BPF_PIN_NOTHING - default, don't pin; > 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail; > 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be > exact state of object's map; > 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this > is how Cilium is pinning PROG_ARRAY maps, if I understand correctly); > 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values. > > This list is only for illustrative purposes, ideally people that have > a lot of experience using pinning for real-world use cases would chime > in on what strategies are useful and make sense. My case was simply to reuse existing maps when reloading a program. Does it make sense for you to add only the simplest cases of listed above? Also, libbpf doesn't use standard naming conventions for pinning maps. Does it make sense to provide a list of already open maps to the bpf_prog_load_xattr function as an attribute? In this case a user can execute his own policy on pinning, but still will have an option to reuse, reset, and merge maps. > > > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps > > too limiting for a generic API as new version of an object file may contain > > new maps which are not yet present in bpf fs at that point. > > > > > + err = bpf_map__reuse_fd(map, pinned_map_fd); > > > + if (err) > > > + return err; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > > > { > > > struct bpf_program *prog; > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > > index d639f47e3110..7fe465a1be76 100644 > > > --- a/tools/lib/bpf/libbpf.h > > > +++ b/tools/lib/bpf/libbpf.h > > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > > > const char *path); > > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, > > > + const char *path); > > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > > > const char *path); > > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > > index 2c6d835620d2..66a30be6696c 100644 > > > --- a/tools/lib/bpf/libbpf.map > > > +++ b/tools/lib/bpf/libbpf.map > > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { > > > btf_dump__new; > > > btf__parse_elf; > > > bpf_object__load_xattr; > > > + bpf_object__reuse_maps; > > > libbpf_num_possible_cpus; > > > } LIBBPF_0.0.3; > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-08 20:37 ` Anton Protopopov @ 2019-07-09 17:40 ` Andrii Nakryiko 2019-07-16 17:14 ` Anton Protopopov 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2019-07-09 17:40 UTC (permalink / raw) To: Anton Protopopov Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Networking, bpf, open list, Andrii Nakryiko On Mon, Jul 8, 2019 at 1:37 PM Anton Protopopov <a.s.protopopov@gmail.com> wrote: > > пн, 8 июл. 2019 г. в 13:54, Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > > > On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > > > On 07/05/2019 10:44 PM, Anton Protopopov wrote: > > > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in > > > > an object by maps pinned to a directory provided in the path argument. Namely, > > > > each map M in the object will be replaced by a map pinned to path/M.name. > > > > > > > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> > > > > --- > > > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ > > > > tools/lib/bpf/libbpf.h | 2 ++ > > > > tools/lib/bpf/libbpf.map | 1 + > > > > 3 files changed, 37 insertions(+) > > > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > > index 4907997289e9..84c9e8f7bfd3 100644 > > > > --- a/tools/lib/bpf/libbpf.c > > > > +++ b/tools/lib/bpf/libbpf.c > > > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > > > > return 0; > > > > } > > > > > > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) > > > > As is, bpf_object__reuse_maps() can be easily implemented by user > > applications, as it's only using public libbpf APIs, so I'm not 100% > > sure we need to add method like that to libbpf. > > The bpf_object__reuse_maps() can definitely be implemented by user > applications, however, to use it a user also needs to re-implement the > bpf_prog_load_xattr funciton, so it seemed to me that adding this > functionality to the library is a better way. I'm still not convinced. Looking at bpf_prog_load_xattr, I think some of what it's doing should be part of bpf_object__object_xattr anyway (all the expected type setting for programs). Besides that, there isn't much more than just bpf_object__open and bpf_object__load, to be honest. By doing open and load explicitly, user gets an opportunity to do whatever adjustment they need: reuse maps, adjust map sizes, etc. So I think we should improve bpf_object__open to "guess" program attach types and add map definition flags to allow reuse declaratively. > > > > > > > +{ > > > > + struct bpf_map *map; > > > > + > > > > + if (!obj) > > > > + return -ENOENT; > > > > + > > > > + if (!path) > > > > + return -EINVAL; > > > > + > > > > + bpf_object__for_each_map(map, obj) { > > > > + int len, err; > > > > + int pinned_map_fd; > > > > + char buf[PATH_MAX]; > > > > > > We'd need to skip the case of bpf_map__is_internal(map) since they are always > > > recreated for the given object. > > > > > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > > > > + if (len < 0) { > > > > + return -EINVAL; > > > > + } else if (len >= PATH_MAX) { > > > > + return -ENAMETOOLONG; > > > > + } > > > > + > > > > + pinned_map_fd = bpf_obj_get(buf); > > > > + if (pinned_map_fd < 0) > > > > + return pinned_map_fd; > > > > > > Should we rather have a new map definition attribute that tells to reuse > > > the map if it's pinned in bpf fs, and if not, we create it and later on > > > pin it? This is what iproute2 is doing and which we're making use of heavily. > > > > I'd like something like that as well. This would play nicely with > > recently added BTF-defined maps as well. > > > > I think it should be not just pin/don't pin flag, but rather pinning > > strategy, to accommodate various typical strategies of handling maps > > that are already pinned. So something like this: > > > > 1. BPF_PIN_NOTHING - default, don't pin; > > 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail; > > 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be > > exact state of object's map; > > 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this > > is how Cilium is pinning PROG_ARRAY maps, if I understand correctly); > > 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values. > > > > This list is only for illustrative purposes, ideally people that have > > a lot of experience using pinning for real-world use cases would chime > > in on what strategies are useful and make sense. > > My case was simply to reuse existing maps when reloading a program. > Does it make sense for you to add only the simplest cases of listed above? Of course, it's enum, so we can start with few clearly useful ones and then expand more if we ever have a need. But I think we still need a bit wider discussion and let people who use pinning to chime in. > > Also, libbpf doesn't use standard naming conventions for pinning maps. We talked about this in another thread related to BTF-defined maps. I think the way to go with this is to actually define a default pinning root path, but allow to override it on bpf_object__open, if user needs a different one. > Does it make sense to provide a list of already open maps to the > bpf_prog_load_xattr function as an attribute? In this case a user > can execute his own policy on pinning, but still will have an option > to reuse, reset, and merge maps. As explained above, I don't think there isn't much added value in bpf_prog_load, so I'd advise to just switch to explicit bpf_object__open + bpf_object__load and get maximum control and flexibility. > > > > > > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps > > > too limiting for a generic API as new version of an object file may contain > > > new maps which are not yet present in bpf fs at that point. > > > > > > > + err = bpf_map__reuse_fd(map, pinned_map_fd); > > > > + if (err) > > > > + return err; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > > > > { > > > > struct bpf_program *prog; > > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > > > index d639f47e3110..7fe465a1be76 100644 > > > > --- a/tools/lib/bpf/libbpf.h > > > > +++ b/tools/lib/bpf/libbpf.h > > > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > > > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > > > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > > > > const char *path); > > > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, > > > > + const char *path); > > > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > > > > const char *path); > > > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > > > index 2c6d835620d2..66a30be6696c 100644 > > > > --- a/tools/lib/bpf/libbpf.map > > > > +++ b/tools/lib/bpf/libbpf.map > > > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { > > > > btf_dump__new; > > > > btf__parse_elf; > > > > bpf_object__load_xattr; > > > > + bpf_object__reuse_maps; > > > > libbpf_num_possible_cpus; > > > > } LIBBPF_0.0.3; > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() 2019-07-09 17:40 ` Andrii Nakryiko @ 2019-07-16 17:14 ` Anton Protopopov 0 siblings, 0 replies; 9+ messages in thread From: Anton Protopopov @ 2019-07-16 17:14 UTC (permalink / raw) To: Andrii Nakryiko Cc: Daniel Borkmann, Alexei Starovoitov, Martin KaFai Lau, Song Liu, Yonghong Song, Networking, bpf, open list, Andrii Nakryiko вт, 9 июл. 2019 г. в 13:40, Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > On Mon, Jul 8, 2019 at 1:37 PM Anton Protopopov > <a.s.protopopov@gmail.com> wrote: > > > > пн, 8 июл. 2019 г. в 13:54, Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > > > > > On Fri, Jul 5, 2019 at 2:53 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > > > > > On 07/05/2019 10:44 PM, Anton Protopopov wrote: > > > > > Add a new API bpf_object__reuse_maps() which can be used to replace all maps in > > > > > an object by maps pinned to a directory provided in the path argument. Namely, > > > > > each map M in the object will be replaced by a map pinned to path/M.name. > > > > > > > > > > Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> > > > > > --- > > > > > tools/lib/bpf/libbpf.c | 34 ++++++++++++++++++++++++++++++++++ > > > > > tools/lib/bpf/libbpf.h | 2 ++ > > > > > tools/lib/bpf/libbpf.map | 1 + > > > > > 3 files changed, 37 insertions(+) > > > > > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > > > index 4907997289e9..84c9e8f7bfd3 100644 > > > > > --- a/tools/lib/bpf/libbpf.c > > > > > +++ b/tools/lib/bpf/libbpf.c > > > > > @@ -3144,6 +3144,40 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path) > > > > > return 0; > > > > > } > > > > > > > > > > +int bpf_object__reuse_maps(struct bpf_object *obj, const char *path) > > > > > > As is, bpf_object__reuse_maps() can be easily implemented by user > > > applications, as it's only using public libbpf APIs, so I'm not 100% > > > sure we need to add method like that to libbpf. > > > > The bpf_object__reuse_maps() can definitely be implemented by user > > applications, however, to use it a user also needs to re-implement the > > bpf_prog_load_xattr funciton, so it seemed to me that adding this > > functionality to the library is a better way. > > I'm still not convinced. Looking at bpf_prog_load_xattr, I think some > of what it's doing should be part of bpf_object__object_xattr anyway > (all the expected type setting for programs). > > Besides that, there isn't much more than just bpf_object__open and > bpf_object__load, to be honest. By doing open and load explicitly, > user gets an opportunity to do whatever adjustment they need: reuse > maps, adjust map sizes, etc. So I think we should improve > bpf_object__open to "guess" program attach types and add map > definition flags to allow reuse declaratively. > > > > > > > > > > > > +{ > > > > > + struct bpf_map *map; > > > > > + > > > > > + if (!obj) > > > > > + return -ENOENT; > > > > > + > > > > > + if (!path) > > > > > + return -EINVAL; > > > > > + > > > > > + bpf_object__for_each_map(map, obj) { > > > > > + int len, err; > > > > > + int pinned_map_fd; > > > > > + char buf[PATH_MAX]; > > > > > > > > We'd need to skip the case of bpf_map__is_internal(map) since they are always > > > > recreated for the given object. > > > > > > > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map)); > > > > > + if (len < 0) { > > > > > + return -EINVAL; > > > > > + } else if (len >= PATH_MAX) { > > > > > + return -ENAMETOOLONG; > > > > > + } > > > > > + > > > > > + pinned_map_fd = bpf_obj_get(buf); > > > > > + if (pinned_map_fd < 0) > > > > > + return pinned_map_fd; > > > > > > > > Should we rather have a new map definition attribute that tells to reuse > > > > the map if it's pinned in bpf fs, and if not, we create it and later on > > > > pin it? This is what iproute2 is doing and which we're making use of heavily. > > > > > > I'd like something like that as well. This would play nicely with > > > recently added BTF-defined maps as well. > > > > > > I think it should be not just pin/don't pin flag, but rather pinning > > > strategy, to accommodate various typical strategies of handling maps > > > that are already pinned. So something like this: > > > > > > 1. BPF_PIN_NOTHING - default, don't pin; > > > 2. BPF_PIN_EXCLUSIVE - pin, but if map is already pinned - fail; > > > 3. BPF_PIN_SET - pin; if existing map exists, reset its state to be > > > exact state of object's map; > > > 4. BPF_PIN_MERGE - pin, if map exists, fill in NULL entries only (this > > > is how Cilium is pinning PROG_ARRAY maps, if I understand correctly); > > > 5. BPF_PIN_MERGE_OVERWRITE - pin, if map exists, overwrite non-NULL values. > > > > > > This list is only for illustrative purposes, ideally people that have > > > a lot of experience using pinning for real-world use cases would chime > > > in on what strategies are useful and make sense. > > > > My case was simply to reuse existing maps when reloading a program. > > Does it make sense for you to add only the simplest cases of listed above? > > Of course, it's enum, so we can start with few clearly useful ones and > then expand more if we ever have a need. But I think we still need a > bit wider discussion and let people who use pinning to chime in. > > > > > Also, libbpf doesn't use standard naming conventions for pinning maps. > > We talked about this in another thread related to BTF-defined maps. I > think the way to go with this is to actually define a default pinning > root path, but allow to override it on bpf_object__open, if user needs > a different one. > > > Does it make sense to provide a list of already open maps to the > > bpf_prog_load_xattr function as an attribute? In this case a user > > can execute his own policy on pinning, but still will have an option > > to reuse, reset, and merge maps. > > As explained above, I don't think there isn't much added value in > bpf_prog_load, so I'd advise to just switch to explicit > bpf_object__open + bpf_object__load and get maximum control and > flexibility. Thanks for your comments. I can see now that using bpf_object__open/bpf_object__load makes better sense. > > > > > > > > > > In bpf_object__reuse_maps() bailing out if bpf_obj_get() fails is perhaps > > > > too limiting for a generic API as new version of an object file may contain > > > > new maps which are not yet present in bpf fs at that point. > > > > > > > > > + err = bpf_map__reuse_fd(map, pinned_map_fd); > > > > > + if (err) > > > > > + return err; > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > int bpf_object__pin_programs(struct bpf_object *obj, const char *path) > > > > > { > > > > > struct bpf_program *prog; > > > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > > > > > index d639f47e3110..7fe465a1be76 100644 > > > > > --- a/tools/lib/bpf/libbpf.h > > > > > +++ b/tools/lib/bpf/libbpf.h > > > > > @@ -82,6 +82,8 @@ int bpf_object__variable_offset(const struct bpf_object *obj, const char *name, > > > > > LIBBPF_API int bpf_object__pin_maps(struct bpf_object *obj, const char *path); > > > > > LIBBPF_API int bpf_object__unpin_maps(struct bpf_object *obj, > > > > > const char *path); > > > > > +LIBBPF_API int bpf_object__reuse_maps(struct bpf_object *obj, > > > > > + const char *path); > > > > > LIBBPF_API int bpf_object__pin_programs(struct bpf_object *obj, > > > > > const char *path); > > > > > LIBBPF_API int bpf_object__unpin_programs(struct bpf_object *obj, > > > > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map > > > > > index 2c6d835620d2..66a30be6696c 100644 > > > > > --- a/tools/lib/bpf/libbpf.map > > > > > +++ b/tools/lib/bpf/libbpf.map > > > > > @@ -172,5 +172,6 @@ LIBBPF_0.0.4 { > > > > > btf_dump__new; > > > > > btf__parse_elf; > > > > > bpf_object__load_xattr; > > > > > + bpf_object__reuse_maps; > > > > > libbpf_num_possible_cpus; > > > > > } LIBBPF_0.0.3; > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH bpf-next 2/2] bpf, libbpf: add an option to reuse existing maps in bpf_prog_load_xattr 2019-07-05 20:40 [PATCH bpf-next 0/2] libbpf: add an option to reuse maps when loading a program Anton Protopopov 2019-07-05 20:44 ` [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() Anton Protopopov @ 2019-07-05 20:44 ` Anton Protopopov 1 sibling, 0 replies; 9+ messages in thread From: Anton Protopopov @ 2019-07-05 20:44 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, netdev, bpf, linux-kernel Cc: Anton Protopopov Add a new pinned_maps_path member to the bpf_prog_load_attr structure and extend the bpf_prog_load_xattr() function to pass this pointer to the new bpf_object__reuse_maps() helper. This change provides users with a simple way to use existing pinned maps when (re)loading BPF programs. Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/lib/bpf/libbpf.c | 8 ++++++++ tools/lib/bpf/libbpf.h | 1 + 2 files changed, 9 insertions(+) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 84c9e8f7bfd3..9daa09c9fe1a 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -3953,6 +3953,14 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr, first_prog = prog; } + if (attr->pinned_maps_path) { + err = bpf_object__reuse_maps(obj, attr->pinned_maps_path); + if (err < 0) { + bpf_object__close(obj); + return err; + } + } + bpf_object__for_each_map(map, obj) { if (!bpf_map__is_offload_neutral(map)) map->map_ifindex = attr->ifindex; diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index 7fe465a1be76..6bf405bb9c1f 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -329,6 +329,7 @@ struct bpf_prog_load_attr { int ifindex; int log_level; int prog_flags; + const char *pinned_maps_path; }; LIBBPF_API int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr, -- 2.19.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-16 17:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-05 20:40 [PATCH bpf-next 0/2] libbpf: add an option to reuse maps when loading a program Anton Protopopov 2019-07-05 20:44 ` [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps() Anton Protopopov 2019-07-05 21:44 ` Daniel Borkmann 2019-07-08 15:11 ` Anton Protopopov 2019-07-08 17:54 ` Andrii Nakryiko 2019-07-08 20:37 ` Anton Protopopov 2019-07-09 17:40 ` Andrii Nakryiko 2019-07-16 17:14 ` Anton Protopopov 2019-07-05 20:44 ` [PATCH bpf-next 2/2] bpf, libbpf: add an option to reuse existing maps in bpf_prog_load_xattr Anton Protopopov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).