Messages in this thread Patch in this message | | | Date | Sat, 22 Jul 2023 19:29:39 -0700 | From | Yury Norov <> | 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@google.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.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
| |