Re: [PATCH bpf-next 3/4] selftests/bpf: add proper section name to bpf prog and rename it

[Alan Maguire <alan.maguire@oracle.com>]

On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> test_skb_cgroup_id_kern.c is currently involved in a manual test. In its
> current form, it can not be used with the auto-generated skeleton APIs,
> because the section name is not valid to allow libbpf to deduce the program
> type.
> 
> Update section name to allow skeleton APIs usage. Also rename the program
> name to make it shorter and more straighforward regarding the API it is
> testing. While doing so, make sure that test_skb_cgroup_id.sh passes to get
> a working reference before converting it to test_progs
> - update the obj name
> - fix loading issue (verifier rejecting the program when loaded through tc,
>   because of map not found), by preloading the whole obj with bpftool
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

> ---
>  .../progs/{test_skb_cgroup_id_kern.c => cgroup_ancestor.c}   |  2 +-
>  tools/testing/selftests/bpf/test_skb_cgroup_id.sh            | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c
> similarity index 97%
> rename from tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
> rename to tools/testing/selftests/bpf/progs/cgroup_ancestor.c
> index 37aacc66cd68..4879645f5827 100644
> --- a/tools/testing/selftests/bpf/progs/test_skb_cgroup_id_kern.c
> +++ b/tools/testing/selftests/bpf/progs/cgroup_ancestor.c
> @@ -28,7 +28,7 @@ static __always_inline void log_nth_level(struct __sk_buff *skb, __u32 level)
>  	bpf_map_update_elem(&cgroup_ids, &level, &id, 0);
>  }
>  
> -SEC("cgroup_id_logger")
> +SEC("tc")
>  int log_cgroup_id(struct __sk_buff *skb)
>  {
>  	/* Loop unroll can't be used here due to [1]. Unrolling manually.
> diff --git a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> index 515c2eafc97f..d7dad49175c2 100755
> --- a/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> +++ b/tools/testing/selftests/bpf/test_skb_cgroup_id.sh
> @@ -30,8 +30,10 @@ setup()
>  	wait_for_ip
>  
>  	tc qdisc add dev ${TEST_IF} clsact
> -	tc filter add dev ${TEST_IF} egress bpf obj ${BPF_PROG_OBJ} \
> -		sec ${BPF_PROG_SECTION} da
> +	mkdir -p /sys/fs/bpf/${BPF_PROG_PIN}
> +	bpftool prog loadall ${BPF_PROG_OBJ} /sys/fs/bpf/${BPF_PROG_PIN} type tc
> +	tc filter add dev ${TEST_IF} egress bpf da object-pinned \
> +		/sys/fs/bpf/${BPF_PROG_PIN}/${BPF_PROG_NAME}
>

Just out of curiosity; did you find that the pin is needed? I would have
thought tc attach would be enough to keep the program aroud.

>  	BPF_PROG_ID=$(tc filter show dev ${TEST_IF} egress | \
>  			awk '/ id / {sub(/.* id /, "", $0); print($1)}')
> @@ -41,6 +43,7 @@ cleanup()
>  {
>  	ip link del ${TEST_IF} 2>/dev/null || :
>  	ip link del ${TEST_IF_PEER} 2>/dev/null || :
> +	rm -rf /sys/fs/bpf/${BPF_PROG_PIN}
>  }
>  
>  main()
> @@ -54,8 +57,9 @@ DIR=$(dirname $0)
>  TEST_IF="test_cgid_1"
>  TEST_IF_PEER="test_cgid_2"
>  MAX_PING_TRIES=5
> -BPF_PROG_OBJ="${DIR}/test_skb_cgroup_id_kern.bpf.o"
> -BPF_PROG_SECTION="cgroup_id_logger"
> +BPF_PROG_PIN="cgroup_ancestor"
> +BPF_PROG_OBJ="${DIR}/${BPF_PROG_PIN}.bpf.o"
> +BPF_PROG_NAME="log_cgroup_id"
>  BPF_PROG_ID=0
>  PROG="${DIR}/test_skb_cgroup_id_user"
>  type ping6 >/dev/null 2>&1 && PING6="ping6" || PING6="ping -6"
>