All of lore.kernel.org
 help / color / mirror / Atom feed
* [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
@ 2022-09-27 11:21 Wang Yufen
  2022-09-27 11:21 ` [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load) Wang Yufen
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Wang Yufen @ 2022-09-27 11:21 UTC (permalink / raw)
  To: quentin, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm

Add auto_attach optional to support one-step load-attach-pin_link.

For example,
   $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach

   $ bpftool link
   26: tracing  name test1  tag f0da7d0058c00236  gpl
   	loaded_at 2022-09-09T21:39:49+0800  uid 0
   	xlated 88B  jited 55B  memlock 4096B  map_ids 3
   	btf_id 55
   28: kprobe  name test3  tag 002ef1bef0723833  gpl
   	loaded_at 2022-09-09T21:39:49+0800  uid 0
   	xlated 88B  jited 56B  memlock 4096B  map_ids 3
   	btf_id 55
   57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
   	loaded_at 2022-09-09T21:41:32+0800  uid 0
   	xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
   	btf_id 82

   $ bpftool link
   1: tracing  prog 26
   	prog_type tracing  attach_type trace_fentry
   3: perf_event  prog 28
   10: perf_event  prog 57

The autoattach optional can support tracepoints, k(ret)probes,
u(ret)probes.

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
v6 -> v7: add info msg print and update doc for the skip program
v5 -> v6: skip the programs not supporting auto-attach,
	  and change optional name from "auto_attach" to "autoattach"
v4 -> v5: some formatting nits of doc
v3 -> v4: rename functions, update doc, bash and do_help()
v2 -> v3: switch to extend prog load command instead of extend perf
v2: https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
 tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index c81362a..84eced8 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1453,6 +1453,72 @@ static int do_run(int argc, char **argv)
 	return ret;
 }
 
+static int
+auto_attach_program(struct bpf_program *prog, const char *path)
+{
+	struct bpf_link *link;
+	int err;
+
+	link = bpf_program__attach(prog);
+	if (!link)
+		return -1;
+
+	err = bpf_link__pin(link, path);
+	if (err) {
+		bpf_link__destroy(link);
+		return err;
+	}
+	return 0;
+}
+
+static int pathname_concat(const char *path, const char *name, char *buf)
+{
+	int len;
+
+	len = snprintf(buf, PATH_MAX, "%s/%s", path, name);
+	if (len < 0)
+		return -EINVAL;
+	if (len >= PATH_MAX)
+		return -ENAMETOOLONG;
+
+	return 0;
+}
+
+static int
+auto_attach_programs(struct bpf_object *obj, const char *path)
+{
+	struct bpf_program *prog;
+	char buf[PATH_MAX];
+	int err;
+
+	bpf_object__for_each_program(prog, obj) {
+		err = pathname_concat(path, bpf_program__name(prog), buf);
+		if (err)
+			goto err_unpin_programs;
+
+		err = auto_attach_program(prog, buf);
+		if (!err)
+			continue;
+		if (errno == EOPNOTSUPP)
+			p_info("Program %s does not support autoattach",
+			       bpf_program__name(prog));
+		else
+			goto err_unpin_programs;
+	}
+
+	return 0;
+
+err_unpin_programs:
+	while ((prog = bpf_object__prev_program(obj, prog))) {
+		if (pathname_concat(path, bpf_program__name(prog), buf))
+			continue;
+
+		bpf_program__unpin(prog, buf);
+	}
+
+	return err;
+}
+
 static int load_with_options(int argc, char **argv, bool first_prog_only)
 {
 	enum bpf_prog_type common_prog_type = BPF_PROG_TYPE_UNSPEC;
@@ -1464,6 +1530,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 	struct bpf_program *prog = NULL, *pos;
 	unsigned int old_map_fds = 0;
 	const char *pinmaps = NULL;
+	bool auto_attach = false;
 	struct bpf_object *obj;
 	struct bpf_map *map;
 	const char *pinfile;
@@ -1583,6 +1650,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, "autoattach")) {
+			auto_attach = true;
+			NEXT_ARG();
 		} else {
 			p_err("expected no more arguments, 'type', 'map' or 'dev', got: '%s'?",
 			      *argv);
@@ -1692,14 +1762,20 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
 			goto err_close_obj;
 		}
 
-		err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
+		if (auto_attach)
+			err = auto_attach_program(prog, pinfile);
+		else
+			err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
 		if (err) {
 			p_err("failed to pin program %s",
 			      bpf_program__section_name(prog));
 			goto err_close_obj;
 		}
 	} else {
-		err = bpf_object__pin_programs(obj, pinfile);
+		if (auto_attach)
+			err = auto_attach_programs(obj, pinfile);
+		else
+			err = bpf_object__pin_programs(obj, pinfile);
 		if (err) {
 			p_err("failed to pin all programs");
 			goto err_close_obj;
@@ -2338,6 +2414,7 @@ static int do_help(int argc, char **argv)
 		"                         [type TYPE] [dev NAME] \\\n"
 		"                         [map { idx IDX | name NAME } MAP]\\\n"
 		"                         [pinmaps MAP_DIR]\n"
+		"                         [autoattach]\n"
 		"       %1$s %2$s attach PROG ATTACH_TYPE [MAP]\n"
 		"       %1$s %2$s detach PROG ATTACH_TYPE [MAP]\n"
 		"       %1$s %2$s run PROG \\\n"
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load)
  2022-09-27 11:21 [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Wang Yufen
@ 2022-09-27 11:21 ` Wang Yufen
  2022-09-30 16:27   ` Quentin Monnet
  2022-09-27 11:21 ` [bpf-next v7 3/3] bpftool: Update the bash completion(add " Wang Yufen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Wang Yufen @ 2022-09-27 11:21 UTC (permalink / raw)
  To: quentin, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm

Add autoattach optional to prog load|loadall for supporting
one-step load-attach-pin_link.

Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
 tools/bpf/bpftool/Documentation/bpftool-prog.rst | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
index eb1b2a2..b81d3d9 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
@@ -31,7 +31,7 @@ PROG COMMANDS
 |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual** | **linum**}]
 |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes** | **linum**}]
 |	**bpftool** **prog pin** *PROG* *FILE*
-|	**bpftool** **prog** { **load** | **loadall** } *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*]
+|	**bpftool** **prog** { **load** | **loadall** } *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*] [**autoattach**]
 |	**bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
 |	**bpftool** **prog tracelog**
@@ -131,7 +131,7 @@ DESCRIPTION
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
 
-	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*]
+	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*] [**autoattach**]
 		  Load bpf program(s) from binary *OBJ* and pin as *PATH*.
 		  **bpftool prog load** pins only the first program from the
 		  *OBJ* as *PATH*. **bpftool prog loadall** pins all programs
@@ -150,6 +150,19 @@ DESCRIPTION
 		  Optional **pinmaps** argument can be provided to pin all
 		  maps under *MAP_DIR* directory.
 
+		  If **autoattach** is specified program will be attached
+		  before pin. In that case, only the link (representing the
+		  program attached to its hook) is pinned, not the program as
+		  such, so the path won't show in "**bpftool prog show -f**",
+		  only show in "**bpftool link show -f**". Also, this only works
+		  when bpftool (libbpf) is able to infer all necessary information
+		  from the objectfile, in particular, it's not supported for all
+		  program types. If the *OBJ* contains multiple programs and
+		  *loadall* is used, if the program A in these programs does not
+		  support auto-attachi, will skip program A(do no operation on
+		  program A), print a info message such as "Program A does not
+		  support autoattach", and continue to autoattach the next program.
+
 		  Note: *PATH* must be located in *bpffs* mount. It must not
 		  contain a dot character ('.'), which is reserved for future
 		  extensions of *bpffs*.
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [bpf-next v7 3/3] bpftool: Update the bash completion(add autoattach to prog load)
  2022-09-27 11:21 [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Wang Yufen
  2022-09-27 11:21 ` [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load) Wang Yufen
@ 2022-09-27 11:21 ` Wang Yufen
  2022-09-30 16:26 ` [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Quentin Monnet
  2022-09-30 20:55 ` Andrii Nakryiko
  3 siblings, 0 replies; 10+ messages in thread
From: Wang Yufen @ 2022-09-27 11:21 UTC (permalink / raw)
  To: quentin, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm

Add autoattach optional to prog load|loadall for supporting
one-step load-attach-pin_link.

Signed-off-by: Wang Yufen <wangyufen@huawei.com>
---
 tools/bpf/bpftool/bash-completion/bpftool | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index dc1641e..2957b42 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -505,6 +505,7 @@ _bpftool()
                             _bpftool_once_attr 'type'
                             _bpftool_once_attr 'dev'
                             _bpftool_once_attr 'pinmaps'
+                            _bpftool_once_attr 'autoattach'
                             return 0
                             ;;
                     esac
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
  2022-09-27 11:21 [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Wang Yufen
  2022-09-27 11:21 ` [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load) Wang Yufen
  2022-09-27 11:21 ` [bpf-next v7 3/3] bpftool: Update the bash completion(add " Wang Yufen
@ 2022-09-30 16:26 ` Quentin Monnet
  2022-10-08  5:16   ` wangyufen
  2022-09-30 20:55 ` Andrii Nakryiko
  3 siblings, 1 reply; 10+ messages in thread
From: Quentin Monnet @ 2022-09-30 16:26 UTC (permalink / raw)
  To: Wang Yufen, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm

Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com>
> Add auto_attach optional to support one-step load-attach-pin_link.

Nit: Now "autoattach" instead of "auto_attach". Same in commit title.

> 
> For example,
>    $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach
> 
>    $ bpftool link
>    26: tracing  name test1  tag f0da7d0058c00236  gpl
>    	loaded_at 2022-09-09T21:39:49+0800  uid 0
>    	xlated 88B  jited 55B  memlock 4096B  map_ids 3
>    	btf_id 55
>    28: kprobe  name test3  tag 002ef1bef0723833  gpl
>    	loaded_at 2022-09-09T21:39:49+0800  uid 0
>    	xlated 88B  jited 56B  memlock 4096B  map_ids 3
>    	btf_id 55
>    57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
>    	loaded_at 2022-09-09T21:41:32+0800  uid 0
>    	xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
>    	btf_id 82
> 
>    $ bpftool link
>    1: tracing  prog 26
>    	prog_type tracing  attach_type trace_fentry
>    3: perf_event  prog 28
>    10: perf_event  prog 57
> 
> The autoattach optional can support tracepoints, k(ret)probes,
> u(ret)probes.
> 
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> ---
> v6 -> v7: add info msg print and update doc for the skip program
> v5 -> v6: skip the programs not supporting auto-attach,
> 	  and change optional name from "auto_attach" to "autoattach"
> v4 -> v5: some formatting nits of doc
> v3 -> v4: rename functions, update doc, bash and do_help()
> v2 -> v3: switch to extend prog load command instead of extend perf
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
>  tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index c81362a..84eced8 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1453,6 +1453,72 @@ static int do_run(int argc, char **argv)
>  	return ret;
>  }
>  
> +static int
> +auto_attach_program(struct bpf_program *prog, const char *path)
> +{
> +	struct bpf_link *link;
> +	int err;
> +
> +	link = bpf_program__attach(prog);
> +	if (!link)
> +		return -1;
> +
> +	err = bpf_link__pin(link, path);
> +	if (err) {
> +		bpf_link__destroy(link);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static int pathname_concat(const char *path, const char *name, char *buf)
> +{
> +	int len;
> +
> +	len = snprintf(buf, PATH_MAX, "%s/%s", path, name);
> +	if (len < 0)
> +		return -EINVAL;
> +	if (len >= PATH_MAX)
> +		return -ENAMETOOLONG;
> +
> +	return 0;
> +}
> +
> +static int
> +auto_attach_programs(struct bpf_object *obj, const char *path)
> +{
> +	struct bpf_program *prog;
> +	char buf[PATH_MAX];
> +	int err;
> +
> +	bpf_object__for_each_program(prog, obj) {
> +		err = pathname_concat(path, bpf_program__name(prog), buf);
> +		if (err)
> +			goto err_unpin_programs;
> +
> +		err = auto_attach_program(prog, buf);
> +		if (!err)
> +			continue;
> +		if (errno == EOPNOTSUPP)
> +			p_info("Program %s does not support autoattach",
> +			       bpf_program__name(prog));
> +		else
> +			goto err_unpin_programs
With this code, if auto-attach fails, then we skip this program and move
on to the next. That's an improvement, but in that case the program
won't remain loaded in the kernel after bpftool exits. My suggestion in
my previous message (sorry if it was not clear) was to fall back to
regular pinning in that case (bpf_obj_pin()), along with the p_info()
message, so we can have the program pinned but not attached and let the
user know. If regular pinning fails as well, then we should unpin all
and error out, for consistency with bpf_object__pin_programs().

And in that case, the (errno == EOPNOTSUPP) with fallback to regular
pinning could maybe be moved into auto_attach_program(), so that
auto-attaching single programs can use the fallback too?

Thanks,
Quentin

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load)
  2022-09-27 11:21 ` [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load) Wang Yufen
@ 2022-09-30 16:27   ` Quentin Monnet
  0 siblings, 0 replies; 10+ messages in thread
From: Quentin Monnet @ 2022-09-30 16:27 UTC (permalink / raw)
  To: Wang Yufen, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm

Tue Sep 27 2022 12:21:15 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com>
> Add autoattach optional to prog load|loadall for supporting
> one-step load-attach-pin_link.
> 
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> ---
>  tools/bpf/bpftool/Documentation/bpftool-prog.rst | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> index eb1b2a2..b81d3d9 100644
> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> @@ -31,7 +31,7 @@ PROG COMMANDS
>  |	**bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual** | **linum**}]
>  |	**bpftool** **prog dump jited**  *PROG* [{**file** *FILE* | **opcodes** | **linum**}]
>  |	**bpftool** **prog pin** *PROG* *FILE*
> -|	**bpftool** **prog** { **load** | **loadall** } *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*]
> +|	**bpftool** **prog** { **load** | **loadall** } *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*] [**autoattach**]
>  |	**bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>  |	**bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>  |	**bpftool** **prog tracelog**
> @@ -131,7 +131,7 @@ DESCRIPTION
>  		  contain a dot character ('.'), which is reserved for future
>  		  extensions of *bpffs*.
>  
> -	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*]
> +	**bpftool prog { load | loadall }** *OBJ* *PATH* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*] [**pinmaps** *MAP_DIR*] [**autoattach**]
>  		  Load bpf program(s) from binary *OBJ* and pin as *PATH*.
>  		  **bpftool prog load** pins only the first program from the
>  		  *OBJ* as *PATH*. **bpftool prog loadall** pins all programs
> @@ -150,6 +150,19 @@ DESCRIPTION
>  		  Optional **pinmaps** argument can be provided to pin all
>  		  maps under *MAP_DIR* directory.
>  
> +		  If **autoattach** is specified program will be attached
> +		  before pin. In that case, only the link (representing the
> +		  program attached to its hook) is pinned, not the program as
> +		  such, so the path won't show in "**bpftool prog show -f**",

Nit: no need for double-quotes, the ** markup is enough.

> +		  only show in "**bpftool link show -f**". Also, this only works
> +		  when bpftool (libbpf) is able to infer all necessary information
> +		  from the objectfile, in particular, it's not supported for all

s/objectfile/object file/

> +		  program types. If the *OBJ* contains multiple programs and
> +		  *loadall* is used, if the program A in these programs does not

**loadall** (keyword)

> +		  support auto-attachi, will skip program A(do no operation on

auto-attach

> +		  program A), print a info message such as "Program A does not
> +		  support autoattach", and continue to autoattach the next program.

continue to auto-attach

> +
>  		  Note: *PATH* must be located in *bpffs* mount. It must not
>  		  contain a dot character ('.'), which is reserved for future
>  		  extensions of *bpffs*.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
  2022-09-27 11:21 [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Wang Yufen
                   ` (2 preceding siblings ...)
  2022-09-30 16:26 ` [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Quentin Monnet
@ 2022-09-30 20:55 ` Andrii Nakryiko
  2022-10-08  5:22   ` wangyufen
  3 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2022-09-30 20:55 UTC (permalink / raw)
  To: Wang Yufen
  Cc: quentin, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix, bpf, linux-kernel, netdev, llvm

On Tue, Sep 27, 2022 at 4:00 AM Wang Yufen <wangyufen@huawei.com> wrote:
>
> Add auto_attach optional to support one-step load-attach-pin_link.
>
> For example,
>    $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach
>
>    $ bpftool link
>    26: tracing  name test1  tag f0da7d0058c00236  gpl
>         loaded_at 2022-09-09T21:39:49+0800  uid 0
>         xlated 88B  jited 55B  memlock 4096B  map_ids 3
>         btf_id 55
>    28: kprobe  name test3  tag 002ef1bef0723833  gpl
>         loaded_at 2022-09-09T21:39:49+0800  uid 0
>         xlated 88B  jited 56B  memlock 4096B  map_ids 3
>         btf_id 55
>    57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
>         loaded_at 2022-09-09T21:41:32+0800  uid 0
>         xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
>         btf_id 82
>
>    $ bpftool link
>    1: tracing  prog 26
>         prog_type tracing  attach_type trace_fentry
>    3: perf_event  prog 28
>    10: perf_event  prog 57
>
> The autoattach optional can support tracepoints, k(ret)probes,
> u(ret)probes.
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
> ---

For next revision, please also attach cover letter describing the
overall goal of the patch set (and that's where the version log
between revisions is put as well).


> v6 -> v7: add info msg print and update doc for the skip program
> v5 -> v6: skip the programs not supporting auto-attach,
>           and change optional name from "auto_attach" to "autoattach"
> v4 -> v5: some formatting nits of doc
> v3 -> v4: rename functions, update doc, bash and do_help()
> v2 -> v3: switch to extend prog load command instead of extend perf
> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
>  tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 79 insertions(+), 2 deletions(-)
>

[...]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
  2022-09-30 16:26 ` [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Quentin Monnet
@ 2022-10-08  5:16   ` wangyufen
  2022-10-10  8:40     ` Quentin Monnet
  0 siblings, 1 reply; 10+ messages in thread
From: wangyufen @ 2022-10-08  5:16 UTC (permalink / raw)
  To: Quentin Monnet, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm


在 2022/10/1 0:26, Quentin Monnet 写道:
> Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com>
>> Add auto_attach optional to support one-step load-attach-pin_link.
> Nit: Now "autoattach" instead of "auto_attach". Same in commit title.
will change in v8, thanks.
>
>> For example,
>>     $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach
>>
>>     $ bpftool link
>>     26: tracing  name test1  tag f0da7d0058c00236  gpl
>>     	loaded_at 2022-09-09T21:39:49+0800  uid 0
>>     	xlated 88B  jited 55B  memlock 4096B  map_ids 3
>>     	btf_id 55
>>     28: kprobe  name test3  tag 002ef1bef0723833  gpl
>>     	loaded_at 2022-09-09T21:39:49+0800  uid 0
>>     	xlated 88B  jited 56B  memlock 4096B  map_ids 3
>>     	btf_id 55
>>     57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
>>     	loaded_at 2022-09-09T21:41:32+0800  uid 0
>>     	xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
>>     	btf_id 82
>>
>>     $ bpftool link
>>     1: tracing  prog 26
>>     	prog_type tracing  attach_type trace_fentry
>>     3: perf_event  prog 28
>>     10: perf_event  prog 57
>>
>> The autoattach optional can support tracepoints, k(ret)probes,
>> u(ret)probes.
>>
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>> ---
>> v6 -> v7: add info msg print and update doc for the skip program
>> v5 -> v6: skip the programs not supporting auto-attach,
>> 	  and change optional name from "auto_attach" to "autoattach"
>> v4 -> v5: some formatting nits of doc
>> v3 -> v4: rename functions, update doc, bash and do_help()
>> v2 -> v3: switch to extend prog load command instead of extend perf
>> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
>> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
>>   tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 79 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>> index c81362a..84eced8 100644
>> --- a/tools/bpf/bpftool/prog.c
>> +++ b/tools/bpf/bpftool/prog.c
>> @@ -1453,6 +1453,72 @@ static int do_run(int argc, char **argv)
>>   	return ret;
>>   }
>>   
>> +static int
>> +auto_attach_program(struct bpf_program *prog, const char *path)
>> +{
>> +	struct bpf_link *link;
>> +	int err;
>> +
>> +	link = bpf_program__attach(prog);
>> +	if (!link)
>> +		return -1;
>> +
>> +	err = bpf_link__pin(link, path);
>> +	if (err) {
>> +		bpf_link__destroy(link);
>> +		return err;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int pathname_concat(const char *path, const char *name, char *buf)
>> +{
>> +	int len;
>> +
>> +	len = snprintf(buf, PATH_MAX, "%s/%s", path, name);
>> +	if (len < 0)
>> +		return -EINVAL;
>> +	if (len >= PATH_MAX)
>> +		return -ENAMETOOLONG;
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +auto_attach_programs(struct bpf_object *obj, const char *path)
>> +{
>> +	struct bpf_program *prog;
>> +	char buf[PATH_MAX];
>> +	int err;
>> +
>> +	bpf_object__for_each_program(prog, obj) {
>> +		err = pathname_concat(path, bpf_program__name(prog), buf);
>> +		if (err)
>> +			goto err_unpin_programs;
>> +
>> +		err = auto_attach_program(prog, buf);
>> +		if (!err)
>> +			continue;
>> +		if (errno == EOPNOTSUPP)
>> +			p_info("Program %s does not support autoattach",
>> +			       bpf_program__name(prog));
>> +		else
>> +			goto err_unpin_programs
> With this code, if auto-attach fails, then we skip this program and move
> on to the next. That's an improvement, but in that case the program
> won't remain loaded in the kernel after bpftool exits. My suggestion in
> my previous message (sorry if it was not clear) was to fall back to
> regular pinning in that case (bpf_obj_pin()), along with the p_info()
> message, so we can have the program pinned but not attached and let the
> user know. If regular pinning fails as well, then we should unpin all
> and error out, for consistency with bpf_object__pin_programs().
>
> And in that case, the (errno == EOPNOTSUPP) with fallback to regular
> pinning could maybe be moved into auto_attach_program(), so that
> auto-attaching single programs can use the fallback too?
>
> Thanks,
> Quentin

If I understand correctly, can we just check link?  as following:

--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1460,9 +1460,10 @@ static int do_run(int argc, char **argv)
         int err;
  
         link = bpf_program__attach(prog);
-       if (!link)
-               return -1;
-
+       if (!link) {
+               p_info("Program %s attach failed", bpf_program__name(prog));
+               return bpf_obj_pin(bpf_program__fd(prog), path);
+       }
         err = bpf_link__pin(link, path);
         if (err) {
                 bpf_link__destroy(link);
@@ -1499,9 +1500,6 @@ static int pathname_concat(const char *path, const char *name, char *buf)
                 err = auto_attach_program(prog, buf);
                 if (!err)
                         continue;
-               if (errno == EOPNOTSUPP)
-                       p_info("Program %s does not support autoattach",
-                              bpf_program__name(prog));
                 else
                         goto err_unpin_programs;
         }


and the doc is modified as follows:

If the program does not support autoattach, will do regular pin along with an
info message such as "Program %s attach failed". If the *OBJ* contains multiple
programs and **loadall** is used, if the program A in these programs does not
support autoattach, the program A will do regular pin along with an info message,
and continue to autoattach the next program.

Thanks,
Wang


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
  2022-09-30 20:55 ` Andrii Nakryiko
@ 2022-10-08  5:22   ` wangyufen
  0 siblings, 0 replies; 10+ messages in thread
From: wangyufen @ 2022-10-08  5:22 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: quentin, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix, bpf, linux-kernel, netdev, llvm


在 2022/10/1 4:55, Andrii Nakryiko 写道:
> On Tue, Sep 27, 2022 at 4:00 AM Wang Yufen <wangyufen@huawei.com> wrote:
>> Add auto_attach optional to support one-step load-attach-pin_link.
>>
>> For example,
>>     $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach
>>
>>     $ bpftool link
>>     26: tracing  name test1  tag f0da7d0058c00236  gpl
>>          loaded_at 2022-09-09T21:39:49+0800  uid 0
>>          xlated 88B  jited 55B  memlock 4096B  map_ids 3
>>          btf_id 55
>>     28: kprobe  name test3  tag 002ef1bef0723833  gpl
>>          loaded_at 2022-09-09T21:39:49+0800  uid 0
>>          xlated 88B  jited 56B  memlock 4096B  map_ids 3
>>          btf_id 55
>>     57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
>>          loaded_at 2022-09-09T21:41:32+0800  uid 0
>>          xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
>>          btf_id 82
>>
>>     $ bpftool link
>>     1: tracing  prog 26
>>          prog_type tracing  attach_type trace_fentry
>>     3: perf_event  prog 28
>>     10: perf_event  prog 57
>>
>> The autoattach optional can support tracepoints, k(ret)probes,
>> u(ret)probes.
>>
>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>> ---
> For next revision, please also attach cover letter describing the
> overall goal of the patch set (and that's where the version log
> between revisions is put as well).

Thanks, will add a cover letter in v8.

>
>
>> v6 -> v7: add info msg print and update doc for the skip program
>> v5 -> v6: skip the programs not supporting auto-attach,
>>            and change optional name from "auto_attach" to "autoattach"
>> v4 -> v5: some formatting nits of doc
>> v3 -> v4: rename functions, update doc, bash and do_help()
>> v2 -> v3: switch to extend prog load command instead of extend perf
>> v2: https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
>> v1: https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
>>   tools/bpf/bpftool/prog.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 79 insertions(+), 2 deletions(-)
>>
> [...]
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
  2022-10-08  5:16   ` wangyufen
@ 2022-10-10  8:40     ` Quentin Monnet
  2022-10-10  8:58       ` wangyufen
  0 siblings, 1 reply; 10+ messages in thread
From: Quentin Monnet @ 2022-10-10  8:40 UTC (permalink / raw)
  To: wangyufen, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm

Sat Oct 08 2022 06:16:42 GMT+0100 ~ wangyufen <wangyufen@huawei.com>
> 
> 在 2022/10/1 0:26, Quentin Monnet 写道:
>> Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com>
>>> Add auto_attach optional to support one-step load-attach-pin_link.
>> Nit: Now "autoattach" instead of "auto_attach". Same in commit title.
> will change in v8, thanks.
>>
>>> For example,
>>>     $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach
>>>
>>>     $ bpftool link
>>>     26: tracing  name test1  tag f0da7d0058c00236  gpl
>>>         loaded_at 2022-09-09T21:39:49+0800  uid 0
>>>         xlated 88B  jited 55B  memlock 4096B  map_ids 3
>>>         btf_id 55
>>>     28: kprobe  name test3  tag 002ef1bef0723833  gpl
>>>         loaded_at 2022-09-09T21:39:49+0800  uid 0
>>>         xlated 88B  jited 56B  memlock 4096B  map_ids 3
>>>         btf_id 55
>>>     57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
>>>         loaded_at 2022-09-09T21:41:32+0800  uid 0
>>>         xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
>>>         btf_id 82
>>>
>>>     $ bpftool link
>>>     1: tracing  prog 26
>>>         prog_type tracing  attach_type trace_fentry
>>>     3: perf_event  prog 28
>>>     10: perf_event  prog 57
>>>
>>> The autoattach optional can support tracepoints, k(ret)probes,
>>> u(ret)probes.
>>>
>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>> ---
>>> v6 -> v7: add info msg print and update doc for the skip program
>>> v5 -> v6: skip the programs not supporting auto-attach,
>>>       and change optional name from "auto_attach" to "autoattach"
>>> v4 -> v5: some formatting nits of doc
>>> v3 -> v4: rename functions, update doc, bash and do_help()
>>> v2 -> v3: switch to extend prog load command instead of extend perf
>>> v2:
>>> https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
>>> v1:
>>> https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
>>>   tools/bpf/bpftool/prog.c | 81
>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 79 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>>> index c81362a..84eced8 100644
>>> --- a/tools/bpf/bpftool/prog.c
>>> +++ b/tools/bpf/bpftool/prog.c
>>> @@ -1453,6 +1453,72 @@ static int do_run(int argc, char **argv)
>>>       return ret;
>>>   }
>>>   +static int
>>> +auto_attach_program(struct bpf_program *prog, const char *path)
>>> +{
>>> +    struct bpf_link *link;
>>> +    int err;
>>> +
>>> +    link = bpf_program__attach(prog);
>>> +    if (!link)
>>> +        return -1;
>>> +
>>> +    err = bpf_link__pin(link, path);
>>> +    if (err) {
>>> +        bpf_link__destroy(link);
>>> +        return err;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static int pathname_concat(const char *path, const char *name, char
>>> *buf)
>>> +{
>>> +    int len;
>>> +
>>> +    len = snprintf(buf, PATH_MAX, "%s/%s", path, name);
>>> +    if (len < 0)
>>> +        return -EINVAL;
>>> +    if (len >= PATH_MAX)
>>> +        return -ENAMETOOLONG;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int
>>> +auto_attach_programs(struct bpf_object *obj, const char *path)
>>> +{
>>> +    struct bpf_program *prog;
>>> +    char buf[PATH_MAX];
>>> +    int err;
>>> +
>>> +    bpf_object__for_each_program(prog, obj) {
>>> +        err = pathname_concat(path, bpf_program__name(prog), buf);
>>> +        if (err)
>>> +            goto err_unpin_programs;
>>> +
>>> +        err = auto_attach_program(prog, buf);
>>> +        if (!err)
>>> +            continue;
>>> +        if (errno == EOPNOTSUPP)
>>> +            p_info("Program %s does not support autoattach",
>>> +                   bpf_program__name(prog));
>>> +        else
>>> +            goto err_unpin_programs
>> With this code, if auto-attach fails, then we skip this program and move
>> on to the next. That's an improvement, but in that case the program
>> won't remain loaded in the kernel after bpftool exits. My suggestion in
>> my previous message (sorry if it was not clear) was to fall back to
>> regular pinning in that case (bpf_obj_pin()), along with the p_info()
>> message, so we can have the program pinned but not attached and let the
>> user know. If regular pinning fails as well, then we should unpin all
>> and error out, for consistency with bpf_object__pin_programs().
>>
>> And in that case, the (errno == EOPNOTSUPP) with fallback to regular
>> pinning could maybe be moved into auto_attach_program(), so that
>> auto-attaching single programs can use the fallback too?
>>
>> Thanks,
>> Quentin
> 
> If I understand correctly, can we just check link?  as following:

Yes, this is exactly what I meant

> 
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1460,9 +1460,10 @@ static int do_run(int argc, char **argv)
>         int err;
>  
>         link = bpf_program__attach(prog);
> -       if (!link)
> -               return -1;
> -
> +       if (!link) {
> +               p_info("Program %s attach failed",
> bpf_program__name(prog));
> +               return bpf_obj_pin(bpf_program__fd(prog), path);
> +       }
>         err = bpf_link__pin(link, path);
>         if (err) {
>                 bpf_link__destroy(link);
> @@ -1499,9 +1500,6 @@ static int pathname_concat(const char *path, const
> char *name, char *buf)
>                 err = auto_attach_program(prog, buf);
>                 if (!err)
>                         continue;
> -               if (errno == EOPNOTSUPP)
> -                       p_info("Program %s does not support autoattach",

p_info("Program %s does not support autoattach, falling back to pinning"

> -                              bpf_program__name(prog));
>                 else
>                         goto err_unpin_programs;
>         }
> 
> 
> and the doc is modified as follows:
> 
> If the program does not support autoattach, will do regular pin along
> with an
> info message such as "Program %s attach failed". If the *OBJ* contains
> multiple
> programs and **loadall** is used, if the program A in these programs
> does not
> support autoattach, the program A will do regular pin along with an info
> message,
> and continue to autoattach the next program.

Not sure the "program A" designation helps too much, I'd simply write this:

"If a program does not support autoattach, bpftool falls back to regular
pinning for that program instead."

Which should be enough for both the "load" and "loadall" behaviours? I
wouldn't mention the help message in the docs (the p_info() won't show
up in the JSON output for example).

Looks good otherwise, thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall
  2022-10-10  8:40     ` Quentin Monnet
@ 2022-10-10  8:58       ` wangyufen
  0 siblings, 0 replies; 10+ messages in thread
From: wangyufen @ 2022-10-10  8:58 UTC (permalink / raw)
  To: Quentin Monnet, ast, daniel, andrii, martin.lau, song, yhs,
	john.fastabend, kpsingh, sdf, haoluo, jolsa, davem, kuba, hawk,
	nathan, ndesaulniers, trix
  Cc: bpf, linux-kernel, netdev, llvm


在 2022/10/10 16:40, Quentin Monnet 写道:
> Sat Oct 08 2022 06:16:42 GMT+0100 ~ wangyufen <wangyufen@huawei.com>
>> 在 2022/10/1 0:26, Quentin Monnet 写道:
>>> Tue Sep 27 2022 12:21:14 GMT+0100 ~ Wang Yufen <wangyufen@huawei.com>
>>>> Add auto_attach optional to support one-step load-attach-pin_link.
>>> Nit: Now "autoattach" instead of "auto_attach". Same in commit title.
>> will change in v8, thanks.
>>>> For example,
>>>>      $ bpftool prog loadall test.o /sys/fs/bpf/test autoattach
>>>>
>>>>      $ bpftool link
>>>>      26: tracing  name test1  tag f0da7d0058c00236  gpl
>>>>          loaded_at 2022-09-09T21:39:49+0800  uid 0
>>>>          xlated 88B  jited 55B  memlock 4096B  map_ids 3
>>>>          btf_id 55
>>>>      28: kprobe  name test3  tag 002ef1bef0723833  gpl
>>>>          loaded_at 2022-09-09T21:39:49+0800  uid 0
>>>>          xlated 88B  jited 56B  memlock 4096B  map_ids 3
>>>>          btf_id 55
>>>>      57: tracepoint  name oncpu  tag 7aa55dfbdcb78941  gpl
>>>>          loaded_at 2022-09-09T21:41:32+0800  uid 0
>>>>          xlated 456B  jited 265B  memlock 4096B  map_ids 17,13,14,15
>>>>          btf_id 82
>>>>
>>>>      $ bpftool link
>>>>      1: tracing  prog 26
>>>>          prog_type tracing  attach_type trace_fentry
>>>>      3: perf_event  prog 28
>>>>      10: perf_event  prog 57
>>>>
>>>> The autoattach optional can support tracepoints, k(ret)probes,
>>>> u(ret)probes.
>>>>
>>>> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
>>>> Signed-off-by: Wang Yufen <wangyufen@huawei.com>
>>>> ---
>>>> v6 -> v7: add info msg print and update doc for the skip program
>>>> v5 -> v6: skip the programs not supporting auto-attach,
>>>>        and change optional name from "auto_attach" to "autoattach"
>>>> v4 -> v5: some formatting nits of doc
>>>> v3 -> v4: rename functions, update doc, bash and do_help()
>>>> v2 -> v3: switch to extend prog load command instead of extend perf
>>>> v2:
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/20220824033837.458197-1-weiyongjun1@huawei.com/
>>>> v1:
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/20220816151725.153343-1-weiyongjun1@huawei.com/
>>>>    tools/bpf/bpftool/prog.c | 81
>>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 79 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>>>> index c81362a..84eced8 100644
>>>> --- a/tools/bpf/bpftool/prog.c
>>>> +++ b/tools/bpf/bpftool/prog.c
>>>> @@ -1453,6 +1453,72 @@ static int do_run(int argc, char **argv)
>>>>        return ret;
>>>>    }
>>>>    +static int
>>>> +auto_attach_program(struct bpf_program *prog, const char *path)
>>>> +{
>>>> +    struct bpf_link *link;
>>>> +    int err;
>>>> +
>>>> +    link = bpf_program__attach(prog);
>>>> +    if (!link)
>>>> +        return -1;
>>>> +
>>>> +    err = bpf_link__pin(link, path);
>>>> +    if (err) {
>>>> +        bpf_link__destroy(link);
>>>> +        return err;
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int pathname_concat(const char *path, const char *name, char
>>>> *buf)
>>>> +{
>>>> +    int len;
>>>> +
>>>> +    len = snprintf(buf, PATH_MAX, "%s/%s", path, name);
>>>> +    if (len < 0)
>>>> +        return -EINVAL;
>>>> +    if (len >= PATH_MAX)
>>>> +        return -ENAMETOOLONG;
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int
>>>> +auto_attach_programs(struct bpf_object *obj, const char *path)
>>>> +{
>>>> +    struct bpf_program *prog;
>>>> +    char buf[PATH_MAX];
>>>> +    int err;
>>>> +
>>>> +    bpf_object__for_each_program(prog, obj) {
>>>> +        err = pathname_concat(path, bpf_program__name(prog), buf);
>>>> +        if (err)
>>>> +            goto err_unpin_programs;
>>>> +
>>>> +        err = auto_attach_program(prog, buf);
>>>> +        if (!err)
>>>> +            continue;
>>>> +        if (errno == EOPNOTSUPP)
>>>> +            p_info("Program %s does not support autoattach",
>>>> +                   bpf_program__name(prog));
>>>> +        else
>>>> +            goto err_unpin_programs
>>> With this code, if auto-attach fails, then we skip this program and move
>>> on to the next. That's an improvement, but in that case the program
>>> won't remain loaded in the kernel after bpftool exits. My suggestion in
>>> my previous message (sorry if it was not clear) was to fall back to
>>> regular pinning in that case (bpf_obj_pin()), along with the p_info()
>>> message, so we can have the program pinned but not attached and let the
>>> user know. If regular pinning fails as well, then we should unpin all
>>> and error out, for consistency with bpf_object__pin_programs().
>>>
>>> And in that case, the (errno == EOPNOTSUPP) with fallback to regular
>>> pinning could maybe be moved into auto_attach_program(), so that
>>> auto-attaching single programs can use the fallback too?
>>>
>>> Thanks,
>>> Quentin
>> If I understand correctly, can we just check link?  as following:
> Yes, this is exactly what I meant
>
>> --- a/tools/bpf/bpftool/prog.c
>> +++ b/tools/bpf/bpftool/prog.c
>> @@ -1460,9 +1460,10 @@ static int do_run(int argc, char **argv)
>>          int err;
>>   
>>          link = bpf_program__attach(prog);
>> -       if (!link)
>> -               return -1;
>> -
>> +       if (!link) {
>> +               p_info("Program %s attach failed",
>> bpf_program__name(prog));
>> +               return bpf_obj_pin(bpf_program__fd(prog), path);
>> +       }
>>          err = bpf_link__pin(link, path);
>>          if (err) {
>>                  bpf_link__destroy(link);
>> @@ -1499,9 +1500,6 @@ static int pathname_concat(const char *path, const
>> char *name, char *buf)
>>                  err = auto_attach_program(prog, buf);
>>                  if (!err)
>>                          continue;
>> -               if (errno == EOPNOTSUPP)
>> -                       p_info("Program %s does not support autoattach",
> p_info("Program %s does not support autoattach, falling back to pinning"
>
>> -                              bpf_program__name(prog));
>>                  else
>>                          goto err_unpin_programs;
>>          }
>>
>>
>> and the doc is modified as follows:
>>
>> If the program does not support autoattach, will do regular pin along
>> with an
>> info message such as "Program %s attach failed". If the *OBJ* contains
>> multiple
>> programs and **loadall** is used, if the program A in these programs
>> does not
>> support autoattach, the program A will do regular pin along with an info
>> message,
>> and continue to autoattach the next program.
> Not sure the "program A" designation helps too much, I'd simply write this:
>
> "If a program does not support autoattach, bpftool falls back to regular
> pinning for that program instead."
>
> Which should be enough for both the "load" and "loadall" behaviours? I
> wouldn't mention the help message in the docs (the p_info() won't show
> up in the JSON output for example).

I got it. Thanks!

>
> Looks good otherwise, thanks!

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-10-10  8:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-27 11:21 [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Wang Yufen
2022-09-27 11:21 ` [bpf-next v7 2/3] bpftool: Update doc (add autoattach to prog load) Wang Yufen
2022-09-30 16:27   ` Quentin Monnet
2022-09-27 11:21 ` [bpf-next v7 3/3] bpftool: Update the bash completion(add " Wang Yufen
2022-09-30 16:26 ` [bpf-next v7 1/3] bpftool: Add auto_attach for bpf prog load|loadall Quentin Monnet
2022-10-08  5:16   ` wangyufen
2022-10-10  8:40     ` Quentin Monnet
2022-10-10  8:58       ` wangyufen
2022-09-30 20:55 ` Andrii Nakryiko
2022-10-08  5:22   ` wangyufen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.