On Wed, 19 Apr 2023 at 03:56, Kui-Feng Lee <thinker.li@xxxxxxxxx> 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@xxxxxxxx>
Right I'd forgotten we could register several struct_ops at once with
the command. OK then, makes sense to pass a directory and pin with the
existing names in that case.
This patch looks all good, apart from a few indent nitpicks.
> ---
> tools/bpf/bpftool/common.c | 14 +++++++
> tools/bpf/bpftool/main.h | 3 ++
> tools/bpf/bpftool/prog.c | 13 ------
> tools/bpf/bpftool/struct_ops.c | 76 ++++++++++++++++++++++++++++------
> 4 files changed, 80 insertions(+), 26 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index 5a73ccf14332..1360c82ae732 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -1091,3 +1091,17 @@ const char *bpf_attach_type_input_str(enum bpf_attach_type t)
> default: return libbpf_bpf_attach_type_str(t);
> }
> }
> +
> +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;
> +}
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index 0ef373cef4c7..f09853f24422 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -262,4 +262,7 @@ static inline bool hashmap__empty(struct hashmap *map)
> return map ? hashmap__size(map) == 0 : true;
> }
>
> +int pathname_concat(char *buf, int buf_sz, const char *path,
> + const char *name);
> +
> #endif
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index afbe3ec342c8..6024b7316875 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1472,19 +1472,6 @@ auto_attach_program(struct bpf_program *prog, const char *path)
> return err;
> }
>
> -static int pathname_concat(char *buf, size_t 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 ((size_t)len >= buf_sz)
> - return -ENAMETOOLONG;
> -
> - return 0;
> -}
> -
> static int
> auto_attach_programs(struct bpf_object *obj, const char *path)
> {
> diff --git a/tools/bpf/bpftool/struct_ops.c b/tools/bpf/bpftool/struct_ops.c
> index b389f4830e11..41643756e400 100644
> --- a/tools/bpf/bpftool/struct_ops.c
> +++ b/tools/bpf/bpftool/struct_ops.c
> @@ -475,21 +475,48 @@ static int do_unregister(int argc, char **argv)
> return cmd_retval(&res, true);
> }
>
> +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 bpf_link__pin(link, pinfile);" would work as well. But I don't
mind much.
> +
> + 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 *linkdir = 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)
> + linkdir = GET_ARG();
> +
> + if (linkdir && mount_bpffs_for_pin(linkdir)) {
> + p_err("can't mount bpffs for pinning");
> + return -1;
> + }
>
> if (verifier_logs)
> /* log_level1 + log_level2 + stats, but not stable UAPI */
> @@ -519,21 +546,44 @@ 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));
> + bpf_map__name(map), strerror(errno));
I think the indent was correct and this change results from your
previous version with "p_err()"?
> + goto clean_link;
> + }
> + 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);
> + goto clean_link;
> + }
> + if (bpf_link_get_info_by_fd(bpf_link__fd(link),
> + &link_info,
Please fix the indent.
> + &link_info_len)) {
> + p_err("Registered %s but can't find link id: %s",
> + bpf_map__name(map), strerror(errno));
> + nr_errs++;
> + goto clean_link;
> + }
> + if (linkdir && pin_link(link, linkdir, info.name)) {
> + p_err("can't pin link %u for %s: %s",
> + link_info.id, info.name,
> + strerror(errno));
> + nr_errs++;
> + goto clean_link;
> + }
> + p_info("Registered %s %s map id %u link id %u",
> + get_kern_struct_ops_name(&info),
> + info.name, info.id, link_info.id);
> +
> +clean_link:
> + bpf_link__disconnect(link);
> + bpf_link__destroy(link);
> }
>
> bpf_object__close(obj);
> @@ -562,7 +612,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 [LINK_DIR]\n"
> " %1$s %2$s unregister STRUCT_OPS_MAP\n"
> " %1$s %2$s help\n"
> "\n"
> --
> 2.34.1
>
