Message-ID: <CAM1=_QSTbC_V6BxrY+ObtkwnW8z_uW+1ckrmxMxA2gTo8HdUQw@mail.gmail.com>
Date:   Wed, 13 Oct 2021 09:52:53 +0200
From:   Johan Almbladh <johan.almbladh@...finetworks.com>
To:     Tiezhu Yang <yangtiezhu@...ngson.cn>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        linux-kernel@...r.kernel.org, Xuefeng Li <lixuefeng@...ngson.cn>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>
Subject: Re: [PATCH bpf-next v4] test_bpf: Add module parameter test_suite

On Sat, Oct 9, 2021 at 3:50 AM Tiezhu Yang <yangtiezhu@...ngson.cn> wrote:
>
> After commit 9298e63eafea ("bpf/tests: Add exhaustive tests of ALU
> operand magnitudes"), when modprobe test_bpf.ko with jit on mips64,
> there exists segment fault due to the following reason:
>
> ALU64_MOV_X: all register value magnitudes jited:1
> Break instruction in kernel code[#1]
>
> It seems that the related jit implementations of some test cases
> in test_bpf() have problems. At this moment, I do not care about
> the segment fault while I just want to verify the test cases of
> tail calls.
>
> Based on the above background and motivation, add the following
> module parameter test_suite to the test_bpf.ko:
> test_suite=<string>: only the specified test suite will be run, the
> string can be "test_bpf", "test_tail_calls" or "test_skb_segment".
>
> If test_suite is not specified, but test_id, test_name or test_range
> is specified, set 'test_bpf' as the default test suite.
>
> This is useful to only test the corresponding test suite when specify
> the valid test_suite string.
>
> Any invalid test suite will result in -EINVAL being returned and no
> tests being run. If the test_suite is not specified or specified as
> empty string, it does not change the current logic, all of the test
> cases will be run.
>
> Here are some test results:
>  # dmesg -c
>  # modprobe test_bpf
>  # dmesg
>  # dmesg | grep Summary
>  test_bpf: Summary: 1009 PASSED, 0 FAILED, [0/997 JIT'ed]
>  test_bpf: test_tail_calls: Summary: 8 PASSED, 0 FAILED, [0/8 JIT'ed]
>  test_bpf: test_skb_segment: Summary: 2 PASSED, 0 FAILED
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_suite=test_bpf
>  # dmesg | tail -1
>  test_bpf: Summary: 1009 PASSED, 0 FAILED, [0/997 JIT'ed]
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_suite=test_tail_calls
>  # dmesg
>  test_bpf: #0 Tail call leaf jited:0 21 PASS
>  [...]
>  test_bpf: #7 Tail call error path, index out of range jited:0 32 PASS
>  test_bpf: test_tail_calls: Summary: 8 PASSED, 0 FAILED, [0/8 JIT'ed]
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_suite=test_skb_segment
>  # dmesg
>  test_bpf: #0 gso_with_rx_frags PASS
>  test_bpf: #1 gso_linear_no_head_frag PASS
>  test_bpf: test_skb_segment: Summary: 2 PASSED, 0 FAILED
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_id=1
>  # dmesg
>  test_bpf: test_bpf: set 'test_bpf' as the default test_suite.
>  test_bpf: #1 TXA jited:0 54 51 50 PASS
>  test_bpf: Summary: 1 PASSED, 0 FAILED, [0/1 JIT'ed]
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_suite=test_bpf test_name=TXA
>  # dmesg
>  test_bpf: #1 TXA jited:0 54 50 51 PASS
>  test_bpf: Summary: 1 PASSED, 0 FAILED, [0/1 JIT'ed]
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_suite=test_tail_calls test_range=6,7
>  # dmesg
>  test_bpf: #6 Tail call error path, NULL target jited:0 41 PASS
>  test_bpf: #7 Tail call error path, index out of range jited:0 32 PASS
>  test_bpf: test_tail_calls: Summary: 2 PASSED, 0 FAILED, [0/2 JIT'ed]
>
>  # rmmod test_bpf
>  # dmesg -c
>  # modprobe test_bpf test_suite=test_skb_segment test_id=1
>  # dmesg
>  test_bpf: #1 gso_linear_no_head_frag PASS
>  test_bpf: test_skb_segment: Summary: 1 PASSED, 0 FAILED
>
> By the way, the above segment fault has been fixed in the latest bpf-next
> tree.
>
> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
> ---
>
> v4:
>   -- Fix the following checkpatch issues:
>      CHECK: Alignment should match open parenthesis
>      CHECK: Please don't use multiple blank lines
>
>      ./scripts/checkpatch.pl --strict *.patch
>      total: 0 errors, 0 warnings, 0 checks, 299 lines checked
>
>      the default max-line-length is 100 in ./scripts/checkpatch.pl,
>      but it seems that the netdev/checkpatch is 80:
>      https://patchwork.hopto.org/static/nipa/559961/12545157/checkpatch/stdout
>
> v3:
>   -- Use test_suite instead of test_type as module parameter
>   -- Make test_id, test_name and test_range selection applied to each test suite
>
> v2:
>   -- Fix typo in the commit message
>   -- Use my private email to send
>

Thanks! I would suggest a few edits to reduce the amount of code
duplication, see comments below. Then it will be less code and fewer
places to change if/when we add new test suites in the future.

>  lib/test_bpf.c | 262 ++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 186 insertions(+), 76 deletions(-)
>
> diff --git a/lib/test_bpf.c b/lib/test_bpf.c
> index e5b10fd..124204e 100644
> --- a/lib/test_bpf.c
> +++ b/lib/test_bpf.c
> @@ -14316,72 +14316,9 @@ module_param_string(test_name, test_name, sizeof(test_name), 0);
>  static int test_id = -1;
>  module_param(test_id, int, 0);
>
> -static int test_range[2] = { 0, ARRAY_SIZE(tests) - 1 };
> +static int test_range[2] = { -1, -1 };

test_range[] is a filter, so you can initialize it to {0, INT_MAX} to
cover all test cases by default. There is no need to trim it to the
actual range of each suite.

>  module_param_array(test_range, int, NULL, 0);
>
> -static __init int find_test_index(const char *test_name)
> -{
> -       int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(tests); i++) {
> -               if (!strcmp(tests[i].descr, test_name))
> -                       return i;
> -       }
> -       return -1;
> -}
> -
> -static __init int prepare_bpf_tests(void)
> -{
> -       if (test_id >= 0) {
> -               /*
> -                * if a test_id was specified, use test_range to
> -                * cover only that test.
> -                */
> -               if (test_id >= ARRAY_SIZE(tests)) {
> -                       pr_err("test_bpf: invalid test_id specified.\n");
> -                       return -EINVAL;
> -               }
> -
> -               test_range[0] = test_id;
> -               test_range[1] = test_id;
> -       } else if (*test_name) {
> -               /*
> -                * if a test_name was specified, find it and setup
> -                * test_range to cover only that test.
> -                */
> -               int idx = find_test_index(test_name);
> -
> -               if (idx < 0) {
> -                       pr_err("test_bpf: no test named '%s' found.\n",
> -                              test_name);
> -                       return -EINVAL;
> -               }
> -               test_range[0] = idx;
> -               test_range[1] = idx;
> -       } else {
> -               /*
> -                * check that the supplied test_range is valid.
> -                */
> -               if (test_range[0] >= ARRAY_SIZE(tests) ||
> -                   test_range[1] >= ARRAY_SIZE(tests) ||
> -                   test_range[0] < 0 || test_range[1] < 0) {
> -                       pr_err("test_bpf: test_range is out of bound.\n");
> -                       return -EINVAL;
> -               }
> -
> -               if (test_range[1] < test_range[0]) {
> -                       pr_err("test_bpf: test_range is ending before it starts.\n");
> -                       return -EINVAL;
> -               }
> -       }
> -
> -       return 0;
> -}
> -
> -static __init void destroy_bpf_tests(void)
> -{
> -}
> -
>  static bool exclude_test(int test_id)
>  {
>         return test_id < test_range[0] || test_id > test_range[1];
> @@ -14553,6 +14490,10 @@ static __init int test_skb_segment(void)
>         for (i = 0; i < ARRAY_SIZE(skb_segment_tests); i++) {
>                 const struct skb_segment_test *test = &skb_segment_tests[i];
>
> +               cond_resched();
> +               if (exclude_test(i))
> +                       continue;
> +
>                 pr_info("#%d %s ", i, test->descr);
>
>                 if (test_skb_segment_single(test)) {
> @@ -14934,6 +14875,8 @@ static __init int test_tail_calls(struct bpf_array *progs)
>                 int ret;
>
>                 cond_resched();
> +               if (exclude_test(i))
> +                       continue;
>
>                 pr_info("#%d %s ", i, test->descr);
>                 if (!fp) {
> @@ -14966,29 +14909,196 @@ static __init int test_tail_calls(struct bpf_array *progs)
>         return err_cnt ? -EINVAL : 0;
>  }
>
> +static char test_suite[32];
> +module_param_string(test_suite, test_suite, sizeof(test_suite), 0);
> +
> +static __init int find_test_index(const char *test_name)
> +{
> +       int i;
> +
> +       if (!strcmp(test_suite, "test_bpf")) {
> +               for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +                       if (!strcmp(tests[i].descr, test_name))
> +                               return i;
> +               }
> +       }
> +
> +       if (!strcmp(test_suite, "test_tail_calls")) {
> +               for (i = 0; i < ARRAY_SIZE(tail_call_tests); i++) {
> +                       if (!strcmp(tail_call_tests[i].descr, test_name))
> +                               return i;
> +               }
> +       }
> +
> +       if (!strcmp(test_suite, "test_skb_segment")) {
> +               for (i = 0; i < ARRAY_SIZE(skb_segment_tests); i++) {
> +                       if (!strcmp(skb_segment_tests[i].descr, test_name))
> +                               return i;
> +               }
> +       }
> +
> +       return -1;
> +}
> +
> +static __init int prepare_bpf_tests(void)
> +{
> +       if (test_id >= 0) {
> +               /*
> +                * if a test_id was specified, use test_range to
> +                * cover only that test.
> +                */
> +               if (!strcmp(test_suite, "test_bpf") &&
> +                   test_id >= ARRAY_SIZE(tests)) {
> +                       pr_err("test_bpf: invalid test_id specified for '%s' suite.\n",
> +                              test_suite);
> +                       return -EINVAL;
> +               }
> +
> +               if (!strcmp(test_suite, "test_tail_calls") &&
> +                   test_id >= ARRAY_SIZE(tail_call_tests)) {
> +                       pr_err("test_bpf: invalid test_id specified for '%s' suite.\n",
> +                              test_suite);
> +                       return -EINVAL;
> +               }
> +
> +               if (!strcmp(test_suite, "test_skb_segment") &&
> +                   test_id >= ARRAY_SIZE(skb_segment_tests)) {
> +                       pr_err("test_bpf: invalid test_id specified for '%s' suite.\n",
> +                              test_suite);
> +                       return -EINVAL;
> +               }
> +
> +               test_range[0] = test_id;
> +               test_range[1] = test_id;
> +       } else if (*test_name) {
> +               /*
> +                * if a test_name was specified, find it and setup
> +                * test_range to cover only that test.
> +                */
> +               int idx = find_test_index(test_name);
> +
> +               if (idx < 0) {
> +                       pr_err("test_bpf: no test named '%s' found for '%s' suite.\n",
> +                              test_name, test_suite);
> +                       return -EINVAL;
> +               }
> +               test_range[0] = idx;
> +               test_range[1] = idx;
> +       } else {
> +               /*
> +                * check that the supplied test_range is valid.
> +                */
> +               if (!strcmp(test_suite, "test_bpf")) {
> +                       if (test_range[0] >= ARRAY_SIZE(tests) ||
> +                           test_range[1] >= ARRAY_SIZE(tests) ||
> +                           test_range[0] < 0 || test_range[1] < 0) {
> +                               pr_err("test_bpf: test_range is out of bound for '%s' suite.\n",
> +                                      test_suite);
> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               if (!strcmp(test_suite, "test_tail_calls")) {
> +                       if (test_range[0] >= ARRAY_SIZE(tail_call_tests) ||
> +                           test_range[1] >= ARRAY_SIZE(tail_call_tests) ||
> +                           test_range[0] < 0 || test_range[1] < 0) {
> +                               pr_err("test_bpf: test_range is out of bound for '%s' suite.\n",
> +                                      test_suite);
> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               if (!strcmp(test_suite, "test_skb_segment")) {
> +                       if (test_range[0] >= ARRAY_SIZE(skb_segment_tests) ||
> +                           test_range[1] >= ARRAY_SIZE(skb_segment_tests) ||
> +                           test_range[0] < 0 || test_range[1] < 0) {
> +                               pr_err("test_bpf: test_range is out of bound for '%s' suite.\n",
> +                                      test_suite);
> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               if (test_range[1] < test_range[0]) {
> +                       pr_err("test_bpf: test_range is ending before it starts.\n");
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}

I would suggest that you compute the valid range first, depending on
the value of test_suite. Then you don't have to duplicate the
constraint checks for each suite name.

> +
> +static __init void destroy_bpf_tests(void)
> +{
> +}
> +
>  static int __init test_bpf_init(void)
>  {
>         struct bpf_array *progs = NULL;
>         int ret;
>
> +       if (strlen(test_suite) &&
> +           strcmp(test_suite, "test_bpf") &&
> +           strcmp(test_suite, "test_tail_calls") &&
> +           strcmp(test_suite, "test_skb_segment")) {
> +               pr_err("test_bpf: invalid test_suite '%s' specified.\n", test_suite);
> +               return -EINVAL;
> +       }
> +
> +       /*
> +        * if test_suite is not specified, but test_id, test_name or test_range
> +        * is specified, set 'test_bpf' as the default test suite.
> +        */
> +       if (!strlen(test_suite) &&
> +           (test_id != -1 || strlen(test_name) ||
> +           (test_range[0] != -1 || test_range[1] != -1))) {

If you initialize test_range[] as suggested, the range condition here
will need to be updated.

> +               pr_info("test_bpf: set 'test_bpf' as the default test_suite.\n");
> +               strcpy(test_suite, "test_bpf");
> +       }
> +
> +       /* if test_range is not specified, set the limit of test_range */
> +       if (test_range[0] == -1 && test_range[1] == -1) {
> +               /* if test_suite is not specified, set the possible max upper limit */
> +               if (!strlen(test_suite)) {
> +                       test_range[0] = 0;
> +                       test_range[1] = ARRAY_SIZE(tests) - 1;
> +               /* otherwise, set the limit of each test_suite */
> +               } else if (!strcmp(test_suite, "test_bpf")) {
> +                       test_range[0] = 0;
> +                       test_range[1] = ARRAY_SIZE(tests) - 1;
> +               } else if (!strcmp(test_suite, "test_tail_calls")) {
> +                       test_range[0] = 0;
> +                       test_range[1] = ARRAY_SIZE(tail_call_tests) - 1;
> +               } else if (!strcmp(test_suite, "test_skb_segment")) {
> +                       test_range[0] = 0;
> +                       test_range[1] = ARRAY_SIZE(skb_segment_tests) - 1;
> +               }
> +       }

if you initialize test_range as suggested, the block above is not needed.

> +
>         ret = prepare_bpf_tests();
>         if (ret < 0)
>                 return ret;
>
> -       ret = test_bpf();
> -       destroy_bpf_tests();
> -       if (ret)
> -               return ret;
> +       if (!strlen(test_suite) || !strcmp(test_suite, "test_bpf")) {
> +               ret = test_bpf();
> +               destroy_bpf_tests();
> +               if (ret)
> +                       return ret;
> +       }
>
> -       ret = prepare_tail_call_tests(&progs);
> -       if (ret)
> -               return ret;
> -       ret = test_tail_calls(progs);
> -       destroy_tail_call_tests(progs);
> -       if (ret)
> -               return ret;
> +       if (!strlen(test_suite) || !strcmp(test_suite, "test_tail_calls")) {
> +               ret = prepare_tail_call_tests(&progs);
> +               if (ret)
> +                       return ret;
> +               ret = test_tail_calls(progs);
> +               destroy_tail_call_tests(progs);
> +               if (ret)
> +                       return ret;
> +       }
>
> -       return test_skb_segment();
> +       if (!strlen(test_suite) || !strcmp(test_suite, "test_skb_segment"))
> +               return test_skb_segment();
> +
> +       return 0;
>  }
>
>  static void __exit test_bpf_exit(void)
> --
> 2.1.0
>

Powered by blists - more mailing lists