Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20230720173956.3674987-1-glider@google.com> <20230720173956.3674987-3-glider@google.com>
 <ZLyQk5Wmcx7Cf84W@yury-ThinkPad>
In-Reply-To: <ZLyQk5Wmcx7Cf84W@yury-ThinkPad>
From:   Alexander Potapenko <glider@google.com>
Date:   Fri, 22 Sep 2023 09:57:32 +0200
Message-ID: <CAG_fn=WQORJConbBPzaNF2Nuxaf9Jmhy1Vgj=nk7gj902SQxOA@mail.gmail.com>
Subject: Re: [PATCH v4 2/5] lib/test_bitmap: add tests for bitmap_{set,get}_value()
To:     Yury Norov <yury.norov@gmail.com>
Cc:     catalin.marinas@arm.com, will@kernel.org, pcc@google.com,
        andreyknvl@gmail.com, andriy.shevchenko@linux.intel.com,
        linux@rasmusvillemoes.dk, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, eugenis@google.com,
        syednwaris@gmail.com, william.gray@linaro.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

> > +/*
> > + * Test bitmap should be big enough to include the cases when start is not in
> > + * the first word, and start+nbits lands in the following word.
> > + */
> > +#define TEST_BIT_LEN (BITS_PER_LONG * 3)
>
> Why not just 1000? Is your code safe against unaligned bitmaps?

done

> > +#define TEST_BYTE_LEN (BITS_TO_LONGS(TEST_BIT_LEN) * sizeof(unsigned long))
>
> BITS_TO_BYTES

done

>
> > +static void __init test_set_get_value(void)
>
> test_bitmap_read_write. Here, and in subjects for #1 and #2.

done

> > +{
> > +     DECLARE_BITMAP(bitmap, TEST_BIT_LEN);
> > +     DECLARE_BITMAP(exp_bitmap, TEST_BIT_LEN);
> > +     /* Prevent constant folding. */
> > +     volatile unsigned long zero_bits = 0;
>
> Use READ_ONCE() instead of volatile

done

> > +     unsigned long val, bit;
> > +     int i;
> > +
> > +     /* Setting/getting zero bytes should not crash the kernel. */
> > +     bitmap_write(NULL, 0, 0, zero_bits);
> > +     val = bitmap_read(NULL, 0, zero_bits);
> > +     expect_eq_ulong(0, val);
>
> No, val is undefined.

Why? bitmap_read(..., ..., 0) always returns 0.

>
> > +
> > +     /*
> > +      * Ensure that bitmap_read() reads the same value that was previously
> > +      * written, and two consequent values are correctly merged.
> > +      * The resulting bit pattern is asymmetric to rule out possible issues
> > +      * with bit numeration order.
> > +      */
> > +     for (i = 0; i < TEST_BIT_LEN - 7; i++) {
>
> Can you add some empty lines in the block below in sake of
> readability? Maybe after expect()?

Done

(snip)

> I suggested the other test:
>
> val = DEADBEEF;
> for (nbits = 1; nbits <= BITS_PER_LONG; nbits++)
>         for (start = 0; start < 1000; i++) {
>                 if (start + nbits >= 1000)
>                         break;;
>
>                 v = val & GENMASK(nbits - 1, 0);
>
>                 memset(bitmap, 0xaa /* also 0xff and 0x00 */, TEST_BYTE_LEN);
>                 memset(exp_bitmap, 0xaa, TEST_BYTE_LEN);
>
>                 for (n = 0; n < nbits; n++)
>                         __assign_bit(v & BIT(n), exp_bitmap, start + n);
>
>                 bitmap_write(bitmap, v, start, nbits);
>                 expect_eq_bitmap(exp_bitmap, bitmap, 1000);
>
>                 r = bitmap_read(bitmap, start, nbits);
>                 expect_eq(r, v);
>         }

I factored out test cases that can benefit from different background
patterns into a separate function and added yours there. Thanks!

> > @@ -1249,6 +1328,8 @@ static void __init selftest(void)
> >       test_for_each_clear_bitrange_from();
> >       test_for_each_set_clump8();
> >       test_for_each_set_bit_wrap();
> > +
> > +     test_set_get_value();
>
> This should append the test_bitmap_* section

Done

> Thanks,
> Yury

Thanks a lot for your comments!