RE: [PATCH bpf-next 3/8] selftests/bpf: Use bpf_link attachments in test_sockmap

[John Fastabend <john.fastabend@gmail.com>]

Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Switch attachments to bpf_link using bpf_program__attach_sockmap() instead
> of bpf_prog_attach().

Sorry it took me a few days to get to this.

Is there a reason to push this to links vs just leave it as is? I had
a plan to port all the test_sockmap tests into prog_tests anyways. I'll
try to push some initial patch next week.

The one advantage of test_sockmap is we can have it run for longer
runs by pushing different options through so might be worth keeping
just for that.

If you really want links here I'm OK with that I guess just asking.

Thanks,
John

> 
> This patch adds a new array progs[] to replace prog_fd[] array, set in
> populate_progs() for each program in bpf object.
> 
> And another new array links[] to save the attached bpf_link. It is
> initalized as NULL in populate_progs, set as the return valuses of
> bpf_program__attach_sockmap(), and detached by bpf_link__detach().
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/test_sockmap.c | 59 ++++++++++++----------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_sockmap.c b/tools/testing/selftests/bpf/test_sockmap.c
> index e7dbf49a2ca6..d7581bbbc473 100644
> --- a/tools/testing/selftests/bpf/test_sockmap.c
> +++ b/tools/testing/selftests/bpf/test_sockmap.c
> @@ -64,6 +64,8 @@ int failed;
>  int map_fd[9];
>  struct bpf_map *maps[9];
>  int prog_fd[9];
> +struct bpf_program *progs[9];
> +struct bpf_link *links[9];
>  
>  int txmsg_pass;
>  int txmsg_redir;
> @@ -960,43 +962,39 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
>  
>  	/* Attach programs to sockmap */
>  	if (!txmsg_omit_skb_parser) {
> -		err = bpf_prog_attach(prog_fd[0], map_fd[0],
> -				      BPF_SK_SKB_STREAM_PARSER, 0);
> -		if (err) {
> +		links[0] = bpf_program__attach_sockmap(progs[0], map_fd[0]);
> +		if (!links[0]) {
>  			fprintf(stderr,
> -				"ERROR: bpf_prog_attach (sockmap %i->%i): %d (%s)\n",
> -				prog_fd[0], map_fd[0], err, strerror(errno));
> -			return err;
> +				"ERROR: bpf_program__attach_sockmap (sockmap %i->%i): (%s)\n",
> +				bpf_program__fd(progs[0]), map_fd[0], strerror(errno));
> +			return -1;
>  		}
>  	}
>  
> -	err = bpf_prog_attach(prog_fd[1], map_fd[0],
> -				BPF_SK_SKB_STREAM_VERDICT, 0);
> -	if (err) {
> -		fprintf(stderr, "ERROR: bpf_prog_attach (sockmap): %d (%s)\n",
> -			err, strerror(errno));
> -		return err;
> +	links[1] = bpf_program__attach_sockmap(progs[1], map_fd[0]);
> +	if (!links[1]) {
> +		fprintf(stderr, "ERROR: bpf_program__attach_sockmap (sockmap): (%s)\n",
> +			strerror(errno));
> +		return -1;
>  	}
>  
>  	/* Attach programs to TLS sockmap */
>  	if (txmsg_ktls_skb) {
>  		if (!txmsg_omit_skb_parser) {
> -			err = bpf_prog_attach(prog_fd[0], map_fd[8],
> -					      BPF_SK_SKB_STREAM_PARSER, 0);
> -			if (err) {
> +			links[2] = bpf_program__attach_sockmap(progs[0], map_fd[8]);
> +			if (!links[2]) {
>  				fprintf(stderr,
> -					"ERROR: bpf_prog_attach (TLS sockmap %i->%i): %d (%s)\n",
> -					prog_fd[0], map_fd[8], err, strerror(errno));
> -				return err;
> +					"ERROR: bpf_program__attach_sockmap (TLS sockmap %i->%i): (%s)\n",
> +					bpf_program__fd(progs[0]), map_fd[8], strerror(errno));
> +				return -1;
>  			}
>  		}
>  
> -		err = bpf_prog_attach(prog_fd[2], map_fd[8],
> -				      BPF_SK_SKB_STREAM_VERDICT, 0);
> -		if (err) {
> -			fprintf(stderr, "ERROR: bpf_prog_attach (TLS sockmap): %d (%s)\n",
> -				err, strerror(errno));
> -			return err;
> +		links[3] = bpf_program__attach_sockmap(progs[2], map_fd[8]);
> +		if (!links[3]) {
> +			fprintf(stderr, "ERROR: bpf_program__attach_sockmap (TLS sockmap): (%s)\n",
> +				strerror(errno));
> +			return -1;
>  		}
>  	}
>  
> @@ -1281,10 +1279,11 @@ static int run_options(struct sockmap_options *options, int cg_fd,  int test)
>  out:
>  	/* Detatch and zero all the maps */
>  	bpf_prog_detach2(prog_fd[3], cg_fd, BPF_CGROUP_SOCK_OPS);
> -	bpf_prog_detach2(prog_fd[0], map_fd[0], BPF_SK_SKB_STREAM_PARSER);
> -	bpf_prog_detach2(prog_fd[1], map_fd[0], BPF_SK_SKB_STREAM_VERDICT);
> -	bpf_prog_detach2(prog_fd[0], map_fd[8], BPF_SK_SKB_STREAM_PARSER);
> -	bpf_prog_detach2(prog_fd[2], map_fd[8], BPF_SK_SKB_STREAM_VERDICT);
> +
> +	for (i = 0; i < ARRAY_SIZE(links); i++) {
> +		if (links[i])
> +			bpf_link__detach(links[i]);
> +	}
>  
>  	if (tx_prog_fd > 0)
>  		bpf_prog_detach2(tx_prog_fd, map_fd[1], BPF_SK_MSG_VERDICT);
> @@ -1836,6 +1835,7 @@ static int populate_progs(char *bpf_file)
>  	i = bpf_object__load(obj);
>  	i = 0;
>  	bpf_object__for_each_program(prog, obj) {
> +		progs[i] = prog;
>  		prog_fd[i] = bpf_program__fd(prog);
>  		i++;
>  	}
> @@ -1850,6 +1850,9 @@ static int populate_progs(char *bpf_file)
>  		}
>  	}
>  
> +	for (i = 0; i < ARRAY_SIZE(links); i++)
> +		links[i] = NULL;
> +
>  	return 0;
>  }
>  
> -- 
> 2.43.0
>