Re: [PATCH v5 2/4] lib/test_bitmap.c: Add for_each_set_clump test cases

[Syed Nayyar Waris]

On Mon, May 4, 2020 at 5:08 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Sun, May 03, 2020 at 04:41:42AM +0530, Syed Nayyar Waris wrote:
> > The introduction of the generic for_each_set_clump macro need test
> > cases to verify the implementation. This patch adds test cases for
> > scenarios in which clump sizes are 8 bits, 24 bits, 30 bits and 6 bits.
> > The cases contain situations where clump is getting split at the word
> > boundary and also when zeroes are present in the start and middle of
> > bitmap.
>
>
> > +static const unsigned long bitmap_test_data[] __initconst = {
> > +     0x38000201,
> > +     0x05ff0f38,
> > +     0xeffedcba,
> > +     0xbbbbabcd,
> > +     0x000000aa,
> > +     0x000000aa,
> > +     0x00ff0000,
> > +     0xaaaaaa00,
> > +     0xff000000,
> > +     0x00aa0000,
> > +     0x00000000,
> > +     0x00000000,
> > +     0x00000000,
> > +     0x0f000000,
> > +     0x00000ac0,
> > +};
> > +
> > +static const unsigned long clump_exp1[] __initconst = {
> > +     0x01,   /* 1 bit set */
> > +     0x02,   /* non-edge 1 bit set */
> > +     0x00,   /* zero bits set */
> > +     0x38,   /* 3 bits set across 4-bit boundary */
> > +     0x38,   /* Repeated clump */
> > +     0x0F,   /* 4 bits set */
> > +     0xFF,   /* all bits set */
> > +     0x05,   /* non-adjacent 2 bits set */
> > +};
> > +
> > +static const unsigned long clump_exp2[] __initconst = {
> > +     0xfedcba,       /* 24 bits */
> > +     0xabcdef,
> > +     0xaabbbb,       /* Clump split between 2 words */
> > +     0x000000,       /* zeroes in between */
> > +     0x0000aa,
> > +     0x000000,
> > +     0x0000ff,
> > +     0xaaaaaa,
> > +     0x000000,
> > +     0x0000ff,
> > +};
> > +
> > +static const unsigned long clump_exp3[] __initconst = {
> > +     0x00000000,     /* starting with 0s*/
> > +     0x00000000,     /* All 0s */
> > +     0x00000000,
> > +     0x00000000,
> > +     0x3f00000f,     /* Non zero set */
> > +     0x2aa80003,
> > +     0x00000aaa,
> > +     0x00003fc0,
> > +};
> > +
> > +static const unsigned long clump_exp4[] __initconst = {
> > +     0x00,
> > +     0x2b,
> > +};
> > +
>
> One more struct here, like
>
> struct clump_test_data {
>         unsigned long *data; // with offset implied
>         unsigned long count;
>         unsigned long size;
>         unsigned long limit;
>         unsigned long *exp;
> };
>
> > +static const unsigned long * const clump_data[] __initconst = {
> > +     clump_exp1,
> > +     clump_exp2,
> > +     clump_exp3,
> > +     clump_exp4,
> > +};
> > +
> >  static void __init test_for_each_set_clump8(void)
> >  {
> >  #define CLUMP_EXP_NUMBITS 64
> > @@ -610,6 +708,48 @@ static void __init test_for_each_set_clump8(void)
> >               expect_eq_clump8(start, CLUMP_EXP_NUMBITS, clump_exp, &clump);
> >  }
> >
> > +static void __init execute_for_each_set_clump_test(unsigned long *bits,
> > +                             unsigned long size,
> > +                             unsigned long clump_size,
> > +                             const unsigned long *clump_exp)
> > +{
> > +     unsigned long start, clump;
> > +
> > +     for_each_set_clump(start, clump, bits, size, clump_size)
> > +             expect_eq_clump(start, size, clump_exp, &clump, clump_size);
> > +}
> > +
>
> > +static void __init prepare_test_data(unsigned long * bits,
> > +                             const unsigned long * test_data,
> > +                             int start, int count)
>
> ... prepare_test_data(struct clump_test_data *data)
> {
>         ...
> }

Hi. I have sent a new patchset (v6) incorporating your review
comments. Regarding your above review comment for function
'prepare_test_data', the parameter 'struct clump_test_data' has
already been declared outside (as was suggested.. see further above),
so I didn't require that (struct clump_test_data) as a parameter for
the function, as it can be accessed from everywhere.

Further, below ...

...

> > +static void __init test_for_each_set_clump(void)
> > +{
> > +     int i;
> > +     int count[] = {2, 8, 4, 1};
> > +     int offset[] = {0, 2, 10, 14};
> > +     unsigned long limit[] = {64, 240, 240, 18};
> > +     unsigned long clump_size[] = {8, 24, 30, 6};
> > +     DECLARE_BITMAP(bits, 256);
> > +
> > +     for(i = 0; i < 4; i++)
> > +     {
> > +             prepare_test_data(bits, bitmap_test_data, offset[i], count[i]);
> > +             execute_for_each_set_clump_test(bits, limit[i],
> > +                                     clump_size[i], clump_data[i]);
> > +     }
>
> As I told you it should be as simple as
>
>         unsigned int i;
>
>         for (i < ARRAY_SIZE(clump_test_data)) {
>                 prepare()
>                 execute()
>         }
>

Since it is required to use 'for loop' with 'ARRAY_SIZE', this implies
that 'clump_test_data'  be an array, which has been done so. I have
done here a minor addition that the 'prepare_test_data' function is
called with argument 'i' (index) to prepare data specifically for the
ith test case. Without passing 'i' it would not be possible (I
believe) to populate the bitmap properly for the ith test case.

Let me know if the new patchset seems alright. Thank you.

Regards
Syed Nayyar Waris