* [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link.
@ 2023-04-19 2:56 Kui-Feng Lee
2023-04-19 2:56 ` [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand Kui-Feng Lee
2023-04-19 23:40 ` [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Quentin Monnet
0 siblings, 2 replies; 6+ messages in thread
From: Kui-Feng Lee @ 2023-04-19 2:56 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, yhs; +Cc: 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/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 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));
+ 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,
+ &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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand.
2023-04-19 2:56 [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Kui-Feng Lee
@ 2023-04-19 2:56 ` Kui-Feng Lee
2023-04-19 23:44 ` Quentin Monnet
2023-04-19 23:40 ` [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Quentin Monnet
1 sibling, 1 reply; 6+ messages in thread
From: Kui-Feng Lee @ 2023-04-19 2:56 UTC (permalink / raw)
To: bpf, ast, martin.lau, song, kernel-team, andrii, yhs; +Cc: Kui-Feng Lee
The "struct_ops register" subcommand now allows for an optional *LINK_DIR*
to be included. This specifies the directory path where bpftool will pin
struct_ops links with the same name as their corresponding map names.
Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
---
tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
index ee53a122c0c7..2111c9550938 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
@@ -51,10 +51,14 @@ DESCRIPTION
for the given struct_ops. Otherwise, it dumps all struct_ops
currently existing in the system.
- **bpftool struct_ops register** *OBJ*
+ **bpftool struct_ops register** *OBJ* [*LINK_DIR*]
Register bpf struct_ops from *OBJ*. All struct_ops under
- the ELF section ".struct_ops" will be registered to
- its kernel subsystem.
+ the ELF section ".struct_ops" and ".struct_ops.link" will
+ be registered to its kernel subsystem. For each
+ struct_ops in the ".struct_ops.link" section, a link
+ will be created. You can give *LINK_DIR* to provide a
+ directory path where these links will be pinned with the
+ same name as their corresponding map name.
**bpftool struct_ops unregister** *STRUCT_OPS_MAP*
Unregister the *STRUCT_OPS_MAP* from the kernel subsystem.
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link.
2023-04-19 2:56 [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Kui-Feng Lee
2023-04-19 2:56 ` [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand Kui-Feng Lee
@ 2023-04-19 23:40 ` Quentin Monnet
2023-04-20 0:22 ` Kui-Feng Lee
1 sibling, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2023-04-19 23:40 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, yhs, Kui-Feng Lee
On Wed, 19 Apr 2023 at 03:56, 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>
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand.
2023-04-19 2:56 ` [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand Kui-Feng Lee
@ 2023-04-19 23:44 ` Quentin Monnet
2023-04-20 0:20 ` Kui-Feng Lee
0 siblings, 1 reply; 6+ messages in thread
From: Quentin Monnet @ 2023-04-19 23:44 UTC (permalink / raw)
To: Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, yhs, Kui-Feng Lee
On Wed, 19 Apr 2023 at 03:56, Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> The "struct_ops register" subcommand now allows for an optional *LINK_DIR*
> to be included. This specifies the directory path where bpftool will pin
> struct_ops links with the same name as their corresponding map names.
>
> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
> ---
> tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
> index ee53a122c0c7..2111c9550938 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
> @@ -51,10 +51,14 @@ DESCRIPTION
> for the given struct_ops. Otherwise, it dumps all struct_ops
> currently existing in the system.
>
> - **bpftool struct_ops register** *OBJ*
> + **bpftool struct_ops register** *OBJ* [*LINK_DIR*]
> Register bpf struct_ops from *OBJ*. All struct_ops under
> - the ELF section ".struct_ops" will be registered to
> - its kernel subsystem.
> + the ELF section ".struct_ops" and ".struct_ops.link" will
> + be registered to its kernel subsystem. For each
> + struct_ops in the ".struct_ops.link" section, a link
> + will be created. You can give *LINK_DIR* to provide a
> + directory path where these links will be pinned with the
> + same name as their corresponding map name.
>
> **bpftool struct_ops unregister** *STRUCT_OPS_MAP*
> Unregister the *STRUCT_OPS_MAP* from the kernel subsystem.
Thanks! Since there are some nits to address on the first patch
anyway, would you mind updating the command summary earlier in this
file as well, please? In section "STRUCT_OPS COMMANDS".
Thanks,
Quentin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand.
2023-04-19 23:44 ` Quentin Monnet
@ 2023-04-20 0:20 ` Kui-Feng Lee
0 siblings, 0 replies; 6+ messages in thread
From: Kui-Feng Lee @ 2023-04-20 0:20 UTC (permalink / raw)
To: Quentin Monnet, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, yhs, Kui-Feng Lee
On 4/19/23 16:44, Quentin Monnet wrote:
> On Wed, 19 Apr 2023 at 03:56, Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> The "struct_ops register" subcommand now allows for an optional *LINK_DIR*
>> to be included. This specifies the directory path where bpftool will pin
>> struct_ops links with the same name as their corresponding map names.
>>
>> Signed-off-by: Kui-Feng Lee <kuifeng@meta.com>
>> ---
>> tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
>> index ee53a122c0c7..2111c9550938 100644
>> --- a/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
>> +++ b/tools/bpf/bpftool/Documentation/bpftool-struct_ops.rst
>> @@ -51,10 +51,14 @@ DESCRIPTION
>> for the given struct_ops. Otherwise, it dumps all struct_ops
>> currently existing in the system.
>>
>> - **bpftool struct_ops register** *OBJ*
>> + **bpftool struct_ops register** *OBJ* [*LINK_DIR*]
>> Register bpf struct_ops from *OBJ*. All struct_ops under
>> - the ELF section ".struct_ops" will be registered to
>> - its kernel subsystem.
>> + the ELF section ".struct_ops" and ".struct_ops.link" will
>> + be registered to its kernel subsystem. For each
>> + struct_ops in the ".struct_ops.link" section, a link
>> + will be created. You can give *LINK_DIR* to provide a
>> + directory path where these links will be pinned with the
>> + same name as their corresponding map name.
>>
>> **bpftool struct_ops unregister** *STRUCT_OPS_MAP*
>> Unregister the *STRUCT_OPS_MAP* from the kernel subsystem.
>
> Thanks! Since there are some nits to address on the first patch
> anyway, would you mind updating the command summary earlier in this
> file as well, please? In section "STRUCT_OPS COMMANDS".
Sure!
>
> Thanks,
> Quentin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link.
2023-04-19 23:40 ` [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Quentin Monnet
@ 2023-04-20 0:22 ` Kui-Feng Lee
0 siblings, 0 replies; 6+ messages in thread
From: Kui-Feng Lee @ 2023-04-20 0:22 UTC (permalink / raw)
To: Quentin Monnet, Kui-Feng Lee
Cc: bpf, ast, martin.lau, song, kernel-team, andrii, yhs, Kui-Feng Lee
On 4/19/23 16:40, Quentin Monnet wrote:
> On Wed, 19 Apr 2023 at 03:56, 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>
>
> 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.
I will change to this.
>
>> +
>> + 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()"?
Yes, it is my bad!
>
>> + 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.
Sure
>
>> + &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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-20 0:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-19 2:56 [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Kui-Feng Lee
2023-04-19 2:56 ` [PATCH bpf-next v2 2/2] bpftool: Update doc to explain struct_ops register subcommand Kui-Feng Lee
2023-04-19 23:44 ` Quentin Monnet
2023-04-20 0:20 ` Kui-Feng Lee
2023-04-19 23:40 ` [PATCH bpf-next v2 1/2] bpftool: Register struct_ops with a link Quentin Monnet
2023-04-20 0:22 ` Kui-Feng Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).