[PATCH bpf-next 0/4] bpf: Generate helpers for pinning through bpf object skeleton

[PATCH bpf-next 0/4] bpf: Generate helpers for pinning through bpf object skeleton

[Yafang Shao]

Currently there're helpers for allowing to open/load/attach BPF object
through BPF object skeleton. Let's also add helpers for pinning through
BPF object skeleton. It could simplify BPF userspace code which wants to
pin the progs into bpffs.

After this change, with command 'bpftool gen skeleton XXX.bpf.o', the
helpers for pinning BPF prog will be generated in BPF object skeleton.

The new helpers are named with __{pin, unpin}_prog, because it only pins
bpf progs. If the user also wants to pin bpf maps, he can use
LIBBPF_PIN_BY_NAME.

Yafang Shao (4):
  libbpf: Define DEFAULT_BPFFS
  libbpf: Add helpers for pinning bpf prog through bpf object skeleton
  bpftool: Fix incorrect return in generated detach helper
  bpftool: Generate helpers for pinning prog through bpf object skeleton

 tools/bpf/bpftool/gen.c     | 18 ++++++++++-
 tools/lib/bpf/bpf_helpers.h |  2 +-
 tools/lib/bpf/libbpf.c      | 61 ++++++++++++++++++++++++++++++++++++-
 tools/lib/bpf/libbpf.h      | 10 ++++--
 tools/lib/bpf/libbpf.map    |  2 ++
 5 files changed, 88 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH bpf-next 1/4] libbpf: Define DEFAULT_BPFFS

[Yafang Shao]

Let's use a macro DEFAULT_BPFFS instead of the hard-coded "/sys/fs/bpf".

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/lib/bpf/bpf_helpers.h | 2 +-
 tools/lib/bpf/libbpf.c      | 2 +-
 tools/lib/bpf/libbpf.h      | 6 ++++--
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
index 44df982d2a5c..9161ebcd3466 100644
--- a/tools/lib/bpf/bpf_helpers.h
+++ b/tools/lib/bpf/bpf_helpers.h
@@ -137,7 +137,7 @@ struct bpf_map_def {
 
 enum libbpf_pin_type {
 	LIBBPF_PIN_NONE,
-	/* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
+	/* PIN_BY_NAME: pin maps by name (in DEFAULT_BPFFS by default) */
 	LIBBPF_PIN_BY_NAME,
 };
 
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 9a213aaaac8a..13fcf91e9e0e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2180,7 +2180,7 @@ static int build_map_pin_path(struct bpf_map *map, const char *path)
 	int len;
 
 	if (!path)
-		path = "/sys/fs/bpf";
+		path = DEFAULT_BPFFS;
 
 	len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
 	if (len < 0)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index cdbfee60ea3e..3784867811a4 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -28,6 +28,8 @@ LIBBPF_API __u32 libbpf_major_version(void);
 LIBBPF_API __u32 libbpf_minor_version(void);
 LIBBPF_API const char *libbpf_version_string(void);
 
+#define DEFAULT_BPFFS "/sys/fs/bpf"
+
 enum libbpf_errno {
 	__LIBBPF_ERRNO__START = 4000,
 
@@ -91,7 +93,7 @@ struct bpf_object_open_opts {
 	bool relaxed_core_relocs;
 	/* maps that set the 'pinning' attribute in their definition will have
 	 * their pin_path attribute set to a file in this directory, and be
-	 * auto-pinned to that path on load; defaults to "/sys/fs/bpf".
+	 * auto-pinned to that path on load; defaults to DEFAULT_BPFFS.
 	 */
 	const char *pin_root_path;
 
@@ -190,7 +192,7 @@ bpf_object__open_xattr(struct bpf_object_open_attr *attr);
 
 enum libbpf_pin_type {
 	LIBBPF_PIN_NONE,
-	/* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
+	/* PIN_BY_NAME: pin maps by name (in DEFAULT_BPFFS by default) */
 	LIBBPF_PIN_BY_NAME,
 };
 
-- 
2.17.1

Re: [PATCH bpf-next 1/4] libbpf: Define DEFAULT_BPFFS

[Daniel Borkmann]

On 4/23/22 4:00 PM, Yafang Shao wrote:
> Let's use a macro DEFAULT_BPFFS instead of the hard-coded "/sys/fs/bpf".
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
[...]
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index cdbfee60ea3e..3784867811a4 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -28,6 +28,8 @@ LIBBPF_API __u32 libbpf_major_version(void);
>   LIBBPF_API __u32 libbpf_minor_version(void);
>   LIBBPF_API const char *libbpf_version_string(void);
>   
> +#define DEFAULT_BPFFS "/sys/fs/bpf"

Small nit, but given this is included in a lot of places potentially, it should
have a LIBBPF_ prefix to avoid collisions. Maybe: LIBBPF_BPFFS_DEFAULT_MNT

>   enum libbpf_errno {
>   	__LIBBPF_ERRNO__START = 4000,
>   
> @@ -91,7 +93,7 @@ struct bpf_object_open_opts {
>   	bool relaxed_core_relocs;
>   	/* maps that set the 'pinning' attribute in their definition will have
>   	 * their pin_path attribute set to a file in this directory, and be
> -	 * auto-pinned to that path on load; defaults to "/sys/fs/bpf".
> +	 * auto-pinned to that path on load; defaults to DEFAULT_BPFFS.
>   	 */
>   	const char *pin_root_path;
>   
> @@ -190,7 +192,7 @@ bpf_object__open_xattr(struct bpf_object_open_attr *attr);
>   
>   enum libbpf_pin_type {
>   	LIBBPF_PIN_NONE,
> -	/* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> +	/* PIN_BY_NAME: pin maps by name (in DEFAULT_BPFFS by default) */
>   	LIBBPF_PIN_BY_NAME,
>   };
>   
> 

Re: [PATCH bpf-next 1/4] libbpf: Define DEFAULT_BPFFS

[Andrii Nakryiko]

On Sat, Apr 23, 2022 at 7:01 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Let's use a macro DEFAULT_BPFFS instead of the hard-coded "/sys/fs/bpf".
>
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  tools/lib/bpf/bpf_helpers.h | 2 +-
>  tools/lib/bpf/libbpf.c      | 2 +-
>  tools/lib/bpf/libbpf.h      | 6 ++++--
>  3 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> index 44df982d2a5c..9161ebcd3466 100644
> --- a/tools/lib/bpf/bpf_helpers.h
> +++ b/tools/lib/bpf/bpf_helpers.h
> @@ -137,7 +137,7 @@ struct bpf_map_def {
>
>  enum libbpf_pin_type {
>         LIBBPF_PIN_NONE,
> -       /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> +       /* PIN_BY_NAME: pin maps by name (in DEFAULT_BPFFS by default) */

how is this improving things? now I need to grep some more to find out
what's the value of DEFAULT_BPFFS is


>         LIBBPF_PIN_BY_NAME,
>  };
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 9a213aaaac8a..13fcf91e9e0e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2180,7 +2180,7 @@ static int build_map_pin_path(struct bpf_map *map, const char *path)
>         int len;
>
>         if (!path)
> -               path = "/sys/fs/bpf";
> +               path = DEFAULT_BPFFS;
>
>         len = snprintf(buf, PATH_MAX, "%s/%s", path, bpf_map__name(map));
>         if (len < 0)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index cdbfee60ea3e..3784867811a4 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -28,6 +28,8 @@ LIBBPF_API __u32 libbpf_major_version(void);
>  LIBBPF_API __u32 libbpf_minor_version(void);
>  LIBBPF_API const char *libbpf_version_string(void);
>
> +#define DEFAULT_BPFFS "/sys/fs/bpf"
> +
>  enum libbpf_errno {
>         __LIBBPF_ERRNO__START = 4000,
>
> @@ -91,7 +93,7 @@ struct bpf_object_open_opts {
>         bool relaxed_core_relocs;
>         /* maps that set the 'pinning' attribute in their definition will have
>          * their pin_path attribute set to a file in this directory, and be
> -        * auto-pinned to that path on load; defaults to "/sys/fs/bpf".
> +        * auto-pinned to that path on load; defaults to DEFAULT_BPFFS.
>          */
>         const char *pin_root_path;
>
> @@ -190,7 +192,7 @@ bpf_object__open_xattr(struct bpf_object_open_attr *attr);
>
>  enum libbpf_pin_type {
>         LIBBPF_PIN_NONE,
> -       /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> +       /* PIN_BY_NAME: pin maps by name (in DEFAULT_BPFFS by default) */
>         LIBBPF_PIN_BY_NAME,
>  };
>
> --
> 2.17.1
>

Re: [PATCH bpf-next 1/4] libbpf: Define DEFAULT_BPFFS

[Yafang Shao]

On Tue, Apr 26, 2022 at 2:45 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 23, 2022 at 7:01 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Let's use a macro DEFAULT_BPFFS instead of the hard-coded "/sys/fs/bpf".
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  tools/lib/bpf/bpf_helpers.h | 2 +-
> >  tools/lib/bpf/libbpf.c      | 2 +-
> >  tools/lib/bpf/libbpf.h      | 6 ++++--
> >  3 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf_helpers.h b/tools/lib/bpf/bpf_helpers.h
> > index 44df982d2a5c..9161ebcd3466 100644
> > --- a/tools/lib/bpf/bpf_helpers.h
> > +++ b/tools/lib/bpf/bpf_helpers.h
> > @@ -137,7 +137,7 @@ struct bpf_map_def {
> >
> >  enum libbpf_pin_type {
> >         LIBBPF_PIN_NONE,
> > -       /* PIN_BY_NAME: pin maps by name (in /sys/fs/bpf by default) */
> > +       /* PIN_BY_NAME: pin maps by name (in DEFAULT_BPFFS by default) */
>
> how is this improving things? now I need to grep some more to find out
> what's the value of DEFAULT_BPFFS is
>

The new added one also uses the "/sys/fs/bpf", so I defined a macro
for them, then they can be kept the same.
I won't change it if you object to it.

[snip]

-- 
Regards
Yafang

[PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Yafang Shao]

Currently there're helpers for allowing to open/load/attach BPF object
through BPF object skeleton. Let's also add helpers for pinning through
BPF object skeleton. It could simplify BPF userspace code which wants to
pin the progs into bpffs.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/lib/bpf/libbpf.c   | 59 ++++++++++++++++++++++++++++++++++++++++
 tools/lib/bpf/libbpf.h   |  4 +++
 tools/lib/bpf/libbpf.map |  2 ++
 3 files changed, 65 insertions(+)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 13fcf91e9e0e..e7ed6c53c525 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -12726,6 +12726,65 @@ void bpf_object__detach_skeleton(struct bpf_object_skeleton *s)
 	}
 }
 
+int bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s,
+				  const char *path)
+{
+	struct bpf_link *link;
+	int err;
+	int i;
+
+	if (!s->prog_cnt)
+		return libbpf_err(-EINVAL);
+
+	if (!path)
+		path = DEFAULT_BPFFS;
+
+	for (i = 0; i < s->prog_cnt; i++) {
+		char buf[PATH_MAX];
+		int len;
+
+		len = snprintf(buf, PATH_MAX, "%s/%s", path, s->progs[i].name);
+		if (len < 0) {
+			err = -EINVAL;
+			goto err_unpin_prog;
+		} else if (len >= PATH_MAX) {
+			err = -ENAMETOOLONG;
+			goto err_unpin_prog;
+		}
+
+		link = *s->progs[i].link;
+		if (!link) {
+			err = -EINVAL;
+			goto err_unpin_prog;
+		}
+
+		err = bpf_link__pin(link, buf);
+		if (err)
+			goto err_unpin_prog;
+	}
+
+	return 0;
+
+err_unpin_prog:
+	bpf_object__unpin_skeleton_prog(s);
+
+	return libbpf_err(err);
+}
+
+void bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s)
+{
+	struct bpf_link *link;
+	int i;
+
+	for (i = 0; i < s->prog_cnt; i++) {
+		link = *s->progs[i].link;
+		if (!link || !link->pin_path)
+			continue;
+
+		bpf_link__unpin(link);
+	}
+}
+
 void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
 {
 	if (!s)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 3784867811a4..af44b0968cca 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -1427,6 +1427,10 @@ bpf_object__open_skeleton(struct bpf_object_skeleton *s,
 LIBBPF_API int bpf_object__load_skeleton(struct bpf_object_skeleton *s);
 LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
 LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
+LIBBPF_API int
+bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s, const char *path);
+LIBBPF_API void
+bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s);
 LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
 
 struct bpf_var_skeleton {
diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index 82f6d62176dd..4e3e37b84b3a 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -55,6 +55,8 @@ LIBBPF_0.0.1 {
 		bpf_object__unload;
 		bpf_object__unpin_maps;
 		bpf_object__unpin_programs;
+		bpf_object__pin_skeleton_prog;
+		bpf_object__unpin_skeleton_prog;
 		bpf_perf_event_read_simple;
 		bpf_prog_attach;
 		bpf_prog_detach;
-- 
2.17.1

Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Daniel Borkmann]

On 4/23/22 4:00 PM, Yafang Shao wrote:
> Currently there're helpers for allowing to open/load/attach BPF object
> through BPF object skeleton. Let's also add helpers for pinning through
> BPF object skeleton. It could simplify BPF userspace code which wants to
> pin the progs into bpffs.

Please elaborate some more on your use case/rationale for the commit message,
do you have orchestration code that will rely on these specifically?

> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>   tools/lib/bpf/libbpf.c   | 59 ++++++++++++++++++++++++++++++++++++++++
>   tools/lib/bpf/libbpf.h   |  4 +++
>   tools/lib/bpf/libbpf.map |  2 ++
>   3 files changed, 65 insertions(+)
> 
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 13fcf91e9e0e..e7ed6c53c525 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -12726,6 +12726,65 @@ void bpf_object__detach_skeleton(struct bpf_object_skeleton *s)
>   	}
>   }
>   
> +int bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s,
> +				  const char *path)

These pin the link, not the prog itself, so the func name is a bit misleading? Also,
what if is this needs to be more customized in future? It doesn't seem very generic.

> +{
> +	struct bpf_link *link;
> +	int err;
> +	int i;
> +
> +	if (!s->prog_cnt)
> +		return libbpf_err(-EINVAL);
> +
> +	if (!path)
> +		path = DEFAULT_BPFFS;
> +
> +	for (i = 0; i < s->prog_cnt; i++) {
> +		char buf[PATH_MAX];
> +		int len;
> +
> +		len = snprintf(buf, PATH_MAX, "%s/%s", path, s->progs[i].name);
> +		if (len < 0) {
> +			err = -EINVAL;
> +			goto err_unpin_prog;
> +		} else if (len >= PATH_MAX) {
> +			err = -ENAMETOOLONG;
> +			goto err_unpin_prog;
> +		}
> +
> +		link = *s->progs[i].link;
> +		if (!link) {
> +			err = -EINVAL;
> +			goto err_unpin_prog;
> +		}
> +
> +		err = bpf_link__pin(link, buf);
> +		if (err)
> +			goto err_unpin_prog;
> +	}
> +
> +	return 0;
> +
> +err_unpin_prog:
> +	bpf_object__unpin_skeleton_prog(s);
> +
> +	return libbpf_err(err);
> +}
> +
> +void bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s)
> +{
> +	struct bpf_link *link;
> +	int i;
> +
> +	for (i = 0; i < s->prog_cnt; i++) {
> +		link = *s->progs[i].link;
> +		if (!link || !link->pin_path)
> +			continue;
> +
> +		bpf_link__unpin(link);
> +	}
> +}
> +
>   void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
>   {
>   	if (!s)
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 3784867811a4..af44b0968cca 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -1427,6 +1427,10 @@ bpf_object__open_skeleton(struct bpf_object_skeleton *s,
>   LIBBPF_API int bpf_object__load_skeleton(struct bpf_object_skeleton *s);
>   LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
>   LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
> +LIBBPF_API int
> +bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s, const char *path);
> +LIBBPF_API void
> +bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s);
>   LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);

Please also add API documentation.

>   struct bpf_var_skeleton {
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 82f6d62176dd..4e3e37b84b3a 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -55,6 +55,8 @@ LIBBPF_0.0.1 {
>   		bpf_object__unload;
>   		bpf_object__unpin_maps;
>   		bpf_object__unpin_programs;
> +		bpf_object__pin_skeleton_prog;
> +		bpf_object__unpin_skeleton_prog;

This would have to go under LIBBPF_0.8.0 if so.

>   		bpf_perf_event_read_simple;
>   		bpf_prog_attach;
>   		bpf_prog_detach;
> 

Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Yafang Shao]

On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 4/23/22 4:00 PM, Yafang Shao wrote:
> > Currently there're helpers for allowing to open/load/attach BPF object
> > through BPF object skeleton. Let's also add helpers for pinning through
> > BPF object skeleton. It could simplify BPF userspace code which wants to
> > pin the progs into bpffs.
>
> Please elaborate some more on your use case/rationale for the commit message,
> do you have orchestration code that will rely on these specifically?
>

We have a bpf manager on our production environment to maintain the
bpf programs, some of which need to be pinned in bpffs, for example
tracing bpf programs, perf_event programs and other bpf hooks added by
ourselves for performance tuning.  These bpf programs don't need a
user agent, while they really work like a kernel module, that is why
we pin them. For these kinds of bpf programs, the bpf manager can help
to simplify the development and deployment.  Take the improvement on
development for example,  the user doesn't need to write userspace
code while he focuses on the kernel side only, and then bpf manager
will do all the other things. Below is a simple example,
   Step1, gen the skeleton for the user provided bpf object file,
              $ bpftool gen skeleton  test.bpf.o > simple.skel.h
   Step2, Compile the bpf object file into a runnable binary
              #include "simple.skel.h"

              #define SIMPLE_BPF_PIN(name, path)  \
              ({                                                              \
                  struct name##_bpf *obj;                      \
                  int err = 0;                                            \
                                                                              \
                  obj = name##_bpf__open();                \
                   if (!obj) {                                              \
                       err = -errno;                                    \
                       goto cleanup;                                 \
                    }                                                         \
                                                                              \
                    err = name##_bpf__load(obj);           \
                    if (err)                                                 \
                        goto cleanup;                                 \
                                                                               \
                     err = name##_bpf__attach(obj);       \
                     if (err)                                                \
                         goto cleanup;                                \
                                                                               \
                     err = name##_bpf__pin_prog(obj, path);      \
                     if (err)                                                \
                         goto cleanup;                                \
                                                                              \
                      goto end;                                         \
                                                                              \
                  cleanup:                                              \
                      name##_bpf__destroy(obj);            \
                  end:                                                     \
                      err;                                                  \
                   })

                   SIMPLE_BPF_PIN(test, "/sys/fs/bpf");

               As the userspace code of FD-based bpf objects are all
the same,  so we can abstract them as above.  The pathset means to add
the non-exist "name##_bpf__pin_prog(obj, path)" for it.

> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >   tools/lib/bpf/libbpf.c   | 59 ++++++++++++++++++++++++++++++++++++++++
> >   tools/lib/bpf/libbpf.h   |  4 +++
> >   tools/lib/bpf/libbpf.map |  2 ++
> >   3 files changed, 65 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 13fcf91e9e0e..e7ed6c53c525 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -12726,6 +12726,65 @@ void bpf_object__detach_skeleton(struct bpf_object_skeleton *s)
> >       }
> >   }
> >
> > +int bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s,
> > +                               const char *path)
>
> These pin the link, not the prog itself, so the func name is a bit misleading? Also,
> what if is this needs to be more customized in future? It doesn't seem very generic.
>

Ah, it should be bpf_object__pin_skeleton_link().
I'm not sure if it can be extended to work for a non-auto-detachable
bpf program, but I know it is hard for pinning tc-bpf programs, which
is also running on our production environment, in this way.

> > +{
> > +     struct bpf_link *link;
> > +     int err;
> > +     int i;
> > +
> > +     if (!s->prog_cnt)
> > +             return libbpf_err(-EINVAL);
> > +
> > +     if (!path)
> > +             path = DEFAULT_BPFFS;
> > +
> > +     for (i = 0; i < s->prog_cnt; i++) {
> > +             char buf[PATH_MAX];
> > +             int len;
> > +
> > +             len = snprintf(buf, PATH_MAX, "%s/%s", path, s->progs[i].name);
> > +             if (len < 0) {
> > +                     err = -EINVAL;
> > +                     goto err_unpin_prog;
> > +             } else if (len >= PATH_MAX) {
> > +                     err = -ENAMETOOLONG;
> > +                     goto err_unpin_prog;
> > +             }
> > +
> > +             link = *s->progs[i].link;
> > +             if (!link) {
> > +                     err = -EINVAL;
> > +                     goto err_unpin_prog;
> > +             }
> > +
> > +             err = bpf_link__pin(link, buf);
> > +             if (err)
> > +                     goto err_unpin_prog;
> > +     }
> > +
> > +     return 0;
> > +
> > +err_unpin_prog:
> > +     bpf_object__unpin_skeleton_prog(s);
> > +
> > +     return libbpf_err(err);
> > +}
> > +
> > +void bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s)
> > +{
> > +     struct bpf_link *link;
> > +     int i;
> > +
> > +     for (i = 0; i < s->prog_cnt; i++) {
> > +             link = *s->progs[i].link;
> > +             if (!link || !link->pin_path)
> > +                     continue;
> > +
> > +             bpf_link__unpin(link);
> > +     }
> > +}
> > +
> >   void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s)
> >   {
> >       if (!s)
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 3784867811a4..af44b0968cca 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -1427,6 +1427,10 @@ bpf_object__open_skeleton(struct bpf_object_skeleton *s,
> >   LIBBPF_API int bpf_object__load_skeleton(struct bpf_object_skeleton *s);
> >   LIBBPF_API int bpf_object__attach_skeleton(struct bpf_object_skeleton *s);
> >   LIBBPF_API void bpf_object__detach_skeleton(struct bpf_object_skeleton *s);
> > +LIBBPF_API int
> > +bpf_object__pin_skeleton_prog(struct bpf_object_skeleton *s, const char *path);
> > +LIBBPF_API void
> > +bpf_object__unpin_skeleton_prog(struct bpf_object_skeleton *s);
> >   LIBBPF_API void bpf_object__destroy_skeleton(struct bpf_object_skeleton *s);
>
> Please also add API documentation.
>

Sure.

> >   struct bpf_var_skeleton {
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index 82f6d62176dd..4e3e37b84b3a 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -55,6 +55,8 @@ LIBBPF_0.0.1 {
> >               bpf_object__unload;
> >               bpf_object__unpin_maps;
> >               bpf_object__unpin_programs;
> > +             bpf_object__pin_skeleton_prog;
> > +             bpf_object__unpin_skeleton_prog;
>
> This would have to go under LIBBPF_0.8.0 if so.
>

Thanks, I will change it.

> >               bpf_perf_event_read_simple;
> >               bpf_prog_attach;
> >               bpf_prog_detach;
> >
>

-- 
Regards
Yafang

Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Andrii Nakryiko]

On Tue, Apr 26, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 4/23/22 4:00 PM, Yafang Shao wrote:
> > > Currently there're helpers for allowing to open/load/attach BPF object
> > > through BPF object skeleton. Let's also add helpers for pinning through
> > > BPF object skeleton. It could simplify BPF userspace code which wants to
> > > pin the progs into bpffs.
> >
> > Please elaborate some more on your use case/rationale for the commit message,
> > do you have orchestration code that will rely on these specifically?
> >
>
> We have a bpf manager on our production environment to maintain the
> bpf programs, some of which need to be pinned in bpffs, for example
> tracing bpf programs, perf_event programs and other bpf hooks added by
> ourselves for performance tuning.  These bpf programs don't need a
> user agent, while they really work like a kernel module, that is why
> we pin them. For these kinds of bpf programs, the bpf manager can help
> to simplify the development and deployment.  Take the improvement on
> development for example,  the user doesn't need to write userspace
> code while he focuses on the kernel side only, and then bpf manager
> will do all the other things. Below is a simple example,
>    Step1, gen the skeleton for the user provided bpf object file,
>               $ bpftool gen skeleton  test.bpf.o > simple.skel.h
>    Step2, Compile the bpf object file into a runnable binary
>               #include "simple.skel.h"
>
>               #define SIMPLE_BPF_PIN(name, path)  \
>               ({                                                              \
>                   struct name##_bpf *obj;                      \
>                   int err = 0;                                            \
>                                                                               \
>                   obj = name##_bpf__open();                \
>                    if (!obj) {                                              \
>                        err = -errno;                                    \
>                        goto cleanup;                                 \
>                     }                                                         \
>                                                                               \
>                     err = name##_bpf__load(obj);           \
>                     if (err)                                                 \
>                         goto cleanup;                                 \
>                                                                                \
>                      err = name##_bpf__attach(obj);       \
>                      if (err)                                                \
>                          goto cleanup;                                \
>                                                                                \
>                      err = name##_bpf__pin_prog(obj, path);      \
>                      if (err)                                                \
>                          goto cleanup;                                \
>                                                                               \
>                       goto end;                                         \
>                                                                               \
>                   cleanup:                                              \
>                       name##_bpf__destroy(obj);            \
>                   end:                                                     \
>                       err;                                                  \
>                    })
>
>                    SIMPLE_BPF_PIN(test, "/sys/fs/bpf");
>
>                As the userspace code of FD-based bpf objects are all
> the same,  so we can abstract them as above.  The pathset means to add
> the non-exist "name##_bpf__pin_prog(obj, path)" for it.
>

Your BPF manager is user-space code that you control, right? I'm not
sure how skeleton is helpful here given your BPF manager is generic
and doesn't work with any specific skeleton, if I understand the idea.
But let's assume that you use skeleton to also embed BPF ELF bytes and
pass them to your manager for "activation". Once you open and load
bpf_object, your BPF manager can generically iterate all BPF programs
using bpf_object_for_each_program(), attempt to attach them with
bpf_program__attach() (see how bpf_object__attach_skeleton is handling
non-auto-attachable programs) and immediately pin the link (no need to
even store it, you can destroy it after pinning immediately). All this
is using generic libbpf APIs and requires no code generation. But keep
in mind that not all struct bpf_link in libbpf are pinnable (not all
links have FD-based BPF link in kernel associated with them), so
you'll have to deal with that somehow (and what you didn't do in this
patch for libbpf implementation).

> > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > > ---
> > >   tools/lib/bpf/libbpf.c   | 59 ++++++++++++++++++++++++++++++++++++++++
> > >   tools/lib/bpf/libbpf.h   |  4 +++
> > >   tools/lib/bpf/libbpf.map |  2 ++
> > >   3 files changed, 65 insertions(+)
> > >

[...]

Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Yafang Shao]

On Wed, Apr 27, 2022 at 7:16 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Apr 26, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > >
> > > On 4/23/22 4:00 PM, Yafang Shao wrote:
> > > > Currently there're helpers for allowing to open/load/attach BPF object
> > > > through BPF object skeleton. Let's also add helpers for pinning through
> > > > BPF object skeleton. It could simplify BPF userspace code which wants to
> > > > pin the progs into bpffs.
> > >
> > > Please elaborate some more on your use case/rationale for the commit message,
> > > do you have orchestration code that will rely on these specifically?
> > >
> >
> > We have a bpf manager on our production environment to maintain the
> > bpf programs, some of which need to be pinned in bpffs, for example
> > tracing bpf programs, perf_event programs and other bpf hooks added by
> > ourselves for performance tuning.  These bpf programs don't need a
> > user agent, while they really work like a kernel module, that is why
> > we pin them. For these kinds of bpf programs, the bpf manager can help
> > to simplify the development and deployment.  Take the improvement on
> > development for example,  the user doesn't need to write userspace
> > code while he focuses on the kernel side only, and then bpf manager
> > will do all the other things. Below is a simple example,
> >    Step1, gen the skeleton for the user provided bpf object file,
> >               $ bpftool gen skeleton  test.bpf.o > simple.skel.h
> >    Step2, Compile the bpf object file into a runnable binary
> >               #include "simple.skel.h"
> >
> >               #define SIMPLE_BPF_PIN(name, path)  \
> >               ({                                                              \
> >                   struct name##_bpf *obj;                      \
> >                   int err = 0;                                            \
> >                                                                               \
> >                   obj = name##_bpf__open();                \
> >                    if (!obj) {                                              \
> >                        err = -errno;                                    \
> >                        goto cleanup;                                 \
> >                     }                                                         \
> >                                                                               \
> >                     err = name##_bpf__load(obj);           \
> >                     if (err)                                                 \
> >                         goto cleanup;                                 \
> >                                                                                \
> >                      err = name##_bpf__attach(obj);       \
> >                      if (err)                                                \
> >                          goto cleanup;                                \
> >                                                                                \
> >                      err = name##_bpf__pin_prog(obj, path);      \
> >                      if (err)                                                \
> >                          goto cleanup;                                \
> >                                                                               \
> >                       goto end;                                         \
> >                                                                               \
> >                   cleanup:                                              \
> >                       name##_bpf__destroy(obj);            \
> >                   end:                                                     \
> >                       err;                                                  \
> >                    })
> >
> >                    SIMPLE_BPF_PIN(test, "/sys/fs/bpf");
> >
> >                As the userspace code of FD-based bpf objects are all
> > the same,  so we can abstract them as above.  The pathset means to add
> > the non-exist "name##_bpf__pin_prog(obj, path)" for it.
> >
>
> Your BPF manager is user-space code that you control, right? I'm not
> sure how skeleton is helpful here given your BPF manager is generic
> and doesn't work with any specific skeleton, if I understand the idea.
> But let's assume that you use skeleton to also embed BPF ELF bytes and
> pass them to your manager for "activation". Once you open and load
> bpf_object, your BPF manager can generically iterate all BPF programs
> using bpf_object_for_each_program(), attempt to attach them with
> bpf_program__attach() (see how bpf_object__attach_skeleton is handling
> non-auto-attachable programs) and immediately pin the link (no need to
> even store it, you can destroy it after pinning immediately). All this
> is using generic libbpf APIs and requires no code generation.

Many thanks for the detailed explanation. Your suggestion can also
work, but with the skeleton we can also generate a binary which can
run independently.  (Technically speaking, the binary is the same as
'./bpf_install target.bpf.o').

>  But keep
> in mind that not all struct bpf_link in libbpf are pinnable (not all
> links have FD-based BPF link in kernel associated with them), so
> you'll have to deal with that somehow (and what you didn't do in this
> patch for libbpf implementation).
>

Right, I have found it. If I understand it correctly, only the link
types defined in enum bpf_link_type (which is in
include/uapi/linux/bpf.h) are pinnable, right?

BTW, is it possible to support pinning all struct bpf_link in libbpf ?


--
Regards
Yafang

Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Yafang Shao]

On Wed, Apr 27, 2022 at 10:45 PM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, Apr 27, 2022 at 7:16 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Tue, Apr 26, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > >
> > > On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > >
> > > > On 4/23/22 4:00 PM, Yafang Shao wrote:
> > > > > Currently there're helpers for allowing to open/load/attach BPF object
> > > > > through BPF object skeleton. Let's also add helpers for pinning through
> > > > > BPF object skeleton. It could simplify BPF userspace code which wants to
> > > > > pin the progs into bpffs.
> > > >
> > > > Please elaborate some more on your use case/rationale for the commit message,
> > > > do you have orchestration code that will rely on these specifically?
> > > >
> > >
> > > We have a bpf manager on our production environment to maintain the
> > > bpf programs, some of which need to be pinned in bpffs, for example
> > > tracing bpf programs, perf_event programs and other bpf hooks added by
> > > ourselves for performance tuning.  These bpf programs don't need a
> > > user agent, while they really work like a kernel module, that is why
> > > we pin them. For these kinds of bpf programs, the bpf manager can help
> > > to simplify the development and deployment.  Take the improvement on
> > > development for example,  the user doesn't need to write userspace
> > > code while he focuses on the kernel side only, and then bpf manager
> > > will do all the other things. Below is a simple example,
> > >    Step1, gen the skeleton for the user provided bpf object file,
> > >               $ bpftool gen skeleton  test.bpf.o > simple.skel.h
> > >    Step2, Compile the bpf object file into a runnable binary
> > >               #include "simple.skel.h"
> > >
> > >               #define SIMPLE_BPF_PIN(name, path)  \
> > >               ({                                                              \
> > >                   struct name##_bpf *obj;                      \
> > >                   int err = 0;                                            \
> > >                                                                               \
> > >                   obj = name##_bpf__open();                \
> > >                    if (!obj) {                                              \
> > >                        err = -errno;                                    \
> > >                        goto cleanup;                                 \
> > >                     }                                                         \
> > >                                                                               \
> > >                     err = name##_bpf__load(obj);           \
> > >                     if (err)                                                 \
> > >                         goto cleanup;                                 \
> > >                                                                                \
> > >                      err = name##_bpf__attach(obj);       \
> > >                      if (err)                                                \
> > >                          goto cleanup;                                \
> > >                                                                                \
> > >                      err = name##_bpf__pin_prog(obj, path);      \
> > >                      if (err)                                                \
> > >                          goto cleanup;                                \
> > >                                                                               \
> > >                       goto end;                                         \
> > >                                                                               \
> > >                   cleanup:                                              \
> > >                       name##_bpf__destroy(obj);            \
> > >                   end:                                                     \
> > >                       err;                                                  \
> > >                    })
> > >
> > >                    SIMPLE_BPF_PIN(test, "/sys/fs/bpf");
> > >
> > >                As the userspace code of FD-based bpf objects are all
> > > the same,  so we can abstract them as above.  The pathset means to add
> > > the non-exist "name##_bpf__pin_prog(obj, path)" for it.
> > >
> >
> > Your BPF manager is user-space code that you control, right? I'm not
> > sure how skeleton is helpful here given your BPF manager is generic
> > and doesn't work with any specific skeleton, if I understand the idea.
> > But let's assume that you use skeleton to also embed BPF ELF bytes and
> > pass them to your manager for "activation". Once you open and load
> > bpf_object, your BPF manager can generically iterate all BPF programs
> > using bpf_object_for_each_program(), attempt to attach them with
> > bpf_program__attach() (see how bpf_object__attach_skeleton is handling
> > non-auto-attachable programs) and immediately pin the link (no need to
> > even store it, you can destroy it after pinning immediately). All this
> > is using generic libbpf APIs and requires no code generation.
>
> Many thanks for the detailed explanation. Your suggestion can also
> work, but with the skeleton we can also generate a binary which can
> run independently.  (Technically speaking, the binary is the same as
> './bpf_install target.bpf.o').
>

Forgot to mention that with skeleton we can also modify the global
data defined in bpf object file, that may need to be abstracted as a
new common helper.  The bpf_object__* functions can't do it, right ?

> >  But keep
> > in mind that not all struct bpf_link in libbpf are pinnable (not all
> > links have FD-based BPF link in kernel associated with them), so
> > you'll have to deal with that somehow (and what you didn't do in this
> > patch for libbpf implementation).
> >
>
> Right, I have found it. If I understand it correctly, only the link
> types defined in enum bpf_link_type (which is in
> include/uapi/linux/bpf.h) are pinnable, right?
>
> BTW, is it possible to support pinning all struct bpf_link in libbpf ?
>
>
> --
> Regards
> Yafang



-- 
Regards
Yafang

Re: [PATCH bpf-next 2/4] libbpf: Add helpers for pinning bpf prog through bpf object skeleton

[Andrii Nakryiko]

On Wed, Apr 27, 2022 at 8:48 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Wed, Apr 27, 2022 at 10:45 PM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > On Wed, Apr 27, 2022 at 7:16 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Tue, Apr 26, 2022 at 8:59 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> > > >
> > > > On Mon, Apr 25, 2022 at 9:57 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> > > > >
> > > > > On 4/23/22 4:00 PM, Yafang Shao wrote:
> > > > > > Currently there're helpers for allowing to open/load/attach BPF object
> > > > > > through BPF object skeleton. Let's also add helpers for pinning through
> > > > > > BPF object skeleton. It could simplify BPF userspace code which wants to
> > > > > > pin the progs into bpffs.
> > > > >
> > > > > Please elaborate some more on your use case/rationale for the commit message,
> > > > > do you have orchestration code that will rely on these specifically?
> > > > >
> > > >
> > > > We have a bpf manager on our production environment to maintain the
> > > > bpf programs, some of which need to be pinned in bpffs, for example
> > > > tracing bpf programs, perf_event programs and other bpf hooks added by
> > > > ourselves for performance tuning.  These bpf programs don't need a
> > > > user agent, while they really work like a kernel module, that is why
> > > > we pin them. For these kinds of bpf programs, the bpf manager can help
> > > > to simplify the development and deployment.  Take the improvement on
> > > > development for example,  the user doesn't need to write userspace
> > > > code while he focuses on the kernel side only, and then bpf manager
> > > > will do all the other things. Below is a simple example,
> > > >    Step1, gen the skeleton for the user provided bpf object file,
> > > >               $ bpftool gen skeleton  test.bpf.o > simple.skel.h
> > > >    Step2, Compile the bpf object file into a runnable binary
> > > >               #include "simple.skel.h"
> > > >
> > > >               #define SIMPLE_BPF_PIN(name, path)  \
> > > >               ({                                                              \
> > > >                   struct name##_bpf *obj;                      \
> > > >                   int err = 0;                                            \
> > > >                                                                               \
> > > >                   obj = name##_bpf__open();                \
> > > >                    if (!obj) {                                              \
> > > >                        err = -errno;                                    \
> > > >                        goto cleanup;                                 \
> > > >                     }                                                         \
> > > >                                                                               \
> > > >                     err = name##_bpf__load(obj);           \
> > > >                     if (err)                                                 \
> > > >                         goto cleanup;                                 \
> > > >                                                                                \
> > > >                      err = name##_bpf__attach(obj);       \
> > > >                      if (err)                                                \
> > > >                          goto cleanup;                                \
> > > >                                                                                \
> > > >                      err = name##_bpf__pin_prog(obj, path);      \
> > > >                      if (err)                                                \
> > > >                          goto cleanup;                                \
> > > >                                                                               \
> > > >                       goto end;                                         \
> > > >                                                                               \
> > > >                   cleanup:                                              \
> > > >                       name##_bpf__destroy(obj);            \
> > > >                   end:                                                     \
> > > >                       err;                                                  \
> > > >                    })
> > > >
> > > >                    SIMPLE_BPF_PIN(test, "/sys/fs/bpf");
> > > >
> > > >                As the userspace code of FD-based bpf objects are all
> > > > the same,  so we can abstract them as above.  The pathset means to add
> > > > the non-exist "name##_bpf__pin_prog(obj, path)" for it.
> > > >
> > >
> > > Your BPF manager is user-space code that you control, right? I'm not
> > > sure how skeleton is helpful here given your BPF manager is generic
> > > and doesn't work with any specific skeleton, if I understand the idea.
> > > But let's assume that you use skeleton to also embed BPF ELF bytes and
> > > pass them to your manager for "activation". Once you open and load
> > > bpf_object, your BPF manager can generically iterate all BPF programs
> > > using bpf_object_for_each_program(), attempt to attach them with
> > > bpf_program__attach() (see how bpf_object__attach_skeleton is handling
> > > non-auto-attachable programs) and immediately pin the link (no need to
> > > even store it, you can destroy it after pinning immediately). All this
> > > is using generic libbpf APIs and requires no code generation.
> >
> > Many thanks for the detailed explanation. Your suggestion can also
> > work, but with the skeleton we can also generate a binary which can
> > run independently.  (Technically speaking, the binary is the same as
> > './bpf_install target.bpf.o').
> >
>
> Forgot to mention that with skeleton we can also modify the global
> data defined in bpf object file, that may need to be abstracted as a
> new common helper.  The bpf_object__* functions can't do it, right ?

I must be missing something because I don't see how you can have
code-generated skeleton and generic BPF manager at the same time. I'm
not saying don't use skeleton, I'm saying you can write this link
pinning code yourself and reuse it in your applications. You can get
access to struct bpf_object through skel->obj.

>
> > >  But keep
> > > in mind that not all struct bpf_link in libbpf are pinnable (not all
> > > links have FD-based BPF link in kernel associated with them), so
> > > you'll have to deal with that somehow (and what you didn't do in this
> > > patch for libbpf implementation).
> > >
> >
> > Right, I have found it. If I understand it correctly, only the link
> > types defined in enum bpf_link_type (which is in
> > include/uapi/linux/bpf.h) are pinnable, right?
> >

It's more complicated. For kprobe/tracepoint, for example, depending
on host kernel version it could be a "fake" libbpf-side-only link, or
it could be a proper kernel object backing it. So as always, it
depends.


> > BTW, is it possible to support pinning all struct bpf_link in libbpf ?

No, it depends on kernel support, libbpf can't do much about this.

> >
> >
> > --
> > Regards
> > Yafang
>
>
>
> --
> Regards
> Yafang

[PATCH bpf-next 3/4] bpftool: Fix incorrect return in generated detach helper

[Yafang Shao]

There is no return value of bpf_object__detach_skeleton(), so we'd
better not return it, that is formal.

Fixes: 5dc7a8b21144 ("bpftool, selftests/bpf: Embed object file inside skeleton")
Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/bpf/bpftool/gen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 7678af364793..8f76d8d9996c 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1171,7 +1171,7 @@ static int do_skeleton(int argc, char **argv)
 		static inline void					    \n\
 		%1$s__detach(struct %1$s *obj)				    \n\
 		{							    \n\
-			return bpf_object__detach_skeleton(obj->skeleton);  \n\
+			bpf_object__detach_skeleton(obj->skeleton);	    \n\
 		}							    \n\
 		",
 		obj_name
-- 
2.17.1

Re: [PATCH bpf-next 3/4] bpftool: Fix incorrect return in generated detach helper

[Andrii Nakryiko]

On Sat, Apr 23, 2022 at 7:02 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> There is no return value of bpf_object__detach_skeleton(), so we'd
> better not return it, that is formal.
>
> Fixes: 5dc7a8b21144 ("bpftool, selftests/bpf: Embed object file inside skeleton")
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>  tools/bpf/bpftool/gen.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 7678af364793..8f76d8d9996c 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1171,7 +1171,7 @@ static int do_skeleton(int argc, char **argv)
>                 static inline void                                          \n\
>                 %1$s__detach(struct %1$s *obj)                              \n\
>                 {                                                           \n\
> -                       return bpf_object__detach_skeleton(obj->skeleton);  \n\
> +                       bpf_object__detach_skeleton(obj->skeleton);         \n\

It's not incorrect to return the result of void-returning function in
another void-returning function. C compiler allows this and we rely on
this property very explicitly in macros like BPF_PROG and BPF_KPROBE.
So if anything, it's not a fix, at best improvement, but even then
quite optional.


>                 }                                                           \n\
>                 ",
>                 obj_name
> --
> 2.17.1
>

Re: [PATCH bpf-next 3/4] bpftool: Fix incorrect return in generated detach helper

[Yafang Shao]

On Tue, Apr 26, 2022 at 2:47 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 23, 2022 at 7:02 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > There is no return value of bpf_object__detach_skeleton(), so we'd
> > better not return it, that is formal.
> >
> > Fixes: 5dc7a8b21144 ("bpftool, selftests/bpf: Embed object file inside skeleton")
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >  tools/bpf/bpftool/gen.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 7678af364793..8f76d8d9996c 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -1171,7 +1171,7 @@ static int do_skeleton(int argc, char **argv)
> >                 static inline void                                          \n\
> >                 %1$s__detach(struct %1$s *obj)                              \n\
> >                 {                                                           \n\
> > -                       return bpf_object__detach_skeleton(obj->skeleton);  \n\
> > +                       bpf_object__detach_skeleton(obj->skeleton);         \n\
>
> It's not incorrect to return the result of void-returning function in
> another void-returning function. C compiler allows this and we rely on
> this property very explicitly in macros like BPF_PROG and BPF_KPROBE.
> So if anything, it's not a fix, at best improvement, but even then
> quite optional.

Right, the C compiler allows it.
I won't change it.

-- 
Regards
Yafang

[PATCH bpf-next 4/4] bpftool: Generate helpers for pinning prog through bpf object skeleton

[Yafang Shao]

After this change, with command 'bpftool gen skeleton XXX.bpf.o', the
helpers for pinning BPF prog will be generated in BPF object skeleton. It
could simplify userspace code which wants to pin bpf progs in bpffs.

The new helpers are named with __{pin, unpin}_prog, because it only pins
bpf progs. If the user also wants to pin bpf maps in bpffs, he can use
LIBBPF_PIN_BY_NAME.

Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
---
 tools/bpf/bpftool/gen.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index 8f76d8d9996c..1d06ebde723b 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -1087,6 +1087,8 @@ static int do_skeleton(int argc, char **argv)
 			static inline int load(struct %1$s *skel);	    \n\
 			static inline int attach(struct %1$s *skel);	    \n\
 			static inline void detach(struct %1$s *skel);	    \n\
+			static inline int pin_prog(struct %1$s *skel, const char *path);\n\
+			static inline void unpin_prog(struct %1$s *skel);   \n\
 			static inline void destroy(struct %1$s *skel);	    \n\
 			static inline const void *elf_bytes(size_t *sz);    \n\
 		#endif /* __cplusplus */				    \n\
@@ -1172,6 +1174,18 @@ static int do_skeleton(int argc, char **argv)
 		%1$s__detach(struct %1$s *obj)				    \n\
 		{							    \n\
 			bpf_object__detach_skeleton(obj->skeleton);	    \n\
+		}							    \n\
+									    \n\
+		static inline int					    \n\
+		%1$s__pin_prog(struct %1$s *obj, const char *path)	    \n\
+		{							    \n\
+			return bpf_object__pin_skeleton_prog(obj->skeleton, path);\n\
+		}							    \n\
+									    \n\
+		static inline void					    \n\
+		%1$s__unpin_prog(struct %1$s *obj)			    \n\
+		{							    \n\
+			bpf_object__unpin_skeleton_prog(obj->skeleton);	    \n\
 		}							    \n\
 		",
 		obj_name
@@ -1237,6 +1251,8 @@ static int do_skeleton(int argc, char **argv)
 		int %1$s::load(struct %1$s *skel) { return %1$s__load(skel); }		\n\
 		int %1$s::attach(struct %1$s *skel) { return %1$s__attach(skel); }	\n\
 		void %1$s::detach(struct %1$s *skel) { %1$s__detach(skel); }		\n\
+		int %1$s::pin_prog(struct %1$s *skel, const char *path) { return %1$s__pin_prog(skel, path); }\n\
+		void %1$s::unpin_prog(struct %1$s *skel) { %1$s__unpin_prog(skel); }	\n\
 		void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }		\n\
 		const void *%1$s::elf_bytes(size_t *sz) { return %1$s__elf_bytes(sz); } \n\
 		#endif /* __cplusplus */				    \n\
-- 
2.17.1

Re: [PATCH bpf-next 4/4] bpftool: Generate helpers for pinning prog through bpf object skeleton

[Daniel Borkmann]

On 4/23/22 4:00 PM, Yafang Shao wrote:
> After this change, with command 'bpftool gen skeleton XXX.bpf.o', the
> helpers for pinning BPF prog will be generated in BPF object skeleton. It
> could simplify userspace code which wants to pin bpf progs in bpffs.
> 
> The new helpers are named with __{pin, unpin}_prog, because it only pins
> bpf progs. If the user also wants to pin bpf maps in bpffs, he can use
> LIBBPF_PIN_BY_NAME.
> 
> Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> ---
>   tools/bpf/bpftool/gen.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> index 8f76d8d9996c..1d06ebde723b 100644
> --- a/tools/bpf/bpftool/gen.c
> +++ b/tools/bpf/bpftool/gen.c
> @@ -1087,6 +1087,8 @@ static int do_skeleton(int argc, char **argv)
>   			static inline int load(struct %1$s *skel);	    \n\
>   			static inline int attach(struct %1$s *skel);	    \n\
>   			static inline void detach(struct %1$s *skel);	    \n\
> +			static inline int pin_prog(struct %1$s *skel, const char *path);\n\
> +			static inline void unpin_prog(struct %1$s *skel);   \n\
>   			static inline void destroy(struct %1$s *skel);	    \n\
>   			static inline const void *elf_bytes(size_t *sz);    \n\
>   		#endif /* __cplusplus */				    \n\
> @@ -1172,6 +1174,18 @@ static int do_skeleton(int argc, char **argv)
>   		%1$s__detach(struct %1$s *obj)				    \n\
>   		{							    \n\
>   			bpf_object__detach_skeleton(obj->skeleton);	    \n\
> +		}							    \n\
> +									    \n\
> +		static inline int					    \n\
> +		%1$s__pin_prog(struct %1$s *obj, const char *path)	    \n\
> +		{							    \n\
> +			return bpf_object__pin_skeleton_prog(obj->skeleton, path);\n\
> +		}							    \n\
> +									    \n\
> +		static inline void					    \n\
> +		%1$s__unpin_prog(struct %1$s *obj)			    \n\
> +		{							    \n\
> +			bpf_object__unpin_skeleton_prog(obj->skeleton);	    \n\
>   		}							    \n\

(This should also have BPF selftest code as in-tree user.)

>   		",
>   		obj_name
> @@ -1237,6 +1251,8 @@ static int do_skeleton(int argc, char **argv)
>   		int %1$s::load(struct %1$s *skel) { return %1$s__load(skel); }		\n\
>   		int %1$s::attach(struct %1$s *skel) { return %1$s__attach(skel); }	\n\
>   		void %1$s::detach(struct %1$s *skel) { %1$s__detach(skel); }		\n\
> +		int %1$s::pin_prog(struct %1$s *skel, const char *path) { return %1$s__pin_prog(skel, path); }\n\
> +		void %1$s::unpin_prog(struct %1$s *skel) { %1$s__unpin_prog(skel); }	\n\
>   		void %1$s::destroy(struct %1$s *skel) { %1$s__destroy(skel); }		\n\
>   		const void *%1$s::elf_bytes(size_t *sz) { return %1$s__elf_bytes(sz); } \n\
>   		#endif /* __cplusplus */				    \n\
> 

Re: [PATCH bpf-next 4/4] bpftool: Generate helpers for pinning prog through bpf object skeleton

[Yafang Shao]

On Mon, Apr 25, 2022 at 10:03 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 4/23/22 4:00 PM, Yafang Shao wrote:
> > After this change, with command 'bpftool gen skeleton XXX.bpf.o', the
> > helpers for pinning BPF prog will be generated in BPF object skeleton. It
> > could simplify userspace code which wants to pin bpf progs in bpffs.
> >
> > The new helpers are named with __{pin, unpin}_prog, because it only pins
> > bpf progs. If the user also wants to pin bpf maps in bpffs, he can use
> > LIBBPF_PIN_BY_NAME.
> >
> > Signed-off-by: Yafang Shao <laoar.shao@gmail.com>
> > ---
> >   tools/bpf/bpftool/gen.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
> > index 8f76d8d9996c..1d06ebde723b 100644
> > --- a/tools/bpf/bpftool/gen.c
> > +++ b/tools/bpf/bpftool/gen.c
> > @@ -1087,6 +1087,8 @@ static int do_skeleton(int argc, char **argv)
> >                       static inline int load(struct %1$s *skel);          \n\
> >                       static inline int attach(struct %1$s *skel);        \n\
> >                       static inline void detach(struct %1$s *skel);       \n\
> > +                     static inline int pin_prog(struct %1$s *skel, const char *path);\n\
> > +                     static inline void unpin_prog(struct %1$s *skel);   \n\
> >                       static inline void destroy(struct %1$s *skel);      \n\
> >                       static inline const void *elf_bytes(size_t *sz);    \n\
> >               #endif /* __cplusplus */                                    \n\
> > @@ -1172,6 +1174,18 @@ static int do_skeleton(int argc, char **argv)
> >               %1$s__detach(struct %1$s *obj)                              \n\
> >               {                                                           \n\
> >                       bpf_object__detach_skeleton(obj->skeleton);         \n\
> > +             }                                                           \n\
> > +                                                                         \n\
> > +             static inline int                                           \n\
> > +             %1$s__pin_prog(struct %1$s *obj, const char *path)          \n\
> > +             {                                                           \n\
> > +                     return bpf_object__pin_skeleton_prog(obj->skeleton, path);\n\
> > +             }                                                           \n\
> > +                                                                         \n\
> > +             static inline void                                          \n\
> > +             %1$s__unpin_prog(struct %1$s *obj)                          \n\
> > +             {                                                           \n\
> > +                     bpf_object__unpin_skeleton_prog(obj->skeleton);     \n\
> >               }                                                           \n\
>
> (This should also have BPF selftest code as in-tree user.)
>

Will do it, thanks.

[snip]

-- 
Regards
Yafang

Re: [PATCH bpf-next 0/4] bpf: Generate helpers for pinning through bpf object skeleton

[Andrii Nakryiko]

On Sat, Apr 23, 2022 at 7:01 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> Currently there're helpers for allowing to open/load/attach BPF object
> through BPF object skeleton. Let's also add helpers for pinning through
> BPF object skeleton. It could simplify BPF userspace code which wants to
> pin the progs into bpffs.
>
> After this change, with command 'bpftool gen skeleton XXX.bpf.o', the
> helpers for pinning BPF prog will be generated in BPF object skeleton.
>
> The new helpers are named with __{pin, unpin}_prog, because it only pins
> bpf progs. If the user also wants to pin bpf maps, he can use
> LIBBPF_PIN_BY_NAME.

API says it's pinning programs, but really it's trying to pin links.
But those links might not even be created for non-auto-attachable
programs, and for others users might or might not set
<skel>.links.<prog_name> links.

There are lots of questions about this new functionality... But the
main one is why do we need it? What does it bring that's hard to do
otherwise?

>
> Yafang Shao (4):
>   libbpf: Define DEFAULT_BPFFS
>   libbpf: Add helpers for pinning bpf prog through bpf object skeleton
>   bpftool: Fix incorrect return in generated detach helper
>   bpftool: Generate helpers for pinning prog through bpf object skeleton
>
>  tools/bpf/bpftool/gen.c     | 18 ++++++++++-
>  tools/lib/bpf/bpf_helpers.h |  2 +-
>  tools/lib/bpf/libbpf.c      | 61 ++++++++++++++++++++++++++++++++++++-
>  tools/lib/bpf/libbpf.h      | 10 ++++--
>  tools/lib/bpf/libbpf.map    |  2 ++
>  5 files changed, 88 insertions(+), 5 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH bpf-next 0/4] bpf: Generate helpers for pinning through bpf object skeleton

[Yafang Shao]

On Tue, Apr 26, 2022 at 2:45 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Apr 23, 2022 at 7:01 AM Yafang Shao <laoar.shao@gmail.com> wrote:
> >
> > Currently there're helpers for allowing to open/load/attach BPF object
> > through BPF object skeleton. Let's also add helpers for pinning through
> > BPF object skeleton. It could simplify BPF userspace code which wants to
> > pin the progs into bpffs.
> >
> > After this change, with command 'bpftool gen skeleton XXX.bpf.o', the
> > helpers for pinning BPF prog will be generated in BPF object skeleton.
> >
> > The new helpers are named with __{pin, unpin}_prog, because it only pins
> > bpf progs. If the user also wants to pin bpf maps, he can use
> > LIBBPF_PIN_BY_NAME.
>
> API says it's pinning programs, but really it's trying to pin links.

Actually it should be bpf_object__pin_skeleton_link().

> But those links might not even be created for non-auto-attachable
> programs, and for others users might or might not set
> <skel>.links.<prog_name> links.
>
> There are lots of questions about this new functionality... But the
> main one is why do we need it? What does it bring that's hard to do
> otherwise?
>

See also my replyment to Daniel[1].
For the FD-based bpf objects, the userspace code is similar, so we can
abstract the userspace code into a common code, and then the developer
doesn't need to write the userspace code any more (if he doesn't have
some special userspace logical.).


[1]. https://lore.kernel.org/bpf/CAEf4BzbhODOBrE=unLOUpo40uUYz72BJX-+uJobiwhF9VFSizQ@mail.gmail.com/T/#m32dfc6343f2b4fba980c62686b245cb6e0133c2f


> >
> > Yafang Shao (4):
> >   libbpf: Define DEFAULT_BPFFS
> >   libbpf: Add helpers for pinning bpf prog through bpf object skeleton
> >   bpftool: Fix incorrect return in generated detach helper
> >   bpftool: Generate helpers for pinning prog through bpf object skeleton
> >
> >  tools/bpf/bpftool/gen.c     | 18 ++++++++++-
> >  tools/lib/bpf/bpf_helpers.h |  2 +-
> >  tools/lib/bpf/libbpf.c      | 61 ++++++++++++++++++++++++++++++++++++-
> >  tools/lib/bpf/libbpf.h      | 10 ++++--
> >  tools/lib/bpf/libbpf.map    |  2 ++
> >  5 files changed, 88 insertions(+), 5 deletions(-)
> >
> > --
> > 2.17.1
> >



-- 
Regards
Yafang