Re: [PATCH v5 2/5] lib/test_bitmap: add tests for bitmap_{read,write}()

From: Yury Norov <yury.norov@gmail.com>
To: Alexander Potapenko <glider@google.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	catalin.marinas@arm.com, will@kernel.org, pcc@google.com,
	andreyknvl@gmail.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 v5 2/5] lib/test_bitmap: add tests for bitmap_{read,write}()
Date: Mon, 25 Sep 2023 09:06:33 -0700	[thread overview]
Message-ID: <ZRGv/5QNbYGIDkgw@yury-ThinkPad> (raw)
In-Reply-To: <CAG_fn=WX+yAFHtbsxSvd41P61jjWtFEePqOs_1AKGJcgaWfVag@mail.gmail.com>

On Mon, Sep 25, 2023 at 04:54:00PM +0200, Alexander Potapenko wrote:
> On Mon, Sep 25, 2023 at 3:09 PM Alexander Potapenko <glider@google.com> wrote:
> >
> > On Mon, Sep 25, 2023 at 2:23 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Mon, Sep 25, 2023 at 02:16:37PM +0200, Alexander Potapenko wrote:
> > >
> > > ...
> > >
> > > > > +/*
> > > > > + * 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 (1000)
> > > >
> > > > Dunno why this didn't fire previously, but CONFIG_CPU_BIG_ENDIAN=y
> > > > kernel reports mismatches here, presumably because the last quad word
> > > > ends up partially initialized.
> > >
> > > Hmm... But if designed and used correctly it shouldn't be the issue,
> > > and 1000, I believe, is carefully chosen to be specifically not dividable
> > > by pow-of-2 value.
> > >
> >
> > The problem manifests already right after initialization:
> >
> > static void __init test_bit_len_1000(void)
> > {
> >         DECLARE_BITMAP(bitmap, TEST_BIT_LEN);
> >         DECLARE_BITMAP(exp_bitmap, TEST_BIT_LEN);
> >         memset(bitmap, 0x00, TEST_BYTE_LEN);
> >         memset(exp_bitmap, 0x00, TEST_BYTE_LEN);
> >         expect_eq_bitmap(exp_bitmap, bitmap, TEST_BIT_LEN);
> > }
> 
> The problem is that there's no direct analog of memset() that can be
> used to initialize bitmaps on both BE and LE systems.

memset fills an array of chars with the same value. In bitmap world we operate
on array of bits, and there are only 2 possible values: '0' and '1'. For those
we've got bitmap_zero() and bitmap_fill().

> bitmap_zero() and bitmap_set() work by rounding up the bitmap size to
> BITS_TO_LONGS(nbits), but there's no bitmap_memset() that would do the
> same for an arbitrary byte pattern.
> We could call memset(..., ..., BITS_TO_LONGS(TEST_BIT_LEN)), but that
> would be similar to declaring a bigger bitmap and not testing the last
> 24 bits.

No, you couldn't. On the test level, bitmap should be considered as a
black box. memset()'ing it may (and did) damage internal structure.

If you have some pattern in mind, you can use bitmap_parselist(). For example,
you can set every 2nd bit in your bitmap like this:

        bitmap_parselist("all:1/2", bitmap, 1000);

Check for almost 100 examples of bitmap_parselist usage in the test for
bitmap_parselist in the same file.

> Overall, unless allocating and initializing bitmaps with size
> divisible by sizeof(long), most of bitmap.c is undefined behavior, so
> I don't think it makes much sense to specifically test this case here
> (given that we do not extend bitmap_equal() in the patch set).

This is wrong statement. People spent huge amount of time making bitmap
API working well for all combinations of lengths and endiannesses,
including Andy and me.

NAK for this and for ignoring my other comment to v4.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel