Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210926223322.848641-1-isabellabdoamaral@usp.br> <20210926223322.848641-4-isabellabdoamaral@usp.br>
In-Reply-To: <20210926223322.848641-4-isabellabdoamaral@usp.br>
From:   David Gow <davidgow@google.com>
Date:   Sat, 2 Oct 2021 15:20:27 +0800
Message-ID: <CABVgOSnC0PfFaT8AWRfE=Rv-RyF5+ZKR6HjnwA9Gp2w35UXC0A@mail.gmail.com>
Subject: Re: [PATCH v2 3/5] test_hash.c: split test_hash_init
To:     Isabella Basso <isabellabdoamaral@usp.br>
Cc:     Geert Uytterhoeven <geert@linux-m68k.org>, ferreiraenzoa@gmail.com,
        augusto.duraes33@gmail.com,
        Brendan Higgins <brendanhiggins@google.com>,
        Daniel Latypov <dlatypov@google.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        ~lkcamp/patches@lists.sr.ht, rodrigosiqueiramelo@gmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@usp.br> wrote:
>
> Split up test_hash_init so that it calls each test more explicitly
> insofar it is possible without rewriting the entire file. This aims at
> improving readability.
>
> Split tests performed on string_or as they don't interfere with those
> performed in hash_or. Also separate pr_info calls about skipped tests as
> they're not part of the tests themselves, but only warn about
> (un)defined arch-specific hash functions.
>
> Changes since v1:
> - As suggested by David Gow:
>   1. Rename arch-specific test functions.
>   2. Remove spare whitespace changes.
> - As suggested by Marco Elver:
>   1. Add struct for carrying test variables.

Nit: Move the changelog to after the "---" (and the correct patch).

>
> Tested-by: David Gow <davidgow@google.com>
> Signed-off-by: Isabella Basso <isabellabdoamaral@usp.br>
> ---

This seems good to me, though I admit this is the part I'm probably
least knowledgeable about. I'm pretty sure there has to be a more
straightforward way to test some of these hash functions, but it's
probably better to keep this as-is rather than doing anything too
drastic in the middle of the port to KUnit.

The biggest downside here is that we now double the number of calls to
fill_buffer() and full_name_hash(), so the test is likely to be a bit
slower. It still runs fast enough (at least with the default SIZE of
256) that it's not noticeable to me, though, so I don't think it's a
problem.

Apart from Marco's comment about the changelog in the commit message
is fixed, this is:

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David


>  lib/test_hash.c | 66 ++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/lib/test_hash.c b/lib/test_hash.c
> index 08fe63776c4f..db9dd18b4e8b 100644
> --- a/lib/test_hash.c
> +++ b/lib/test_hash.c
> @@ -153,11 +153,39 @@ test_int_hash(unsigned long long h64, u32 hash_or[2][33])
>
>  #define SIZE 256       /* Run time is cubic in SIZE */
>
> -static int __init
> -test_hash_init(void)
> +static int __init test_string_or(void)
>  {
>         char buf[SIZE+1];
> -       u32 string_or = 0, hash_or[2][33] = { { 0, } };
> +       u32 string_or = 0;
> +       int i, j;
> +
> +       fill_buf(buf, SIZE, 1);
> +
> +       /* Test every possible non-empty substring in the buffer. */
> +       for (j = SIZE; j > 0; --j) {
> +               buf[j] = '\0';
> +
> +               for (i = 0; i <= j; i++) {
> +                       u32 h0 = full_name_hash(buf+i, buf+i, j-i);
> +
> +                       string_or |= h0;
> +               } /* i */
> +       } /* j */
> +
> +       /* The OR of all the hash values should cover all the bits */
> +       if (~string_or) {
> +               pr_err("OR of all string hash results = %#x != %#x",
> +                      string_or, -1u);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int __init test_hash_or(void)
> +{
> +       char buf[SIZE+1];
> +       u32 hash_or[2][33] = { { 0, } };
>         unsigned tests = 0;
>         unsigned long long h64 = 0;
>         int i, j;
> @@ -187,7 +215,6 @@ test_hash_init(void)
>                                 return -EINVAL;
>                         }
>
> -                       string_or |= h0;
>                         h64 = h64 << 32 | h0;   /* For use with hash_64 */
>                         if (!test_int_hash(h64, hash_or))
>                                 return -EINVAL;
> @@ -195,12 +222,6 @@ test_hash_init(void)
>                 } /* i */
>         } /* j */
>
> -       /* The OR of all the hash values should cover all the bits */
> -       if (~string_or) {
> -               pr_err("OR of all string hash results = %#x != %#x",
> -                       string_or, -1u);
> -               return -EINVAL;
> -       }
>         if (~hash_or[0][0]) {
>                 pr_err("OR of all __hash_32 results = %#x != %#x",
>                         hash_or[0][0], -1u);
> @@ -232,6 +253,13 @@ test_hash_init(void)
>                 }
>         }
>
> +       pr_notice("%u tests passed.", tests);
> +
> +       return 0;
> +}
> +
> +static void __init notice_skipped_tests(void)
> +{
>         /* Issue notices about skipped tests. */
>  #ifdef HAVE_ARCH__HASH_32
>  #if HAVE_ARCH__HASH_32 != 1
> @@ -247,10 +275,24 @@ test_hash_init(void)
>  #else
>         pr_info("hash_64() has no arch implementation to test.");
>  #endif
> +}
>
> -       pr_notice("%u tests passed.", tests);
> +static int __init
> +test_hash_init(void)
> +{
> +       int ret;
>
> -       return 0;
> +       ret = test_string_or();
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = test_hash_or();
> +       if (ret < 0)
> +               return ret;
> +
> +       notice_skipped_tests();
> +
> +       return ret;
>  }
>
>  static void __exit test_hash_exit(void)
> --
> 2.33.0
>