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

[Kui-Feng Lee <sinquersw@gmail.com>]

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