[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZLyQk5Wmcx7Cf84W@yury-ThinkPad>
Date: Sat, 22 Jul 2023 19:29:39 -0700
From: Yury Norov <yury.norov@...il.com>
To: Alexander Potapenko <glider@...gle.com>
Cc: catalin.marinas@....com, will@...nel.org, pcc@...gle.com,
andreyknvl@...il.com, andriy.shevchenko@...ux.intel.com,
linux@...musvillemoes.dk, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, eugenis@...gle.com,
syednwaris@...il.com, william.gray@...aro.org
Subject: Re: [PATCH v4 2/5] lib/test_bitmap: add tests for
bitmap_{set,get}_value()
On Thu, Jul 20, 2023 at 07:39:53PM +0200, Alexander Potapenko wrote:
> Add basic tests ensuring that values can be added at arbitrary positions
> of the bitmap, including those spanning into the adjacent unsigned
> longs.
>
> Signed-off-by: Alexander Potapenko <glider@...gle.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
[...]
> +/*
> + * 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?
> +#define TEST_BYTE_LEN (BITS_TO_LONGS(TEST_BIT_LEN) * sizeof(unsigned long))
BITS_TO_BYTES
> +static void __init test_set_get_value(void)
test_bitmap_read_write. Here, and in subjects for #1 and #2.
> +{
> + 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
> + 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.
> +
> + /*
> + * 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()?
> + bitmap_zero(bitmap, TEST_BIT_LEN);
> + bitmap_write(bitmap, 0b10101UL, i, 5);
> + val = bitmap_read(bitmap, i, 5);
> + expect_eq_ulong(0b10101UL, val);
> + bitmap_write(bitmap, 0b101UL, i + 5, 3);
> + val = bitmap_read(bitmap, i + 5, 3);
> + expect_eq_ulong(0b101UL, val);
> + val = bitmap_read(bitmap, i, 8);
> + expect_eq_ulong(0b10110101UL, val);
> + }
> +
> + /*
> + * Check that setting a single bit does not accidentally touch the
> + * adjacent bits.
> + */
> + for (i = 0; i < TEST_BIT_LEN; i++) {
> + /*
> + * A 0b10101010 pattern to catch both 0s replaced to 1s and vice
> + * versa.
> + */
> + memset(bitmap, 0xaa, TEST_BYTE_LEN);
> + memset(exp_bitmap, 0xaa, TEST_BYTE_LEN);
> + for (bit = 0; bit <= 1; bit++) {
> + bitmap_write(bitmap, bit, i, 1);
> + __assign_bit(i, exp_bitmap, bit);
> + expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN);
> + }
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);
}
> + }
> +
> + /* Ensure setting 0 bits does not change anything. */
> + memset(bitmap, 0xaa, TEST_BYTE_LEN);
> + memset(exp_bitmap, 0xaa, TEST_BYTE_LEN);
> + for (i = 0; i < TEST_BIT_LEN; i++) {
> + bitmap_write(bitmap, ~0UL, i, 0);
> + expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN);
> + }
> +}
> +#undef TEST_BYTE_LEN
> +#undef TEST_BIT_LEN
> +
> static void __init selftest(void)
> {
> test_zero_clear();
> @@ -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
Thanks,
Yury
Powered by blists - more mailing lists