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

[Alexander Potapenko]

On Mon, Sep 25, 2023 at 6:06 PM Yury Norov <yury.norov@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Sep 25, 2023 at 2:23 PM Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxxxxxxxx> 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.

You are right about this. bitmap_zero() and bitmap_fill() are calling
memset() under the hood, but I shouldn't have assumed doing raw memset
is safe.
Unfortunately lib/test_bitmap.c does a bunch of memsets already, which
probably led to the confusion.


> 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.

Thanks! This solves my problem.
I am planning to use an intermediate bitmap to avoid parsing the
pattern multiple times.

>
> > 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.

Please accept my apologies for that, I didn't mean to insult you,
Andy, or anyone else.
My understanding of the comment at the top of lib/bitmap.c was that
the bitmap API is expected to work in the presence of uninitialized
values, which is actually not what the comment says.
bitmap_zero() and such do ensure that none of the tail bytes remain
uninitialized, so we are safe as long as those functions are used.


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

If you are talking about this comment:
https://lore.kernel.org/lkml/ZQ2WboApqYyEkjjG@yury-ThinkPad/, I was
going to incorporate it in v6 (as it was sent after I published v5).
I am fine with not mandating the return value for reading/writing zero bytes.