Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and creating bpf qdisc as default qdisc

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 5/1/25 3:30 PM, Amery Hung wrote:
> First, test that bpf qdisc can be set as default qdisc. Then, attach
> an mq qdisc to see if bpf qdisc can be successfully created and grafted.
>
> The test is a sequential test as net.core.default_qdisc is global.
>
> Signed-off-by: Amery Hung <ameryhung@xxxxxxxxx>
> ---
>   .../selftests/bpf/prog_tests/bpf_qdisc.c      | 78 +++++++++++++++++++
>   1 file changed, 78 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> index c9a54177c84e..c954cc2ae64f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_qdisc.c
> @@ -159,6 +159,79 @@ static void test_qdisc_attach_to_non_root(void)
>   	bpf_qdisc_fifo__destroy(fifo_skel);
>   }
>   
> +static int get_default_qdisc(char *qdisc_name)
> +{
> +	FILE *f;
> +	int num;
> +
> +	f = fopen("/proc/sys/net/core/default_qdisc", "r");
> +	if (!f)
> +		return -errno;
> +
> +	num = fscanf(f, "%s", qdisc_name);
> +	fclose(f);
> +
> +	return num == 1 ? 0 : -EFAULT;
> +}
> +
> +static void test_default_qdisc_attach_to_mq(void)
> +{
> +	struct bpf_qdisc_fifo *fifo_skel;
> +	char default_qdisc[IFNAMSIZ];
> +	struct netns_obj *netns;
> +	char tc_qdisc_show[64];
> +	struct bpf_link *link;
> +	char *str_ret;
> +	FILE *tc;
> +	int err;
> +
> +	fifo_skel = bpf_qdisc_fifo__open_and_load();
> +	if (!ASSERT_OK_PTR(fifo_skel, "bpf_qdisc_fifo__open_and_load"))
> +		return;
> +
> +	link = bpf_map__attach_struct_ops(fifo_skel->maps.fifo);

	fifo_skel->links.fifo = bpf_map__attach_struct_ops(....);

Then no need to bpf_link__destroy(link). bpf_qdisc_fifo__destroy() should do.

> +	if (!ASSERT_OK_PTR(link, "bpf_map__attach_struct_ops")) {
> +		bpf_qdisc_fifo__destroy(fifo_skel);
> +		return;
> +	}
> +
> +	err = get_default_qdisc(default_qdisc);
> +	if (!ASSERT_OK(err, "read sysctl net.core.default_qdisc"))
> +		goto out;
> +
> +	err = write_sysctl("/proc/sys/net/core/default_qdisc", "bpf_fifo");
> +	if (!ASSERT_OK(err, "write sysctl net.core.default_qdisc"))
> +		goto out;
> +
> +	netns = netns_new("bpf_qdisc_ns", true);
> +	if (!ASSERT_OK_PTR(netns, "netns_new"))
> +		goto out;

This should be 'goto out_restore_dflt_qdisc'.

I would stay with minimum number of cleanup labels if possible. Initialize the 
variables that need to be cleaned up instead. There is no need to optimize each 
cleanup case for a test,

e.g. "struct netns_obj netns = NULL; char default_qdisc[IFNAMSIZ] = {};..."

> +
> +	SYS(out_restore_dflt_qdisc, "ip link add veth0 type veth peer veth1");
> +	SYS(out_delete_netns, "tc qdisc add dev veth0 root handle 1: mq");
> +
> +	tc = popen("tc qdisc show dev veth0 parent 1:1", "r");
> +	if (!ASSERT_OK_PTR(tc, "tc qdisc show dev veth0 parent 1:1"))
> +		goto out_delete_netns;
> +
> +	str_ret = fgets(tc_qdisc_show, sizeof(tc_qdisc_show), tc);
> +	if (!ASSERT_OK_PTR(str_ret, "tc qdisc show dev veth0 parent 1:1"))
> +		goto out_delete_netns;
> +
> +	str_ret = strstr(tc_qdisc_show, "qdisc bpf_fifo");
> +	if (!ASSERT_OK_PTR(str_ret, "check if bpf_fifo is created"))
> +		goto out_delete_netns;

Instead of pipe and grep, how about having the bpf_fifo_init bpf prog to set a 
global variable when called and then check the "fifo_skel->bss->init_called == 
true" here?

> +
> +	SYS(out_delete_netns, "tc qdisc delete dev veth0 root mq");
> +out_delete_netns:
> +	netns_free(netns);
> +out_restore_dflt_qdisc:
> +	write_sysctl("/proc/sys/net/core/default_qdisc", default_qdisc);
> +out:
> +	bpf_link__destroy(link);
> +	bpf_qdisc_fifo__destroy(fifo_skel);
> +}
> +
>   void test_bpf_qdisc(void)
>   {
>   	struct netns_obj *netns;
> @@ -178,3 +251,8 @@ void test_bpf_qdisc(void)
>   
>   	netns_free(netns);
>   }
> +
> +void serial_test_bpf_qdisc_default(void)
> +{
> +	test_default_qdisc_attach_to_mq();
> +}