2021-12-05 04:51:12

by Menglong Dong

[permalink] [raw]
Subject: [PATCH] bpftool: add support of pin prog by name

From: Menglong Dong <[email protected]>

For now, the command 'bpftool prog loadall' use section name as the
name of the pin file. However, once there are prog with the same
section name in ELF file, this command will failed with the error
'File Exist'.

So, add the support of pin prog by function name with the 'pinbyname'
argument.

Signed-off-by: Menglong Dong <[email protected]>
---
tools/bpf/bpftool/prog.c | 7 +++++++
tools/lib/bpf/libbpf.c | 5 +++++
tools/lib/bpf/libbpf.h | 2 ++
3 files changed, 14 insertions(+)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index e47e8b06cc3d..74e0aaebfefc 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1471,6 +1471,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
unsigned int old_map_fds = 0;
const char *pinmaps = NULL;
struct bpf_object *obj;
+ bool pinbyname = false;
struct bpf_map *map;
const char *pinfile;
unsigned int i, j;
@@ -1589,6 +1590,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
goto err_free_reuse_maps;

pinmaps = GET_ARG();
+ } else if (is_prefix(*argv, "pinbyname")) {
+ pinbyname = true;
+ NEXT_ARG();
} else {
p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
*argv);
@@ -1616,6 +1620,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
goto err_close_obj;
}

+ if (pinbyname)
+ bpf_program__set_pinname(pos,
+ (char *)bpf_program__name(pos));
bpf_program__set_ifindex(pos, ifindex);
bpf_program__set_type(pos, prog_type);
bpf_program__set_expected_attach_type(pos, expected_attach_type);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index f6faa33c80fa..e8fc1d0fe16e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8119,6 +8119,11 @@ void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
prog->prog_ifindex = ifindex;
}

+void bpf_program__set_pinname(struct bpf_program *prog, char *name)
+{
+ prog->pin_name = name;
+}
+
const char *bpf_program__name(const struct bpf_program *prog)
{
return prog->name;
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 4ec69f224342..107cf736c2bb 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -216,6 +216,8 @@ LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
__u32 ifindex);
+LIBBPF_API void bpf_program__set_pinname(struct bpf_program *prog,
+ char *name);

LIBBPF_API const char *bpf_program__name(const struct bpf_program *prog);
LIBBPF_API const char *bpf_program__section_name(const struct bpf_program *prog);
--
2.30.2



2021-12-06 21:22:14

by Andrii Nakryiko

[permalink] [raw]
Subject: Re: [PATCH] bpftool: add support of pin prog by name

On Sat, Dec 4, 2021 at 8:51 PM <[email protected]> wrote:
>
> From: Menglong Dong <[email protected]>
>
> For now, the command 'bpftool prog loadall' use section name as the
> name of the pin file. However, once there are prog with the same
> section name in ELF file, this command will failed with the error
> 'File Exist'.
>
> So, add the support of pin prog by function name with the 'pinbyname'
> argument.
>
> Signed-off-by: Menglong Dong <[email protected]>
> ---

Doesn't [0] do that already?

[0] https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

> tools/bpf/bpftool/prog.c | 7 +++++++
> tools/lib/bpf/libbpf.c | 5 +++++
> tools/lib/bpf/libbpf.h | 2 ++
> 3 files changed, 14 insertions(+)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index e47e8b06cc3d..74e0aaebfefc 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1471,6 +1471,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> unsigned int old_map_fds = 0;
> const char *pinmaps = NULL;
> struct bpf_object *obj;
> + bool pinbyname = false;
> struct bpf_map *map;
> const char *pinfile;
> unsigned int i, j;
> @@ -1589,6 +1590,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> goto err_free_reuse_maps;
>
> pinmaps = GET_ARG();
> + } else if (is_prefix(*argv, "pinbyname")) {
> + pinbyname = true;
> + NEXT_ARG();
> } else {
> p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
> *argv);
> @@ -1616,6 +1620,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> goto err_close_obj;
> }
>
> + if (pinbyname)
> + bpf_program__set_pinname(pos,
> + (char *)bpf_program__name(pos));
> bpf_program__set_ifindex(pos, ifindex);
> bpf_program__set_type(pos, prog_type);
> bpf_program__set_expected_attach_type(pos, expected_attach_type);
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f6faa33c80fa..e8fc1d0fe16e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8119,6 +8119,11 @@ void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
> prog->prog_ifindex = ifindex;
> }
>
> +void bpf_program__set_pinname(struct bpf_program *prog, char *name)
> +{
> + prog->pin_name = name;

BPF maps have bpf_map__set_pin_path(), setting a full path is more
flexible approach, I think, so if we had to do something here, it's
better to add bpf_program__set_ping_path().


> +}
> +
> const char *bpf_program__name(const struct bpf_program *prog)
> {
> return prog->name;
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 4ec69f224342..107cf736c2bb 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -216,6 +216,8 @@ LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
> LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
> LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
> __u32 ifindex);
> +LIBBPF_API void bpf_program__set_pinname(struct bpf_program *prog,
> + char *name);
>
> LIBBPF_API const char *bpf_program__name(const struct bpf_program *prog);
> LIBBPF_API const char *bpf_program__section_name(const struct bpf_program *prog);
> --
> 2.30.2
>

2021-12-07 01:58:11

by Menglong Dong

[permalink] [raw]
Subject: Re: [PATCH] bpftool: add support of pin prog by name

On Tue, Dec 7, 2021 at 5:22 AM Andrii Nakryiko
<[email protected]> wrote:
>
> On Sat, Dec 4, 2021 at 8:51 PM <[email protected]> wrote:
> >
> > From: Menglong Dong <[email protected]>
> >
> > For now, the command 'bpftool prog loadall' use section name as the
> > name of the pin file. However, once there are prog with the same
> > section name in ELF file, this command will failed with the error
> > 'File Exist'.
> >
> > So, add the support of pin prog by function name with the 'pinbyname'
> > argument.
> >
> > Signed-off-by: Menglong Dong <[email protected]>
> > ---
>
> Doesn't [0] do that already?
>
> [0] https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/
>

Ops....Sorry, I didn't notice that patch :/

> > tools/bpf/bpftool/prog.c | 7 +++++++
> > tools/lib/bpf/libbpf.c | 5 +++++
> > tools/lib/bpf/libbpf.h | 2 ++
> > 3 files changed, 14 insertions(+)
> >
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index e47e8b06cc3d..74e0aaebfefc 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -1471,6 +1471,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > unsigned int old_map_fds = 0;
> > const char *pinmaps = NULL;
> > struct bpf_object *obj;
> > + bool pinbyname = false;
> > struct bpf_map *map;
> > const char *pinfile;
> > unsigned int i, j;
> > @@ -1589,6 +1590,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > goto err_free_reuse_maps;
> >
> > pinmaps = GET_ARG();
> > + } else if (is_prefix(*argv, "pinbyname")) {
> > + pinbyname = true;
> > + NEXT_ARG();
> > } else {
> > p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
> > *argv);
> > @@ -1616,6 +1620,9 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> > goto err_close_obj;
> > }
> >
> > + if (pinbyname)
> > + bpf_program__set_pinname(pos,
> > + (char *)bpf_program__name(pos));
> > bpf_program__set_ifindex(pos, ifindex);
> > bpf_program__set_type(pos, prog_type);
> > bpf_program__set_expected_attach_type(pos, expected_attach_type);
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index f6faa33c80fa..e8fc1d0fe16e 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -8119,6 +8119,11 @@ void bpf_program__set_ifindex(struct bpf_program *prog, __u32 ifindex)
> > prog->prog_ifindex = ifindex;
> > }
> >
> > +void bpf_program__set_pinname(struct bpf_program *prog, char *name)
> > +{
> > + prog->pin_name = name;
>
> BPF maps have bpf_map__set_pin_path(), setting a full path is more
> flexible approach, I think, so if we had to do something here, it's
> better to add bpf_program__set_ping_path().

Yeah, I think it's a good idea. I'll do something about it.

Thanks!
Menglong Dong

>
>
> > +}
> > +
> > const char *bpf_program__name(const struct bpf_program *prog)
> > {
> > return prog->name;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 4ec69f224342..107cf736c2bb 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -216,6 +216,8 @@ LIBBPF_API int bpf_program__set_priv(struct bpf_program *prog, void *priv,
> > LIBBPF_API void *bpf_program__priv(const struct bpf_program *prog);
> > LIBBPF_API void bpf_program__set_ifindex(struct bpf_program *prog,
> > __u32 ifindex);
> > +LIBBPF_API void bpf_program__set_pinname(struct bpf_program *prog,
> > + char *name);
> >
> > LIBBPF_API const char *bpf_program__name(const struct bpf_program *prog);
> > LIBBPF_API const char *bpf_program__section_name(const struct bpf_program *prog);
> > --
> > 2.30.2
> >