[PATCH bpf-next 0/2] libbpf: add an option to reuse maps when loading a program

[PATCH bpf-next 0/2] libbpf: add an option to reuse maps when loading a program

[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

[PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

[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

[PATCH bpf-next 2/2] bpf, libbpf: add an option to reuse existing maps in bpf_prog_load_xattr

[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

Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

[Daniel Borkmann]

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

Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

[Anton Protopopov]

пт, 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;
> >
>

Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

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

Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

[Anton Protopopov]

пн, 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;
> > >
> >

Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

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

Re: [PATCH bpf-next 1/2] bpf, libbpf: add a new API bpf_object__reuse_maps()

[Anton Protopopov]

вт, 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;
> > > > >
> > > >