Date:   Mon, 30 Jan 2023 16:49:52 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Joanne Koong <joannelkoong@...il.com>
Cc:     bpf@...r.kernel.org, daniel@...earbox.net, andrii@...nel.org,
        martin.lau@...nel.org, ast@...nel.org, netdev@...r.kernel.org,
        memxor@...il.com, kernel-team@...com
Subject: Re: [PATCH v9 bpf-next 5/5] selftests/bpf: tests for using dynptrs to
 parse skb and xdp buffers

On Fri, Jan 27, 2023 at 11:18 AM Joanne Koong <joannelkoong@...il.com> wrote:
>
> Test skb and xdp dynptr functionality in the following ways:
>
> 1) progs/test_cls_redirect_dynptr.c
>    * Rewrite "progs/test_cls_redirect.c" test to use dynptrs to parse
>      skb data
>
>    * This is a great example of how dynptrs can be used to simplify a
>      lot of the parsing logic for non-statically known values.
>
>      When measuring the user + system time between the original version
>      vs. using dynptrs, and averaging the time for 10 runs (using
>      "time ./test_progs -t cls_redirect"):
>          original version: 0.092 sec
>          with dynptrs: 0.078 sec
>
> 2) progs/test_xdp_dynptr.c
>    * Rewrite "progs/test_xdp.c" test to use dynptrs to parse xdp data
>
>      When measuring the user + system time between the original version
>      vs. using dynptrs, and averaging the time for 10 runs (using
>      "time ./test_progs -t xdp_attach"):
>          original version: 0.118 sec
>          with dynptrs: 0.094 sec
>
> 3) progs/test_l4lb_noinline_dynptr.c
>    * Rewrite "progs/test_l4lb_noinline.c" test to use dynptrs to parse
>      skb data
>
>      When measuring the user + system time between the original version
>      vs. using dynptrs, and averaging the time for 10 runs (using
>      "time ./test_progs -t l4lb_all"):
>          original version: 0.062 sec
>          with dynptrs: 0.081 sec
>
>      For number of processed verifier instructions:
>          original version: 6268 insns
>          with dynptrs: 2588 insns
>
> 4) progs/test_parse_tcp_hdr_opt_dynptr.c
>    * Add sample code for parsing tcp hdr opt lookup using dynptrs.
>      This logic is lifted from a real-world use case of packet parsing
>      in katran [0], a layer 4 load balancer. The original version
>      "progs/test_parse_tcp_hdr_opt.c" (not using dynptrs) is included
>      here as well, for comparison.
>
>      When measuring the user + system time between the original version
>      vs. using dynptrs, and averaging the time for 10 runs (using
>      "time ./test_progs -t parse_tcp_hdr_opt"):
>          original version: 0.031 sec
>          with dynptrs: 0.045 sec
>
> 5) progs/dynptr_success.c
>    * Add test case "test_skb_readonly" for testing attempts at writes /
>      data slices on a prog type with read-only skb ctx.
>
> 6) progs/dynptr_fail.c
>    * Add test cases "skb_invalid_data_slice{1,2}" and
>      "xdp_invalid_data_slice" for testing that helpers that modify the
>      underlying packet buffer automatically invalidate the associated
>      data slice.
>    * Add test cases "skb_invalid_ctx" and "xdp_invalid_ctx" for testing
>      that prog types that do not support bpf_dynptr_from_skb/xdp don't
>      have access to the API.
>    * Add test case "skb_invalid_write" for testing that writes to a
>      read-only data slice are rejected by the verifier.
>
> [0]
> https://github.com/facebookincubator/katran/blob/main/katran/lib/bpf/pckt_parsing.h
>
> Signed-off-by: Joanne Koong <joannelkoong@...il.com>
> Acked-by: Andrii Nakryiko <andrii@...nel.org>
> ---
>  .../selftests/bpf/prog_tests/cls_redirect.c   |  25 +
>  .../testing/selftests/bpf/prog_tests/dynptr.c |  63 +-
>  .../selftests/bpf/prog_tests/l4lb_all.c       |   2 +
>  .../bpf/prog_tests/parse_tcp_hdr_opt.c        |  93 ++
>  .../selftests/bpf/prog_tests/xdp_attach.c     |  11 +-
>  .../testing/selftests/bpf/progs/dynptr_fail.c | 124 +++
>  .../selftests/bpf/progs/dynptr_success.c      |  28 +
>  .../bpf/progs/test_cls_redirect_dynptr.c      | 973 ++++++++++++++++++
>  .../bpf/progs/test_l4lb_noinline_dynptr.c     | 474 +++++++++
>  .../bpf/progs/test_parse_tcp_hdr_opt.c        | 119 +++
>  .../bpf/progs/test_parse_tcp_hdr_opt_dynptr.c | 112 ++
>  .../selftests/bpf/progs/test_xdp_dynptr.c     | 237 +++++
>  .../selftests/bpf/test_tcp_hdr_options.h      |   1 +
>  13 files changed, 2248 insertions(+), 14 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/parse_tcp_hdr_opt.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_cls_redirect_dynptr.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_l4lb_noinline_dynptr.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_parse_tcp_hdr_opt.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_parse_tcp_hdr_opt_dynptr.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_xdp_dynptr.c
>

[...]

> diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_attach.c b/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
> index 062fbc8c8e5e..28c453bbb84a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/xdp_attach.c
> @@ -4,11 +4,10 @@
>  #define IFINDEX_LO 1
>  #define XDP_FLAGS_REPLACE              (1U << 4)
>
> -void serial_test_xdp_attach(void)
> +static void serial_test_xdp_attach(const char *file)
>  {
>         __u32 duration = 0, id1, id2, id0 = 0, len;
>         struct bpf_object *obj1, *obj2, *obj3;
> -       const char *file = "./test_xdp.bpf.o";
>         struct bpf_prog_info info = {};
>         int err, fd1, fd2, fd3;
>         LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> @@ -85,3 +84,11 @@ void serial_test_xdp_attach(void)
>  out_1:
>         bpf_object__close(obj1);
>  }
> +
> +void test_xdp_attach(void)

this test was marked as serial (it starts with "serial_test_"), so we
probably want to preserve that? Just keep this as
"serial_test_xdp_attach" and rename above serial_test_xdp_attach to
something like "subtest_xdp_attach" (this name doesn't matter to
test_progs runner).

> +{
> +       if (test__start_subtest("xdp_attach"))
> +               serial_test_xdp_attach("./test_xdp.bpf.o");
> +       if (test__start_subtest("xdp_attach_dynptr"))
> +               serial_test_xdp_attach("./test_xdp_dynptr.bpf.o");
> +}

[...]

Powered by blists - more mailing lists