Re: [PATCH v4 2/5] lib/test_bitmap: add tests for bitmap_{set,get}_value()

From: Yury Norov
Date: Sat Jul 22 2023 - 22:50:53 EST


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@xxxxxxxxxx>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

[...]

> +/*
> + * 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