Date:   Thu, 22 Dec 2022 20:06:48 -0800
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Martin KaFai Lau <martin.lau@...ux.dev>
Cc:     ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
        song@...nel.org, yhs@...com, john.fastabend@...il.com,
        kpsingh@...nel.org, haoluo@...gle.com, jolsa@...nel.org,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        netdev@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH bpf-next v5 11/17] selftests/bpf: Verify xdp_metadata
 xdp->af_xdp path

On Thu, Dec 22, 2022 at 4:40 PM Martin KaFai Lau <martin.lau@...ux.dev> wrote:
>
> On 12/20/22 2:20 PM, Stanislav Fomichev wrote:
> > +static int open_xsk(const char *ifname, struct xsk *xsk)
> > +{
> > +     int mmap_flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE;
> > +     const struct xsk_socket_config socket_config = {
> > +             .rx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > +             .tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > +             .libbpf_flags = XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD,
> > +             .xdp_flags = XDP_FLAGS,
> > +             .bind_flags = XDP_COPY,
> > +     };
> > +     const struct xsk_umem_config umem_config = {
> > +             .fill_size = XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > +             .comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS,
> > +             .frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE,
> > +             .flags = XDP_UMEM_UNALIGNED_CHUNK_FLAG,
> > +     };
> > +     __u32 idx;
> > +     u64 addr;
> > +     int ret;
> > +     int i;
> > +
> > +     xsk->umem_area = mmap(NULL, UMEM_SIZE, PROT_READ | PROT_WRITE, mmap_flags, -1, 0);
> > +     if (!ASSERT_NEQ(xsk->umem_area, MAP_FAILED, "mmap"))
> > +             return -1;
> > +
> > +     ret = xsk_umem__create(&xsk->umem,
> > +                            xsk->umem_area, UMEM_SIZE,
> > +                            &xsk->fill,
> > +                            &xsk->comp,
> > +                            &umem_config);
> > +     if (!ASSERT_OK(ret, "xsk_umem__create"))
> > +             return ret;
> > +
> > +     ret = xsk_socket__create(&xsk->socket, ifname, QUEUE_ID,
> > +                              xsk->umem,
> > +                              &xsk->rx,
> > +                              &xsk->tx,
> > +                              &socket_config);
> > +     if (!ASSERT_OK(ret, "xsk_socket__create"))
> > +             return ret;
> > +
> > +     /* First half of umem is for TX. This way address matches 1-to-1
> > +      * to the completion queue index.
> > +      */
> > +
> > +     for (i = 0; i < UMEM_NUM / 2; i++) {
> > +             addr = i * UMEM_FRAME_SIZE;
> > +             printf("%p: tx_desc[%d] -> %lx\n", xsk, i, addr);
>
> Do you still need this verbose printf which is in a loop?  Also, how about other
> printf in this test?

In case we'd ever need to debug this test, those printfs shouldn't
hurt, right? Or are you concerned about this test polluting the output
with something like 'test_progs -v -v' ?

> > +     }
> > +
> > +     /* Second half of umem is for RX. */
> > +
> > +     ret = xsk_ring_prod__reserve(&xsk->fill, UMEM_NUM / 2, &idx);
> > +     if (!ASSERT_EQ(UMEM_NUM / 2, ret, "xsk_ring_prod__reserve"))
> > +             return ret;
> > +     if (!ASSERT_EQ(idx, 0, "fill idx != 0"))
> > +             return -1;
> > +
> > +     for (i = 0; i < UMEM_NUM / 2; i++) {
> > +             addr = (UMEM_NUM / 2 + i) * UMEM_FRAME_SIZE;
> > +             printf("%p: rx_desc[%d] -> %lx\n", xsk, i, addr);
> > +             *xsk_ring_prod__fill_addr(&xsk->fill, i) = addr;
> > +     }
> > +     xsk_ring_prod__submit(&xsk->fill, ret);
> > +
> > +     return 0;
> > +}
> > +
>
> [ ... ]
>
> > +void test_xdp_metadata(void)
> > +{
> > +     struct xdp_metadata2 *bpf_obj2 = NULL;
> > +     struct xdp_metadata *bpf_obj = NULL;
> > +     struct bpf_program *new_prog, *prog;
> > +     struct nstoken *tok = NULL;
> > +     __u32 queue_id = QUEUE_ID;
> > +     struct bpf_map *prog_arr;
> > +     struct xsk tx_xsk = {};
> > +     struct xsk rx_xsk = {};
> > +     __u32 val, key = 0;
> > +     int retries = 10;
> > +     int rx_ifindex;
> > +     int sock_fd;
> > +     int ret;
> > +
> > +     /* Setup new networking namespace, with a veth pair. */
> > +
> > +     SYS("ip netns add xdp_metadata");
> > +     tok = open_netns("xdp_metadata");
> > +     SYS("ip link add numtxqueues 1 numrxqueues 1 " TX_NAME
> > +         " type veth peer " RX_NAME " numtxqueues 1 numrxqueues 1");
> > +     SYS("ip link set dev " TX_NAME " address 00:00:00:00:00:01");
> > +     SYS("ip link set dev " RX_NAME " address 00:00:00:00:00:02");
> > +     SYS("ip link set dev " TX_NAME " up");
> > +     SYS("ip link set dev " RX_NAME " up");
> > +     SYS("ip addr add " TX_ADDR "/" PREFIX_LEN " dev " TX_NAME);
> > +     SYS("ip addr add " RX_ADDR "/" PREFIX_LEN " dev " RX_NAME);
> > +
> > +     rx_ifindex = if_nametoindex(RX_NAME);
> > +
> > +     /* Setup separate AF_XDP for TX and RX interfaces. */
> > +
> > +     ret = open_xsk(TX_NAME, &tx_xsk);
> > +     if (!ASSERT_OK(ret, "open_xsk(TX_NAME)"))
> > +             goto out;
> > +
> > +     ret = open_xsk(RX_NAME, &rx_xsk);
> > +     if (!ASSERT_OK(ret, "open_xsk(RX_NAME)"))
> > +             goto out;
> > +
> > +     bpf_obj = xdp_metadata__open();
> > +     if (!ASSERT_OK_PTR(bpf_obj, "open skeleton"))
> > +             goto out;
> > +
> > +     prog = bpf_object__find_program_by_name(bpf_obj->obj, "rx");
> > +     bpf_program__set_ifindex(prog, rx_ifindex);
> > +     bpf_program__set_flags(prog, BPF_F_XDP_DEV_BOUND_ONLY);
> > +
> > +     if (!ASSERT_OK(xdp_metadata__load(bpf_obj), "load skeleton"))
> > +             goto out;
> > +
> > +     /* Make sure we can't add dev-bound programs to prog maps. */
> > +     prog_arr = bpf_object__find_map_by_name(bpf_obj->obj, "prog_arr");
> > +     if (!ASSERT_OK_PTR(prog_arr, "no prog_arr map"))
> > +             goto out;
> > +
> > +     val = bpf_program__fd(prog);
> > +     if (!ASSERT_ERR(bpf_map__update_elem(prog_arr, &key, sizeof(key),
> > +                                          &val, sizeof(val), BPF_ANY),
> > +                     "update prog_arr"))
> > +             goto out;
> > +
> > +     /* Attach BPF program to RX interface. */
> > +
> > +     ret = bpf_xdp_attach(rx_ifindex,
> > +                          bpf_program__fd(bpf_obj->progs.rx),
> > +                          XDP_FLAGS, NULL);
> > +     if (!ASSERT_GE(ret, 0, "bpf_xdp_attach"))
> > +             goto out;
> > +
> > +     sock_fd = xsk_socket__fd(rx_xsk.socket);
> > +     ret = bpf_map_update_elem(bpf_map__fd(bpf_obj->maps.xsk), &queue_id, &sock_fd, 0);
> > +     if (!ASSERT_GE(ret, 0, "bpf_map_update_elem"))
> > +             goto out;
> > +
> > +     /* Send packet destined to RX AF_XDP socket. */
> > +     if (!ASSERT_GE(generate_packet(&tx_xsk, AF_XDP_CONSUMER_PORT), 0,
> > +                    "generate AF_XDP_CONSUMER_PORT"))
> > +             goto out;
> > +
> > +     /* Verify AF_XDP RX packet has proper metadata. */
> > +     if (!ASSERT_GE(verify_xsk_metadata(&rx_xsk), 0,
> > +                    "verify_xsk_metadata"))
> > +             goto out;
> > +
> > +     complete_tx(&tx_xsk);
> > +
> > +     /* Make sure freplace correctly picks up original bound device
> > +      * and doesn't crash.
> > +      */
> > +
> > +     bpf_obj2 = xdp_metadata2__open();
> > +     if (!ASSERT_OK_PTR(bpf_obj2, "open skeleton"))
> > +             goto out;
> > +
> > +     new_prog = bpf_object__find_program_by_name(bpf_obj2->obj, "freplace_rx");
> > +     bpf_program__set_attach_target(new_prog, bpf_program__fd(prog), "rx");
> > +
> > +     if (!ASSERT_OK(xdp_metadata2__load(bpf_obj2), "load freplace skeleton"))
> > +             goto out;
> > +
> > +     if (!ASSERT_OK(xdp_metadata2__attach(bpf_obj2), "attach freplace"))
> > +             goto out;
> > +
> > +     /* Send packet to trigger . */
> > +     if (!ASSERT_GE(generate_packet(&tx_xsk, AF_XDP_CONSUMER_PORT), 0,
> > +                    "generate freplace packet"))
> > +             goto out;
> > +
> > +     while (!retries--) {
> > +             if (bpf_obj2->bss->called)
> > +                     break;
> > +             usleep(10);
> > +     }
> > +     ASSERT_GT(bpf_obj2->bss->called, 0, "not called");
> > +
> > +out:
> > +     close_xsk(&rx_xsk);
> > +     close_xsk(&tx_xsk);
> > +     if (bpf_obj2)
>
> nit. no need to test NULL.  xdp_metadata2__destroy() can handle it.

SG, thanks!

> > +             xdp_metadata2__destroy(bpf_obj2);
> > +     if (bpf_obj)
>
> Same here.
>
> > +             xdp_metadata__destroy(bpf_obj);
> > +     system("ip netns del xdp_metadata");
>
> didn't know netns can be deleted before close_netns(tok).  Can you double check?

Should probably work because 'ip netns del' just unlinks the netns fd?
But let's properly reoder.


> > +     if (tok)
> > +             close_netns(tok);
> > +}
>
>

Powered by blists - more mailing lists