[PATCH bpf-next] bpftool: Register struct_ops with a link.

[PATCH bpf-next] bpftool: Register struct_ops with a link.

[Kui-Feng Lee]

You can include an optional path after specifying the object name for the
'struct_ops register' subcommand.

Since the commit 226bc6ae6405 ("Merge branch 'Transit between BPF TCP
congestion controls.'") has been accepted, it is now possible to create a
link for a struct_ops. This can be done by defining a struct_ops in
SEC(".struct_ops.link") to make libbpf returns a real link. If we don't pin
the links before leaving bpftool, they will disappear. To instruct bpftool
to pin the links in a directory with the names of the maps, we need to
provide the path of that directory.

Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
 tools/bpf/bpftool/struct_ops.c | 86 ++++++++++++++++++++++++++++------
 1 file changed, 72 insertions(+), 14 deletions(-)

diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
index b389f4830e11..d1ae39f9d8df 100644
--- a/tools/bpf/bpftool/struct_ops.c
+++ b/tools/bpf/bpftool/struct_ops.c
@@ -475,21 +475,62 @@ static int do_unregister(int argc, char **argv)
 	return cmd_retval(&res, true);
 }
 
+static int pathname_concat(char *buf, int buf_sz, const char *path,
+			   const char *name)
+{
+	int len;
+
+	len = snprintf(buf, buf_sz, "%s/%s", path, name);
+	if (len < 0)
+		return -EINVAL;
+	if (len >= buf_sz)
+		return -ENAMETOOLONG;
+
+	return 0;
+}
+
+static int pin_link(struct bpf_link *link, const char *pindir,
+		    const char *name)
+{
+	char pinfile[PATH_MAX];
+	int err;
+
+	err = pathname_concat(pinfile, sizeof(pinfile), pindir, name);
+	if (err)
+		return -1;
+
+	err = bpf_link__pin(link, pinfile);
+	if (err)
+		return -1;
+
+	return 0;
+}
+
 static int do_register(int argc, char **argv)
 {
 	LIBBPF_OPTS(bpf_object_open_opts, open_opts);
+	__u32 link_info_len = sizeof(struct bpf_link_info);
+	struct bpf_link_info link_info = {};
 	struct bpf_map_info info = {};
 	__u32 info_len = sizeof(info);
 	int nr_errs = 0, nr_maps = 0;
+	const char *pindir = NULL;
 	struct bpf_object *obj;
 	struct bpf_link *link;
 	struct bpf_map *map;
 	const char *file;
 
-	if (argc != 1)
+	if (argc != 1 && argc != 2)
 		usage();
 
 	file = GET_ARG();
+	if (argc == 1)
+		pindir = GET_ARG();
+
+	if (pindir && mount_bpffs_for_pin(pindir)) {
+		p_err("can't mount bpffs for pinning");
+		return -1;
+	}
 
 	if (verifier_logs)
 		/* log_level1 + log_level2 + stats, but not stable UAPI */
@@ -519,21 +560,38 @@ static int do_register(int argc, char **argv)
 		}
 		nr_maps++;
 
-		bpf_link__disconnect(link);
-		bpf_link__destroy(link);
-
-		if (!bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
-					    &info_len))
-			p_info("Registered %s %s id %u",
-			       get_kern_struct_ops_name(&info),
-			       bpf_map__name(map),
-			       info.id);
-		else
+		if (bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
+					   &info_len)) {
 			/* Not p_err.  The struct_ops was attached
 			 * successfully.
 			 */
-			p_info("Registered %s but can't find id: %s",
-			       bpf_map__name(map), strerror(errno));
+			p_err("Registered %s but can't find id: %s",
+			      bpf_map__name(map), strerror(errno));
+			nr_errs++;
+		} else if (!(bpf_map__map_flags(map) & BPF_F_LINK)) {
+			p_info("Registered %s %s id %u",
+			       get_kern_struct_ops_name(&info),
+			       info.name,
+			       info.id);
+		} else if (bpf_link_get_info_by_fd(bpf_link__fd(link),
+						   &link_info,
+					    &link_info_len)) {
+			p_err("Registered %s but can't find link id: %s",
+			      bpf_map__name(map), strerror(errno));
+			nr_errs++;
+		} else if (pindir && pin_link(link, pindir, info.name)) {
+			p_err("can't pin link %u for %s: %s",
+			      link_info.id, info.name,
+			      strerror(errno));
+			nr_errs++;
+		} else
+			p_info("Registered %s %s map id %u link id %u",
+			       get_kern_struct_ops_name(&info),
+			       info.name, info.id, link_info.id);
+
+		bpf_link__disconnect(link);
+		bpf_link__destroy(link);
+
 	}
 
 	bpf_object__close(obj);
@@ -562,7 +620,7 @@ static int do_help(int argc, char **argv)
 	fprintf(stderr,
 		"Usage: %1$s %2$s { show | list } [STRUCT_OPS_MAP]\n"
 		"       %1$s %2$s dump [STRUCT_OPS_MAP]\n"
-		"       %1$s %2$s register OBJ\n"
+		"       %1$s %2$s register OBJ [PATH]\n"
 		"       %1$s %2$s unregister STRUCT_OPS_MAP\n"
 		"       %1$s %2$s help\n"
 		"\n"
-- 
2.34.1

Re: [PATCH bpf-next] bpftool: Register struct_ops with a link.

[Quentin Monnet]

On Tue, 18 Apr 2023 at 21:01, Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> You can include an optional path after specifying the object name for the
> 'struct_ops register' subcommand.
>
> Since the commit 226bc6ae6405 ("Merge branch 'Transit between BPF TCP
> congestion controls.'") has been accepted, it is now possible to create a
> link for a struct_ops. This can be done by defining a struct_ops in
> SEC(".struct_ops.link") to make libbpf returns a real link. If we don't pin
> the links before leaving bpftool, they will disappear. To instruct bpftool
> to pin the links in a directory with the names of the maps, we need to
> provide the path of that directory.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
>  tools/bpf/bpftool/struct_ops.c | 86 ++++++++++++++++++++++++++++------
>  1 file changed, 72 insertions(+), 14 deletions(-)
>
> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
> index b389f4830e11..d1ae39f9d8df 100644
> --- a/tools/bpf/bpftool/struct_ops.c
> +++ b/tools/bpf/bpftool/struct_ops.c
> @@ -475,21 +475,62 @@ static int do_unregister(int argc, char **argv)
>         return cmd_retval(&res, true);
>  }
>
> +static int pathname_concat(char *buf, int buf_sz, const char *path,
> +                          const char *name)
> +{
> +       int len;
> +
> +       len = snprintf(buf, buf_sz, "%s/%s", path, name);
> +       if (len < 0)
> +               return -EINVAL;
> +       if (len >= buf_sz)
> +               return -ENAMETOOLONG;
> +
> +       return 0;
> +}

This is nearly identical to the one in prog.c. If we do need this, we
should move it to common.c and reuse it.

> +
> +static int pin_link(struct bpf_link *link, const char *pindir,
> +                   const char *name)
> +{
> +       char pinfile[PATH_MAX];
> +       int err;
> +
> +       err = pathname_concat(pinfile, sizeof(pinfile), pindir, name);
> +       if (err)
> +               return -1;
> +
> +       err = bpf_link__pin(link, pinfile);
> +       if (err)
> +               return -1;
> +
> +       return 0;
> +}
> +
>  static int do_register(int argc, char **argv)
>  {
>         LIBBPF_OPTS(bpf_object_open_opts, open_opts);
> +       __u32 link_info_len = sizeof(struct bpf_link_info);
> +       struct bpf_link_info link_info = {};
>         struct bpf_map_info info = {};
>         __u32 info_len = sizeof(info);
>         int nr_errs = 0, nr_maps = 0;
> +       const char *pindir = NULL;
>         struct bpf_object *obj;
>         struct bpf_link *link;
>         struct bpf_map *map;
>         const char *file;
>
> -       if (argc != 1)
> +       if (argc != 1 && argc != 2)
>                 usage();
>
>         file = GET_ARG();
> +       if (argc == 1)
> +               pindir = GET_ARG();
> +
> +       if (pindir && mount_bpffs_for_pin(pindir)) {
> +               p_err("can't mount bpffs for pinning");
> +               return -1;
> +       }
>
>         if (verifier_logs)
>                 /* log_level1 + log_level2 + stats, but not stable UAPI */
> @@ -519,21 +560,38 @@ static int do_register(int argc, char **argv)
>                 }
>                 nr_maps++;
>
> -               bpf_link__disconnect(link);
> -               bpf_link__destroy(link);
> -
> -               if (!bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
> -                                           &info_len))
> -                       p_info("Registered %s %s id %u",
> -                              get_kern_struct_ops_name(&info),
> -                              bpf_map__name(map),
> -                              info.id);
> -               else
> +               if (bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
> +                                          &info_len)) {
>                         /* Not p_err.  The struct_ops was attached
>                          * successfully.
>                          */
> -                       p_info("Registered %s but can't find id: %s",
> -                              bpf_map__name(map), strerror(errno));
> +                       p_err("Registered %s but can't find id: %s",
> +                             bpf_map__name(map), strerror(errno));

See comment right above: p_info() is probably enough here. If for some
reason we do need to switch to an error message and change the
existing behaviour, can you please motivate it and make it a separate
commit (and update the comment)?

> +                       nr_errs++;
> +               } else if (!(bpf_map__map_flags(map) & BPF_F_LINK)) {
> +                       p_info("Registered %s %s id %u",
> +                              get_kern_struct_ops_name(&info),
> +                              info.name,
> +                              info.id);
> +               } else if (bpf_link_get_info_by_fd(bpf_link__fd(link),
> +                                                  &link_info,
> +                                           &link_info_len)) {
> +                       p_err("Registered %s but can't find link id: %s",
> +                             bpf_map__name(map), strerror(errno));
> +                       nr_errs++;
> +               } else if (pindir && pin_link(link, pindir, info.name)) {

Why do we have "pindir" and not a pinned path? Instead of taking a
directory name to concatenate, why not let the user specify the pinned
path directly, as we do for maps, programs, and links already? The
only existing use of dirname + concat I can think of is for "bpftool
prog loadall", but this is because we need one path to pin multiple
programs. Here we just have one, so let the user choose their path?

I would also avoid using "pin" too much in variable or function names.
I know we have "bpf_link__pin()", but I find it makes things confusing
between the concepts of pinned objects (through BPF_OBJ_PIN) and of
BPF links. How about "linkdir" or "linkpath" instead?

> +                       p_err("can't pin link %u for %s: %s",
> +                             link_info.id, info.name,
> +                             strerror(errno));
> +                       nr_errs++;
> +               } else
> +                       p_info("Registered %s %s map id %u link id %u",
> +                              get_kern_struct_ops_name(&info),
> +                              info.name, info.id, link_info.id);

Missing curly brackets on the "else" block.

I find it not easy to follow the logic in this long "else if..."
chain, it would probably feel more natural with simple "if"s and some
"goto"s to reach the bpf_link__disconnect() call below. But maybe this
is just me.

> +
> +               bpf_link__disconnect(link);
> +               bpf_link__destroy(link);
> +

Nit: We don't need this empty line.

>         }
>
>         bpf_object__close(obj);
> @@ -562,7 +620,7 @@ static int do_help(int argc, char **argv)
>         fprintf(stderr,
>                 "Usage: %1$s %2$s { show | list } [STRUCT_OPS_MAP]\n"
>                 "       %1$s %2$s dump [STRUCT_OPS_MAP]\n"
> -               "       %1$s %2$s register OBJ\n"
> +               "       %1$s %2$s register OBJ [PATH]\n"

This is not enough to understand what PATH means here. I'd use
something like "LINK_DIR", or preferably "LINK_PATH" if we let users
specify the full path. And we need to update the
bpftool-struct_ops.rst man page (under bpftool's Documentation/) to
explain what this optional argument is for, can you please take care
of this?

We usually have to update the bash completion too, but it seems that
it offers filenames multiple times already after "bpftool struct_ops
register", which is not intentional but covers completion for the new
argument.

>                 "       %1$s %2$s unregister STRUCT_OPS_MAP\n"
>                 "       %1$s %2$s help\n"
>                 "\n"
> --
> 2.34.1
>

Re: [PATCH bpf-next] bpftool: Register struct_ops with a link.

[Kui-Feng Lee]

On 4/18/23 15:55, Quentin Monnet wrote:
> On Tue, 18 Apr 2023 at 21:01, Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> You can include an optional path after specifying the object name for the
>> 'struct_ops register' subcommand.
>>
>> Since the commit 226bc6ae6405 ("Merge branch 'Transit between BPF TCP
>> congestion controls.'") has been accepted, it is now possible to create a
>> link for a struct_ops. This can be done by defining a struct_ops in
>> SEC(".struct_ops.link") to make libbpf returns a real link. If we don't pin
>> the links before leaving bpftool, they will disappear. To instruct bpftool
>> to pin the links in a directory with the names of the maps, we need to
>> provide the path of that directory.
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>>   tools/bpf/bpftool/struct_ops.c | 86 ++++++++++++++++++++++++++++------
>>   1 file changed, 72 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
>> index b389f4830e11..d1ae39f9d8df 100644
>> --- a/tools/bpf/bpftool/struct_ops.c
>> +++ b/tools/bpf/bpftool/struct_ops.c
>> @@ -475,21 +475,62 @@ static int do_unregister(int argc, char **argv)
>>          return cmd_retval(&res, true);
>>   }
>>
>> +static int pathname_concat(char *buf, int buf_sz, const char *path,
>> +                          const char *name)
>> +{
>> +       int len;
>> +
>> +       len = snprintf(buf, buf_sz, "%s/%s", path, name);
>> +       if (len < 0)
>> +               return -EINVAL;
>> +       if (len >= buf_sz)
>> +               return -ENAMETOOLONG;
>> +
>> +       return 0;
>> +}
> 
> This is nearly identical to the one in prog.c. If we do need this, we
> should move it to common.c and reuse it.

Got it!

> 
>> +
>> +static int pin_link(struct bpf_link *link, const char *pindir,
>> +                   const char *name)
>> +{
>> +       char pinfile[PATH_MAX];
>> +       int err;
>> +
>> +       err = pathname_concat(pinfile, sizeof(pinfile), pindir, name);
>> +       if (err)
>> +               return -1;
>> +
>> +       err = bpf_link__pin(link, pinfile);
>> +       if (err)
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> +
>>   static int do_register(int argc, char **argv)
>>   {
>>          LIBBPF_OPTS(bpf_object_open_opts, open_opts);
>> +       __u32 link_info_len = sizeof(struct bpf_link_info);
>> +       struct bpf_link_info link_info = {};
>>          struct bpf_map_info info = {};
>>          __u32 info_len = sizeof(info);
>>          int nr_errs = 0, nr_maps = 0;
>> +       const char *pindir = NULL;
>>          struct bpf_object *obj;
>>          struct bpf_link *link;
>>          struct bpf_map *map;
>>          const char *file;
>>
>> -       if (argc != 1)
>> +       if (argc != 1 && argc != 2)
>>                  usage();
>>
>>          file = GET_ARG();
>> +       if (argc == 1)
>> +               pindir = GET_ARG();
>> +
>> +       if (pindir && mount_bpffs_for_pin(pindir)) {
>> +               p_err("can't mount bpffs for pinning");
>> +               return -1;
>> +       }
>>
>>          if (verifier_logs)
>>                  /* log_level1 + log_level2 + stats, but not stable UAPI */
>> @@ -519,21 +560,38 @@ static int do_register(int argc, char **argv)
>>                  }
>>                  nr_maps++;
>>
>> -               bpf_link__disconnect(link);
>> -               bpf_link__destroy(link);
>> -
>> -               if (!bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
>> -                                           &info_len))
>> -                       p_info("Registered %s %s id %u",
>> -                              get_kern_struct_ops_name(&info),
>> -                              bpf_map__name(map),
>> -                              info.id);
>> -               else
>> +               if (bpf_map_get_info_by_fd(bpf_map__fd(map), &info,
>> +                                          &info_len)) {
>>                          /* Not p_err.  The struct_ops was attached
>>                           * successfully.
>>                           */
>> -                       p_info("Registered %s but can't find id: %s",
>> -                              bpf_map__name(map), strerror(errno));
>> +                       p_err("Registered %s but can't find id: %s",
>> +                             bpf_map__name(map), strerror(errno));
> 
> See comment right above: p_info() is probably enough here. If for some
> reason we do need to switch to an error message and change the
> existing behaviour, can you please motivate it and make it a separate
> commit (and update the comment)?

Ok! I will revert this change concerning about behavior changing.

> 
>> +                       nr_errs++;
>> +               } else if (!(bpf_map__map_flags(map) & BPF_F_LINK)) {
>> +                       p_info("Registered %s %s id %u",
>> +                              get_kern_struct_ops_name(&info),
>> +                              info.name,
>> +                              info.id);
>> +               } else if (bpf_link_get_info_by_fd(bpf_link__fd(link),
>> +                                                  &link_info,
>> +                                           &link_info_len)) {
>> +                       p_err("Registered %s but can't find link id: %s",
>> +                             bpf_map__name(map), strerror(errno));
>> +                       nr_errs++;
>> +               } else if (pindir && pin_link(link, pindir, info.name)) {
> 
> Why do we have "pindir" and not a pinned path? Instead of taking a
> directory name to concatenate, why not let the user specify the pinned
> path directly, as we do for maps, programs, and links already? The
> only existing use of dirname + concat I can think of is for "bpftool
> prog loadall", but this is because we need one path to pin multiple
> programs. Here we just have one, so let the user choose their path?

We could have multiple struct_ops in an object file as well.

> 
> I would also avoid using "pin" too much in variable or function names.
> I know we have "bpf_link__pin()", but I find it makes things confusing
> between the concepts of pinned objects (through BPF_OBJ_PIN) and of
> BPF links. How about "linkdir" or "linkpath" instead?

It sounds good!

> 
>> +                       p_err("can't pin link %u for %s: %s",
>> +                             link_info.id, info.name,
>> +                             strerror(errno));
>> +                       nr_errs++;
>> +               } else
>> +                       p_info("Registered %s %s map id %u link id %u",
>> +                              get_kern_struct_ops_name(&info),
>> +                              info.name, info.id, link_info.id);
> 
> Missing curly brackets on the "else" block.
> 
> I find it not easy to follow the logic in this long "else if..."
> chain, it would probably feel more natural with simple "if"s and some
> "goto"s to reach the bpf_link__disconnect() call below. But maybe this
> is just me.

I agree actually! I implemented it with goto first, but move to if-else 
to following the style I found in map.c. If you think goto is better, I 
will be glad to move to it.

> 
>> +
>> +               bpf_link__disconnect(link);
>> +               bpf_link__destroy(link);
>> +
> 
> Nit: We don't need this empty line.
> 

Got it!

>>          }
>>
>>          bpf_object__close(obj);
>> @@ -562,7 +620,7 @@ static int do_help(int argc, char **argv)
>>          fprintf(stderr,
>>                  "Usage: %1$s %2$s { show | list } [STRUCT_OPS_MAP]\n"
>>                  "       %1$s %2$s dump [STRUCT_OPS_MAP]\n"
>> -               "       %1$s %2$s register OBJ\n"
>> +               "       %1$s %2$s register OBJ [PATH]\n"
> 
> This is not enough to understand what PATH means here. I'd use
> something like "LINK_DIR", or preferably "LINK_PATH" if we let users
> specify the full path. And we need to update the
> bpftool-struct_ops.rst man page (under bpftool's Documentation/) to
> explain what this optional argument is for, can you please take care
> of this?

Sure!

> 
> We usually have to update the bash completion too, but it seems that
> it offers filenames multiple times already after "bpftool struct_ops
> register", which is not intentional but covers completion for the new
> argument.

I didn'know about bash completion before.
Good to know!

> 
>>                  "       %1$s %2$s unregister STRUCT_OPS_MAP\n"
>>                  "       %1$s %2$s help\n"
>>                  "\n"
>> --
>> 2.34.1
>>