Message-ID: <CAMB2axO2Jkc4Ec051+BYhju2Vr_GwzZL6yhHGuohMdg2q6WLRQ@mail.gmail.com>
Date: Fri, 2 May 2025 10:52:23 -0700
From: Amery Hung <ameryhung@...il.com>
To: Martin KaFai Lau <martin.lau@...ux.dev>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, alexei.starovoitov@...il.com, 
	andrii@...nel.org, daniel@...earbox.net, martin.lau@...nel.org, 
	xiyou.wangcong@...il.com, kernel-team@...a.com
Subject: Re: [PATCH bpf-next/net v1 2/5] selftests/bpf: Test setting and
 creating bpf qdisc as default qdisc

On Thu, May 1, 2025 at 4:58 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> 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@...il.com>
> > ---
> >   .../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.
>

I see. I assume it will also be okay to set autoattach and call
bpf_qdisc_fifo__attach()?

> > +     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] = {};..."
>

I will simplify the cleanup as you suggested.

> > +
> > +     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?
>

That simplifies things quite a bit. I will use a global variable to
check if the qdisc is being created.

> > +
> > +     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();
> > +}
>

Powered by blists - more mailing lists