Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 22 Jul 2023 19:29:39 -0700
From:   Yury Norov <yury.norov@gmail.com>
To:     Alexander Potapenko <glider@google.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
Subject: Re: [PATCH v4 2/5] lib/test_bitmap: add tests for
 bitmap_{set,get}_value()
Message-ID: <ZLyQk5Wmcx7Cf84W@yury-ThinkPad>
References: <20230720173956.3674987-1-glider@google.com>
 <20230720173956.3674987-3-glider@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230720173956.3674987-3-glider@google.com>
Precedence: bulk

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