Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 22/02/18 11:14, Igor Stoppa wrote:
> 
> 
> On 22/02/18 00:28, Kees Cook wrote:
>> On Tue, Feb 20, 2018 at 8:59 AM, Igor Stoppa  wrote:
>>>
>>>
>>> On 13/02/18 01:50, Kees Cook wrote:
 On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  
 wrote:
> 
> [...]
> 
> +   genalloc_selftest();

 I wonder if it's possible to make this module-loadable instead? That
 way it could be built and tested separately.
>>>
>>> In my case modules are not an option.
>>> Of course it could be still built in, but what is the real gain?
>>
>> The gain for it being a module is that it can be loaded and tested
>> separately from the final kernel image and module collection. For
>> example, Chrome OS builds lots of debugging test modules but doesn't
>> include them on the final image. They're only used for testing, and
>> can be separate from the kernel and "production" modules.
> 
> ok

I started to turn this into a module, but after all it doesn't seem like
it would give any real advantage, compared to the current implementation.

This testing is meant to catch bugs in memory management as early as
possible in the boot phase, before users of genalloc start to fail in
mysterious ways.

This includes, but is not limited to: MCE on x86, uncached pages
provider on arm64, dma on arm.

Should genalloc fail, it's highly unlikely that the test rig would even
reach the point where it can load a module and run it, even if it is
located in initrd.

The test would not be run, precisely at the moment where its output
would be needed the most, leaving a crash log that is hard to debug
because of memory corruption.

I do not know how Chrome OS builds are organized, but I imagine that
probably there is a separate test build, where options like lockdep,
ubsan, etc. are enabled.

All options that cannot be left enabled in a production kernel, but are
very useful for sanity checks and require a separate build.

Genalloc testing should be added there, rather than in a module, imho.

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 22/02/18 11:14, Igor Stoppa wrote:
> 
> 
> On 22/02/18 00:28, Kees Cook wrote:
>> On Tue, Feb 20, 2018 at 8:59 AM, Igor Stoppa  wrote:
>>>
>>>
>>> On 13/02/18 01:50, Kees Cook wrote:
 On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  
 wrote:
> 
> [...]
> 
> +   genalloc_selftest();

 I wonder if it's possible to make this module-loadable instead? That
 way it could be built and tested separately.
>>>
>>> In my case modules are not an option.
>>> Of course it could be still built in, but what is the real gain?
>>
>> The gain for it being a module is that it can be loaded and tested
>> separately from the final kernel image and module collection. For
>> example, Chrome OS builds lots of debugging test modules but doesn't
>> include them on the final image. They're only used for testing, and
>> can be separate from the kernel and "production" modules.
> 
> ok

I started to turn this into a module, but after all it doesn't seem like
it would give any real advantage, compared to the current implementation.

This testing is meant to catch bugs in memory management as early as
possible in the boot phase, before users of genalloc start to fail in
mysterious ways.

This includes, but is not limited to: MCE on x86, uncached pages
provider on arm64, dma on arm.

Should genalloc fail, it's highly unlikely that the test rig would even
reach the point where it can load a module and run it, even if it is
located in initrd.

The test would not be run, precisely at the moment where its output
would be needed the most, leaving a crash log that is hard to debug
because of memory corruption.

I do not know how Chrome OS builds are organized, but I imagine that
probably there is a separate test build, where options like lockdep,
ubsan, etc. are enabled.

All options that cannot be left enabled in a production kernel, but are
very useful for sanity checks and require a separate build.

Genalloc testing should be added there, rather than in a module, imho.

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 22/02/18 00:28, Kees Cook wrote:
> On Tue, Feb 20, 2018 at 8:59 AM, Igor Stoppa  wrote:
>>
>>
>> On 13/02/18 01:50, Kees Cook wrote:
>>> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:

[...]

 +   genalloc_selftest();
>>>
>>> I wonder if it's possible to make this module-loadable instead? That
>>> way it could be built and tested separately.
>>
>> In my case modules are not an option.
>> Of course it could be still built in, but what is the real gain?
> 
> The gain for it being a module is that it can be loaded and tested
> separately from the final kernel image and module collection. For
> example, Chrome OS builds lots of debugging test modules but doesn't
> include them on the final image. They're only used for testing, and
> can be separate from the kernel and "production" modules.

ok

> 
>> [...]
>>
 +   BUG_ON(compare_bitmaps(pool, action->pattern));
>>>
>>> There's been a lot recently on BUG vs WARN. It does seem crazy to not
>>> BUG for an allocator selftest, but if we can avoid it, we should.
>>
>> If this fails, I would expect that memory corruption is almost guaranteed.
>> Do we really want to allow the boot to continue, possibly mounting a
>> filesystem, only to corrupt it? It seems very dangerous.
> 
> I would include the rationale in either a comment or the commit log.
> BUG() tends to need to be very well justified these days.

ok

> 
>>> Also, I wonder if it might make sense to split this series up a little
>>> more, as in:
>>>
>>> 1/n: add genalloc selftest
>>> 2/n: update bitmaps
>>> 3/n: add/change bitmap tests to selftest

[...]

> I'll leave it up to the -mm folks, but that was just my thought.

The reasons why I have doubts about your proposal are:

1) leaving 2/n and 3/n separate mean that the selftest could be broken,
if one does bisections with selftest enabled

2) since I would need to change the selftest, to make it work with the
updated bitmap, it would not really prove that the change is correct.

If there was a selftest that can work without changes, after the bitmap
update, I would definitely agree to introduce it first.

OTOH, as I wrote, the bitmap patch itself is already introducing
verification of the calculated value (from the bitmap) against the
provided value (from the call parameters).

This, unfortunately, cannot be split, because it still relies on the
"find end of the allocation" capability introduced by the very same patch.

Anyway, putting aside concerns about the verification of the correctness
of the patch, I still see point #1 as a problem: breaking bisectability.

Or did I not understand correctly how this works?

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 22/02/18 00:28, Kees Cook wrote:
> On Tue, Feb 20, 2018 at 8:59 AM, Igor Stoppa  wrote:
>>
>>
>> On 13/02/18 01:50, Kees Cook wrote:
>>> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:

[...]

 +   genalloc_selftest();
>>>
>>> I wonder if it's possible to make this module-loadable instead? That
>>> way it could be built and tested separately.
>>
>> In my case modules are not an option.
>> Of course it could be still built in, but what is the real gain?
> 
> The gain for it being a module is that it can be loaded and tested
> separately from the final kernel image and module collection. For
> example, Chrome OS builds lots of debugging test modules but doesn't
> include them on the final image. They're only used for testing, and
> can be separate from the kernel and "production" modules.

ok

> 
>> [...]
>>
 +   BUG_ON(compare_bitmaps(pool, action->pattern));
>>>
>>> There's been a lot recently on BUG vs WARN. It does seem crazy to not
>>> BUG for an allocator selftest, but if we can avoid it, we should.
>>
>> If this fails, I would expect that memory corruption is almost guaranteed.
>> Do we really want to allow the boot to continue, possibly mounting a
>> filesystem, only to corrupt it? It seems very dangerous.
> 
> I would include the rationale in either a comment or the commit log.
> BUG() tends to need to be very well justified these days.

ok

> 
>>> Also, I wonder if it might make sense to split this series up a little
>>> more, as in:
>>>
>>> 1/n: add genalloc selftest
>>> 2/n: update bitmaps
>>> 3/n: add/change bitmap tests to selftest

[...]

> I'll leave it up to the -mm folks, but that was just my thought.

The reasons why I have doubts about your proposal are:

1) leaving 2/n and 3/n separate mean that the selftest could be broken,
if one does bisections with selftest enabled

2) since I would need to change the selftest, to make it work with the
updated bitmap, it would not really prove that the change is correct.

If there was a selftest that can work without changes, after the bitmap
update, I would definitely agree to introduce it first.

OTOH, as I wrote, the bitmap patch itself is already introducing
verification of the calculated value (from the bitmap) against the
provided value (from the call parameters).

This, unfortunately, cannot be split, because it still relies on the
"find end of the allocation" capability introduced by the very same patch.

Anyway, putting aside concerns about the verification of the correctness
of the patch, I still see point #1 as a problem: breaking bisectability.

Or did I not understand correctly how this works?

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Kees Cook]

On Tue, Feb 20, 2018 at 8:59 AM, Igor Stoppa  wrote:
>
>
> On 13/02/18 01:50, Kees Cook wrote:
>> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
>
> [...]
>
>>>  lib/genalloc-selftest.c   | 400 
>>> ++
>>
>> Nit: make this test_genalloc.c instead.
>
> ok
>
> [...]
>
>>> +   genalloc_selftest();
>>
>> I wonder if it's possible to make this module-loadable instead? That
>> way it could be built and tested separately.
>
> In my case modules are not an option.
> Of course it could be still built in, but what is the real gain?

The gain for it being a module is that it can be loaded and tested
separately from the final kernel image and module collection. For
example, Chrome OS builds lots of debugging test modules but doesn't
include them on the final image. They're only used for testing, and
can be separate from the kernel and "production" modules.

> [...]
>
>>> +   BUG_ON(compare_bitmaps(pool, action->pattern));
>>
>> There's been a lot recently on BUG vs WARN. It does seem crazy to not
>> BUG for an allocator selftest, but if we can avoid it, we should.
>
> If this fails, I would expect that memory corruption is almost guaranteed.
> Do we really want to allow the boot to continue, possibly mounting a
> filesystem, only to corrupt it? It seems very dangerous.

I would include the rationale in either a comment or the commit log.
BUG() tends to need to be very well justified these days.

>> Also, I wonder if it might make sense to split this series up a little
>> more, as in:
>>
>> 1/n: add genalloc selftest
>> 2/n: update bitmaps
>> 3/n: add/change bitmap tests to selftest
>>
>> Maybe I'm over-thinking it, but the great thing about this self test
>> is that it's checking much more than just the bitmap changes you're
>> making, and that can be used to "prove" that genalloc continues to
>> work after the changes (i.e. the selftest passes before the changes,
>> and after, rather than just after).
>
> If I really have to ... but to me the evidence that the changes to the
> bitmaps do really work is already provided by the bitmap patch itself.
>
> Since the patch doesn't remove the parameter indicating the space to be
> freed, it can actually compare what the kernel passes to it vs. what it
> thinks the space should be.
>
> If the values were different, it would complain, but it doesn't ...
> Isn't that even stronger evidence that the bitmap changes work as expected?
>
> (eventually the parameter can be removed, but I intentionally left it,
> for facilitating the merge)

I'll leave it up to the -mm folks, but that was just my thought.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/6] genalloc: selftest

[Kees Cook]

On Tue, Feb 20, 2018 at 8:59 AM, Igor Stoppa  wrote:
>
>
> On 13/02/18 01:50, Kees Cook wrote:
>> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
>
> [...]
>
>>>  lib/genalloc-selftest.c   | 400 
>>> ++
>>
>> Nit: make this test_genalloc.c instead.
>
> ok
>
> [...]
>
>>> +   genalloc_selftest();
>>
>> I wonder if it's possible to make this module-loadable instead? That
>> way it could be built and tested separately.
>
> In my case modules are not an option.
> Of course it could be still built in, but what is the real gain?

The gain for it being a module is that it can be loaded and tested
separately from the final kernel image and module collection. For
example, Chrome OS builds lots of debugging test modules but doesn't
include them on the final image. They're only used for testing, and
can be separate from the kernel and "production" modules.

> [...]
>
>>> +   BUG_ON(compare_bitmaps(pool, action->pattern));
>>
>> There's been a lot recently on BUG vs WARN. It does seem crazy to not
>> BUG for an allocator selftest, but if we can avoid it, we should.
>
> If this fails, I would expect that memory corruption is almost guaranteed.
> Do we really want to allow the boot to continue, possibly mounting a
> filesystem, only to corrupt it? It seems very dangerous.

I would include the rationale in either a comment or the commit log.
BUG() tends to need to be very well justified these days.

>> Also, I wonder if it might make sense to split this series up a little
>> more, as in:
>>
>> 1/n: add genalloc selftest
>> 2/n: update bitmaps
>> 3/n: add/change bitmap tests to selftest
>>
>> Maybe I'm over-thinking it, but the great thing about this self test
>> is that it's checking much more than just the bitmap changes you're
>> making, and that can be used to "prove" that genalloc continues to
>> work after the changes (i.e. the selftest passes before the changes,
>> and after, rather than just after).
>
> If I really have to ... but to me the evidence that the changes to the
> bitmaps do really work is already provided by the bitmap patch itself.
>
> Since the patch doesn't remove the parameter indicating the space to be
> freed, it can actually compare what the kernel passes to it vs. what it
> thinks the space should be.
>
> If the values were different, it would complain, but it doesn't ...
> Isn't that even stronger evidence that the bitmap changes work as expected?
>
> (eventually the parameter can be removed, but I intentionally left it,
> for facilitating the merge)

I'll leave it up to the -mm folks, but that was just my thought.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 13/02/18 01:50, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:

[...]

>>  lib/genalloc-selftest.c   | 400 
>> ++
> 
> Nit: make this test_genalloc.c instead.

ok

[...]

>> +   genalloc_selftest();
> 
> I wonder if it's possible to make this module-loadable instead? That
> way it could be built and tested separately.

In my case modules are not an option.
Of course it could be still built in, but what is the real gain?

[...]

>> +config GENERIC_ALLOCATOR_SELFTEST
> 
> Like the other lib/test_*.c targets, I'd call this TEST_GENERIC_ALLOCATOR.

ok

[...]

>> +   BUG_ON(compare_bitmaps(pool, action->pattern));
> 
> There's been a lot recently on BUG vs WARN. It does seem crazy to not
> BUG for an allocator selftest, but if we can avoid it, we should.

If this fails, I would expect that memory corruption is almost guaranteed.
Do we really want to allow the boot to continue, possibly mounting a
filesystem, only to corrupt it? It seems very dangerous.

> Also, I wonder if it might make sense to split this series up a little
> more, as in:
> 
> 1/n: add genalloc selftest
> 2/n: update bitmaps
> 3/n: add/change bitmap tests to selftest
> 
> Maybe I'm over-thinking it, but the great thing about this self test
> is that it's checking much more than just the bitmap changes you're
> making, and that can be used to "prove" that genalloc continues to
> work after the changes (i.e. the selftest passes before the changes,
> and after, rather than just after).

If I really have to ... but to me the evidence that the changes to the
bitmaps do really work is already provided by the bitmap patch itself.

Since the patch doesn't remove the parameter indicating the space to be
freed, it can actually compare what the kernel passes to it vs. what it
thinks the space should be.

If the values were different, it would complain, but it doesn't ...
Isn't that even stronger evidence that the bitmap changes work as expected?


(eventually the parameter can be removed, but I intentionally left it,
for facilitating the merge)

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 13/02/18 01:50, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:

[...]

>>  lib/genalloc-selftest.c   | 400 
>> ++
> 
> Nit: make this test_genalloc.c instead.

ok

[...]

>> +   genalloc_selftest();
> 
> I wonder if it's possible to make this module-loadable instead? That
> way it could be built and tested separately.

In my case modules are not an option.
Of course it could be still built in, but what is the real gain?

[...]

>> +config GENERIC_ALLOCATOR_SELFTEST
> 
> Like the other lib/test_*.c targets, I'd call this TEST_GENERIC_ALLOCATOR.

ok

[...]

>> +   BUG_ON(compare_bitmaps(pool, action->pattern));
> 
> There's been a lot recently on BUG vs WARN. It does seem crazy to not
> BUG for an allocator selftest, but if we can avoid it, we should.

If this fails, I would expect that memory corruption is almost guaranteed.
Do we really want to allow the boot to continue, possibly mounting a
filesystem, only to corrupt it? It seems very dangerous.

> Also, I wonder if it might make sense to split this series up a little
> more, as in:
> 
> 1/n: add genalloc selftest
> 2/n: update bitmaps
> 3/n: add/change bitmap tests to selftest
> 
> Maybe I'm over-thinking it, but the great thing about this self test
> is that it's checking much more than just the bitmap changes you're
> making, and that can be used to "prove" that genalloc continues to
> work after the changes (i.e. the selftest passes before the changes,
> and after, rather than just after).

If I really have to ... but to me the evidence that the changes to the
bitmaps do really work is already provided by the bitmap patch itself.

Since the patch doesn't remove the parameter indicating the space to be
freed, it can actually compare what the kernel passes to it vs. what it
thinks the space should be.

If the values were different, it would complain, but it doesn't ...
Isn't that even stronger evidence that the bitmap changes work as expected?


(eventually the parameter can be removed, but I intentionally left it,
for facilitating the merge)

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Kees Cook]

On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
> Introduce a set of macros for writing concise test cases for genalloc.
>
> The test cases are meant to provide regression testing, when working on
> new functionality for genalloc.
>
> Primarily they are meant to confirm that the various allocation strategy
> will continue to work as expected.
>
> The execution of the self testing is controlled through a Kconfig option.
>
> Signed-off-by: Igor Stoppa 
> ---
>  include/linux/genalloc-selftest.h |  26 +++
>  init/main.c   |   2 +
>  lib/Kconfig   |  15 ++
>  lib/Makefile  |   1 +
>  lib/genalloc-selftest.c   | 400 
> ++

Nit: make this test_genalloc.c instead.

>  5 files changed, 444 insertions(+)
>  create mode 100644 include/linux/genalloc-selftest.h
>  create mode 100644 lib/genalloc-selftest.c
>
> diff --git a/include/linux/genalloc-selftest.h 
> b/include/linux/genalloc-selftest.h
> new file mode 100644
> index ..e0ac8f963abc
> --- /dev/null
> +++ b/include/linux/genalloc-selftest.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * genalloc-selftest.h
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + */
> +
> +
> +#ifndef __LINUX_GENALLOC_SELFTEST_H
> +#define __LINUX_GENALLOC_SELFTEST_H
> +
> +
> +#ifdef CONFIG_GENERIC_ALLOCATOR_SELFTEST
> +
> +#include 
> +
> +void genalloc_selftest(void);
> +
> +#else
> +
> +static inline void genalloc_selftest(void){};
> +
> +#endif
> +
> +#endif
> diff --git a/init/main.c b/init/main.c
> index a8100b954839..fb844aa3eb8c 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -89,6 +89,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -660,6 +661,7 @@ asmlinkage __visible void __init start_kernel(void)
>  */
> mem_encrypt_init();
>
> +   genalloc_selftest();

I wonder if it's possible to make this module-loadable instead? That
way it could be built and tested separately. Regardless, I love
gaining more internal selftests, this is great. :)

>  #ifdef CONFIG_BLK_DEV_INITRD
> if (initrd_start && !initrd_below_start_ok &&
> page_to_pfn(virt_to_page((void *)initrd_start)) < min_low_pfn) {
> diff --git a/lib/Kconfig b/lib/Kconfig
> index e96089499371..0d526c004e81 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -287,6 +287,21 @@ config DECOMPRESS_LZ4
>  config GENERIC_ALLOCATOR
> bool
>
> +config GENERIC_ALLOCATOR_SELFTEST

Like the other lib/test_*.c targets, I'd call this TEST_GENERIC_ALLOCATOR.

> +   bool "genalloc tester"
> +   default n
> +   select GENERIC_ALLOCATOR
> +   help
> + Enable automated testing of the generic allocator.
> + The testing is primarily for the tracking of allocated space.
> +
> +config GENERIC_ALLOCATOR_SELFTEST_VERBOSE
> +   bool "make the genalloc tester more verbose"
> +   default n
> +   select GENERIC_ALLOCATOR_SELFTEST
> +   help
> + More information will be displayed during the self-testing.
> +
>  #
>  # reed solomon support is select'ed if needed
>  #
> diff --git a/lib/Makefile b/lib/Makefile
> index a90d4fcd748f..fadb30abde08 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -108,6 +108,7 @@ obj-$(CONFIG_LIBCRC32C) += libcrc32c.o
>  obj-$(CONFIG_CRC8) += crc8.o
>  obj-$(CONFIG_XXHASH)   += xxhash.o
>  obj-$(CONFIG_GENERIC_ALLOCATOR) += genalloc.o
> +obj-$(CONFIG_GENERIC_ALLOCATOR_SELFTEST) += genalloc-selftest.o
>
>  obj-$(CONFIG_842_COMPRESS) += 842/
>  obj-$(CONFIG_842_DECOMPRESS) += 842/
> diff --git a/lib/genalloc-selftest.c b/lib/genalloc-selftest.c
> new file mode 100644
> index ..e86be22c5512
> --- /dev/null
> +++ b/lib/genalloc-selftest.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * genalloc-selftest.c
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +/*
> + * Keep the bitmap small, while including case of cross-ulong mapping.
> + * For simplicity, the test cases use only 1 chunk of memory.
> + */
> +#define BITMAP_SIZE_C 16
> +#define ALLOC_ORDER 0
> +
> +#define ULONG_SIZE (sizeof(unsigned long))
> +#define BITMAP_SIZE_UL (BITMAP_SIZE_C / ULONG_SIZE)
> +#define MIN_ALLOC_SIZE (1 << ALLOC_ORDER)
> +#define ENTRIES (BITMAP_SIZE_C * 8)
> +#define CHUNK_SIZE  (MIN_ALLOC_SIZE * ENTRIES)
> +
> +#ifndef CONFIG_GENERIC_ALLOCATOR_SELFTEST_VERBOSE
> +
> +static inline void print_first_chunk_bitmap(struct gen_pool *pool) {}
> +
> +#else
> +
> +static void print_first_chunk_bitmap(struct gen_pool *pool)
> +{
> +   struct gen_pool_chunk *chunk;
> +   char bitmap[BITMAP_SIZE_C * 2 + 1];
> + 

Re: [PATCH 2/6] genalloc: selftest

[Kees Cook]

On Mon, Feb 12, 2018 at 8:52 AM, Igor Stoppa  wrote:
> Introduce a set of macros for writing concise test cases for genalloc.
>
> The test cases are meant to provide regression testing, when working on
> new functionality for genalloc.
>
> Primarily they are meant to confirm that the various allocation strategy
> will continue to work as expected.
>
> The execution of the self testing is controlled through a Kconfig option.
>
> Signed-off-by: Igor Stoppa 
> ---
>  include/linux/genalloc-selftest.h |  26 +++
>  init/main.c   |   2 +
>  lib/Kconfig   |  15 ++
>  lib/Makefile  |   1 +
>  lib/genalloc-selftest.c   | 400 
> ++

Nit: make this test_genalloc.c instead.

>  5 files changed, 444 insertions(+)
>  create mode 100644 include/linux/genalloc-selftest.h
>  create mode 100644 lib/genalloc-selftest.c
>
> diff --git a/include/linux/genalloc-selftest.h 
> b/include/linux/genalloc-selftest.h
> new file mode 100644
> index ..e0ac8f963abc
> --- /dev/null
> +++ b/include/linux/genalloc-selftest.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * genalloc-selftest.h
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + */
> +
> +
> +#ifndef __LINUX_GENALLOC_SELFTEST_H
> +#define __LINUX_GENALLOC_SELFTEST_H
> +
> +
> +#ifdef CONFIG_GENERIC_ALLOCATOR_SELFTEST
> +
> +#include 
> +
> +void genalloc_selftest(void);
> +
> +#else
> +
> +static inline void genalloc_selftest(void){};
> +
> +#endif
> +
> +#endif
> diff --git a/init/main.c b/init/main.c
> index a8100b954839..fb844aa3eb8c 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -89,6 +89,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -660,6 +661,7 @@ asmlinkage __visible void __init start_kernel(void)
>  */
> mem_encrypt_init();
>
> +   genalloc_selftest();

I wonder if it's possible to make this module-loadable instead? That
way it could be built and tested separately. Regardless, I love
gaining more internal selftests, this is great. :)

>  #ifdef CONFIG_BLK_DEV_INITRD
> if (initrd_start && !initrd_below_start_ok &&
> page_to_pfn(virt_to_page((void *)initrd_start)) < min_low_pfn) {
> diff --git a/lib/Kconfig b/lib/Kconfig
> index e96089499371..0d526c004e81 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -287,6 +287,21 @@ config DECOMPRESS_LZ4
>  config GENERIC_ALLOCATOR
> bool
>
> +config GENERIC_ALLOCATOR_SELFTEST

Like the other lib/test_*.c targets, I'd call this TEST_GENERIC_ALLOCATOR.

> +   bool "genalloc tester"
> +   default n
> +   select GENERIC_ALLOCATOR
> +   help
> + Enable automated testing of the generic allocator.
> + The testing is primarily for the tracking of allocated space.
> +
> +config GENERIC_ALLOCATOR_SELFTEST_VERBOSE
> +   bool "make the genalloc tester more verbose"
> +   default n
> +   select GENERIC_ALLOCATOR_SELFTEST
> +   help
> + More information will be displayed during the self-testing.
> +
>  #
>  # reed solomon support is select'ed if needed
>  #
> diff --git a/lib/Makefile b/lib/Makefile
> index a90d4fcd748f..fadb30abde08 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -108,6 +108,7 @@ obj-$(CONFIG_LIBCRC32C) += libcrc32c.o
>  obj-$(CONFIG_CRC8) += crc8.o
>  obj-$(CONFIG_XXHASH)   += xxhash.o
>  obj-$(CONFIG_GENERIC_ALLOCATOR) += genalloc.o
> +obj-$(CONFIG_GENERIC_ALLOCATOR_SELFTEST) += genalloc-selftest.o
>
>  obj-$(CONFIG_842_COMPRESS) += 842/
>  obj-$(CONFIG_842_DECOMPRESS) += 842/
> diff --git a/lib/genalloc-selftest.c b/lib/genalloc-selftest.c
> new file mode 100644
> index ..e86be22c5512
> --- /dev/null
> +++ b/lib/genalloc-selftest.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * genalloc-selftest.c
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +/*
> + * Keep the bitmap small, while including case of cross-ulong mapping.
> + * For simplicity, the test cases use only 1 chunk of memory.
> + */
> +#define BITMAP_SIZE_C 16
> +#define ALLOC_ORDER 0
> +
> +#define ULONG_SIZE (sizeof(unsigned long))
> +#define BITMAP_SIZE_UL (BITMAP_SIZE_C / ULONG_SIZE)
> +#define MIN_ALLOC_SIZE (1 << ALLOC_ORDER)
> +#define ENTRIES (BITMAP_SIZE_C * 8)
> +#define CHUNK_SIZE  (MIN_ALLOC_SIZE * ENTRIES)
> +
> +#ifndef CONFIG_GENERIC_ALLOCATOR_SELFTEST_VERBOSE
> +
> +static inline void print_first_chunk_bitmap(struct gen_pool *pool) {}
> +
> +#else
> +
> +static void print_first_chunk_bitmap(struct gen_pool *pool)
> +{
> +   struct gen_pool_chunk *chunk;
> +   char bitmap[BITMAP_SIZE_C * 2 + 1];
> +   unsigned long i;
> +   char *bm = bitmap;
> +   char *entry;
> +
> +   if 

Re: [PATCH 2/6] genalloc: selftest

[Matthew Wilcox]

On Sun, Feb 11, 2018 at 12:27:14PM -0800, Randy Dunlap wrote:
> On 02/11/18 12:22, Philippe Ombredanne wrote:
> > nit... For a comment in .h this line should be instead its own comment
> > as the first line:
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> Why are we treating header files (.h) differently than .c files?
> Either one can use the C++ "//" comment syntax.

This is now documented!

Documentation/process/license-rules.rst:

   If a specific tool cannot handle the standard comment style, then the
   appropriate comment mechanism which the tool accepts shall be used. This
   is the reason for having the "/\* \*/" style comment in C header
   files. There was build breakage observed with generated .lds files where
   'ld' failed to parse the C++ comment. This has been fixed by now, but
   there are still older assembler tools which cannot handle C++ style
   comments.

Personally, I find this disappointing.  I find this:

// SPDX-License-Identifier: GPL-2.0+
/*
 * XArray implementation
 * Copyright (c) 2017 Microsoft Corporation
 * Author: Matthew Wilcox 
 */

much less visually appealling than

/*
 * XArray implementation
 * Copyright (c) 2017 Microsoft Corporation
 * Author: Matthew Wilcox 
 * SPDX-License-Identifier: GPL-2.0+
 */

I can't see this variation making a tag extraction tool harder to write.

Re: [PATCH 2/6] genalloc: selftest

[Matthew Wilcox]

On Sun, Feb 11, 2018 at 12:27:14PM -0800, Randy Dunlap wrote:
> On 02/11/18 12:22, Philippe Ombredanne wrote:
> > nit... For a comment in .h this line should be instead its own comment
> > as the first line:
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> Why are we treating header files (.h) differently than .c files?
> Either one can use the C++ "//" comment syntax.

This is now documented!

Documentation/process/license-rules.rst:

   If a specific tool cannot handle the standard comment style, then the
   appropriate comment mechanism which the tool accepts shall be used. This
   is the reason for having the "/\* \*/" style comment in C header
   files. There was build breakage observed with generated .lds files where
   'ld' failed to parse the C++ comment. This has been fixed by now, but
   there are still older assembler tools which cannot handle C++ style
   comments.

Personally, I find this disappointing.  I find this:

// SPDX-License-Identifier: GPL-2.0+
/*
 * XArray implementation
 * Copyright (c) 2017 Microsoft Corporation
 * Author: Matthew Wilcox 
 */

much less visually appealling than

/*
 * XArray implementation
 * Copyright (c) 2017 Microsoft Corporation
 * Author: Matthew Wilcox 
 * SPDX-License-Identifier: GPL-2.0+
 */

I can't see this variation making a tag extraction tool harder to write.

Re: [PATCH 2/6] genalloc: selftest

[Randy Dunlap]

On 02/11/18 12:22, Philippe Ombredanne wrote:
> On Sun, Feb 11, 2018 at 4:19 AM, Igor Stoppa  wrote:
>> Introduce a set of macros for writing concise test cases for genalloc.
>>
>> The test cases are meant to provide regression testing, when working on
>> new functionality for genalloc.
>>
>> Primarily they are meant to confirm that the various allocation strategy
>> will continue to work as expected.
>>
>> The execution of the self testing is controlled through a Kconfig option.
>>
>> Signed-off-by: Igor Stoppa 
> 
> 
> 
>> --- /dev/null
>> +++ b/include/linux/genalloc-selftest.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0
> 
> nit... For a comment in .h this line should be instead its own comment
> as the first line:
>> +/* SPDX-License-Identifier: GPL-2.0 */

Why are we treating header files (.h) differently than .c files?
Either one can use the C++ "//" comment syntax.

> 
> 
>> --- /dev/null
>> +++ b/lib/genalloc-selftest.c
>> @@ -0,0 +1,400 @@
>> +/* SPDX-License-Identifier: GPL-2.0
> 
> And for a comment in .c this line should use C++ style as the first line:
> 
>> +// SPDX-License-Identifier: GPL-2.0
> 
> Please check the docs for this (I know this can feel surprising but
> this has been debated at great length on list)
> 
> Thank you!
> 


-- 
~Randy

Re: [PATCH 2/6] genalloc: selftest

[Randy Dunlap]

On 02/11/18 12:22, Philippe Ombredanne wrote:
> On Sun, Feb 11, 2018 at 4:19 AM, Igor Stoppa  wrote:
>> Introduce a set of macros for writing concise test cases for genalloc.
>>
>> The test cases are meant to provide regression testing, when working on
>> new functionality for genalloc.
>>
>> Primarily they are meant to confirm that the various allocation strategy
>> will continue to work as expected.
>>
>> The execution of the self testing is controlled through a Kconfig option.
>>
>> Signed-off-by: Igor Stoppa 
> 
> 
> 
>> --- /dev/null
>> +++ b/include/linux/genalloc-selftest.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: GPL-2.0
> 
> nit... For a comment in .h this line should be instead its own comment
> as the first line:
>> +/* SPDX-License-Identifier: GPL-2.0 */

Why are we treating header files (.h) differently than .c files?
Either one can use the C++ "//" comment syntax.

> 
> 
>> --- /dev/null
>> +++ b/lib/genalloc-selftest.c
>> @@ -0,0 +1,400 @@
>> +/* SPDX-License-Identifier: GPL-2.0
> 
> And for a comment in .c this line should use C++ style as the first line:
> 
>> +// SPDX-License-Identifier: GPL-2.0
> 
> Please check the docs for this (I know this can feel surprising but
> this has been debated at great length on list)
> 
> Thank you!
> 


-- 
~Randy

Re: [PATCH 2/6] genalloc: selftest

[Philippe Ombredanne]

On Sun, Feb 11, 2018 at 4:19 AM, Igor Stoppa  wrote:
> Introduce a set of macros for writing concise test cases for genalloc.
>
> The test cases are meant to provide regression testing, when working on
> new functionality for genalloc.
>
> Primarily they are meant to confirm that the various allocation strategy
> will continue to work as expected.
>
> The execution of the self testing is controlled through a Kconfig option.
>
> Signed-off-by: Igor Stoppa 



> --- /dev/null
> +++ b/include/linux/genalloc-selftest.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0

nit... For a comment in .h this line should be instead its own comment
as the first line:
> +/* SPDX-License-Identifier: GPL-2.0 */



> --- /dev/null
> +++ b/lib/genalloc-selftest.c
> @@ -0,0 +1,400 @@
> +/* SPDX-License-Identifier: GPL-2.0

And for a comment in .c this line should use C++ style as the first line:

> +// SPDX-License-Identifier: GPL-2.0

Please check the docs for this (I know this can feel surprising but
this has been debated at great length on list)

Thank you!
-- 
Cordially
Philippe Ombredanne

Re: [PATCH 2/6] genalloc: selftest

[Philippe Ombredanne]

On Sun, Feb 11, 2018 at 4:19 AM, Igor Stoppa  wrote:
> Introduce a set of macros for writing concise test cases for genalloc.
>
> The test cases are meant to provide regression testing, when working on
> new functionality for genalloc.
>
> Primarily they are meant to confirm that the various allocation strategy
> will continue to work as expected.
>
> The execution of the self testing is controlled through a Kconfig option.
>
> Signed-off-by: Igor Stoppa 



> --- /dev/null
> +++ b/include/linux/genalloc-selftest.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0

nit... For a comment in .h this line should be instead its own comment
as the first line:
> +/* SPDX-License-Identifier: GPL-2.0 */



> --- /dev/null
> +++ b/lib/genalloc-selftest.c
> @@ -0,0 +1,400 @@
> +/* SPDX-License-Identifier: GPL-2.0

And for a comment in .c this line should use C++ style as the first line:

> +// SPDX-License-Identifier: GPL-2.0

Please check the docs for this (I know this can feel surprising but
this has been debated at great length on list)

Thank you!
-- 
Cordially
Philippe Ombredanne

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 07/02/18 22:25, kbuild test robot wrote:

[...]

>>> lib/genalloc-selftest.c:17:10: fatal error: asm/set_memory.h: No such file 
>>> or directory
> #include 

This header is unnecessary and will be removed.

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 07/02/18 22:25, kbuild test robot wrote:

[...]

>>> lib/genalloc-selftest.c:17:10: fatal error: asm/set_memory.h: No such file 
>>> or directory
> #include 

This header is unnecessary and will be removed.

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 05/02/18 00:19, Randy Dunlap wrote:
> On 02/04/2018 08:47 AM, Igor Stoppa wrote:

[...]

> Please use kernel multi-line comment style.

ok for all of them

[...]

>> +BUG_ON(!locations[action->location]);
>> +print_first_chunk_bitmap(pool);
>> +BUG_ON(compare_bitmaps(pool, action->pattern));
> 
> BUG_ON() seems harsh to me, but some of the other self-tests also do that.

I would expect that the test never fails, if one is not modifying
anything related to genalloc.

But if an error slips in during development of genalloc or anything
related (like the functions used to scan the bitmaps), I think it's
better to pull the handbrake immediately, because failure in tracking
correctly the memory allocation is likely to cause corruption and every
sort of mysterious weird errors.

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 05/02/18 00:19, Randy Dunlap wrote:
> On 02/04/2018 08:47 AM, Igor Stoppa wrote:

[...]

> Please use kernel multi-line comment style.

ok for all of them

[...]

>> +BUG_ON(!locations[action->location]);
>> +print_first_chunk_bitmap(pool);
>> +BUG_ON(compare_bitmaps(pool, action->pattern));
> 
> BUG_ON() seems harsh to me, but some of the other self-tests also do that.

I would expect that the test never fails, if one is not modifying
anything related to genalloc.

But if an error slips in during development of genalloc or anything
related (like the functions used to scan the bitmaps), I think it's
better to pull the handbrake immediately, because failure in tracking
correctly the memory allocation is likely to cause corruption and every
sort of mysterious weird errors.

--
igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 05/02/18 02:14, Randy Dunlap wrote:
> On 02/04/2018 03:03 PM, Matthew Wilcox wrote:
>> On Sun, Feb 04, 2018 at 02:19:22PM -0800, Randy Dunlap wrote:
 +#ifndef __GENALLOC_SELFTEST_H__
 +#define __GENALLOC_SELFTEST_H__
>>>
>>> Please use _LINUX_GENALLOC_SELFTEST_H_
>>
>> willy@bobo:~/kernel/linux$ git grep define.*_H__$ include/linux/*.h |wc -l
>> 98
>> willy@bobo:~/kernel/linux$ git grep define.*_H_$ include/linux/*.h |wc -l
>> 110
>> willy@bobo:~/kernel/linux$ git grep define.*_H$ include/linux/*.h |wc -l
>> 885
>>
>> No trailing underscore is 8x as common as one trailing underscore.
> 
> OK, ack.

ok, I'll move to _LINUX_xxx_yyy_H format

--
thanks, igor

Re: [PATCH 2/6] genalloc: selftest

[Igor Stoppa]

On 05/02/18 02:14, Randy Dunlap wrote:
> On 02/04/2018 03:03 PM, Matthew Wilcox wrote:
>> On Sun, Feb 04, 2018 at 02:19:22PM -0800, Randy Dunlap wrote:
 +#ifndef __GENALLOC_SELFTEST_H__
 +#define __GENALLOC_SELFTEST_H__
>>>
>>> Please use _LINUX_GENALLOC_SELFTEST_H_
>>
>> willy@bobo:~/kernel/linux$ git grep define.*_H__$ include/linux/*.h |wc -l
>> 98
>> willy@bobo:~/kernel/linux$ git grep define.*_H_$ include/linux/*.h |wc -l
>> 110
>> willy@bobo:~/kernel/linux$ git grep define.*_H$ include/linux/*.h |wc -l
>> 885
>>
>> No trailing underscore is 8x as common as one trailing underscore.
> 
> OK, ack.

ok, I'll move to _LINUX_xxx_yyy_H format

--
thanks, igor

Re: [PATCH 2/6] genalloc: selftest

[kbuild test robot]

Hi Igor,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on v4.15 next-20180207]
[cannot apply to linus/master mmotm/master]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Igor-Stoppa/mm-security-ro-protection-for-dynamic-data/20180207-171252
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git 
for-next/pstore
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

>> lib/genalloc-selftest.c:17:10: fatal error: asm/set_memory.h: No such file 
>> or directory
#include 
 ^~
   compilation terminated.

vim +17 lib/genalloc-selftest.c

  > 17  #include 
18  #include 
19  #include 
20  #include 
21  #include 
22  
23  
24  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 2/6] genalloc: selftest

[kbuild test robot]

Hi Igor,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on kees/for-next/pstore]
[also build test ERROR on v4.15 next-20180207]
[cannot apply to linus/master mmotm/master]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Igor-Stoppa/mm-security-ro-protection-for-dynamic-data/20180207-171252
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git 
for-next/pstore
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

>> lib/genalloc-selftest.c:17:10: fatal error: asm/set_memory.h: No such file 
>> or directory
#include 
 ^~
   compilation terminated.

vim +17 lib/genalloc-selftest.c

  > 17  #include 
18  #include 
19  #include 
20  #include 
21  #include 
22  
23  
24  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 2/6] genalloc: selftest

[Randy Dunlap]

On 02/04/2018 03:03 PM, Matthew Wilcox wrote:
> On Sun, Feb 04, 2018 at 02:19:22PM -0800, Randy Dunlap wrote:
>>> +#ifndef __GENALLOC_SELFTEST_H__
>>> +#define __GENALLOC_SELFTEST_H__
>>
>> Please use _LINUX_GENALLOC_SELFTEST_H_
> 
> willy@bobo:~/kernel/linux$ git grep define.*_H__$ include/linux/*.h |wc -l
> 98
> willy@bobo:~/kernel/linux$ git grep define.*_H_$ include/linux/*.h |wc -l
> 110
> willy@bobo:~/kernel/linux$ git grep define.*_H$ include/linux/*.h |wc -l
> 885
> 
> No trailing underscore is 8x as common as one trailing underscore.

OK, ack.

thanks,
-- 
~Randy

Re: [PATCH 2/6] genalloc: selftest

[Randy Dunlap]

On 02/04/2018 03:03 PM, Matthew Wilcox wrote:
> On Sun, Feb 04, 2018 at 02:19:22PM -0800, Randy Dunlap wrote:
>>> +#ifndef __GENALLOC_SELFTEST_H__
>>> +#define __GENALLOC_SELFTEST_H__
>>
>> Please use _LINUX_GENALLOC_SELFTEST_H_
> 
> willy@bobo:~/kernel/linux$ git grep define.*_H__$ include/linux/*.h |wc -l
> 98
> willy@bobo:~/kernel/linux$ git grep define.*_H_$ include/linux/*.h |wc -l
> 110
> willy@bobo:~/kernel/linux$ git grep define.*_H$ include/linux/*.h |wc -l
> 885
> 
> No trailing underscore is 8x as common as one trailing underscore.

OK, ack.

thanks,
-- 
~Randy

Re: [PATCH 2/6] genalloc: selftest

[Matthew Wilcox]

On Sun, Feb 04, 2018 at 02:19:22PM -0800, Randy Dunlap wrote:
> > +#ifndef __GENALLOC_SELFTEST_H__
> > +#define __GENALLOC_SELFTEST_H__
> 
> Please use _LINUX_GENALLOC_SELFTEST_H_

willy@bobo:~/kernel/linux$ git grep define.*_H__$ include/linux/*.h |wc -l
98
willy@bobo:~/kernel/linux$ git grep define.*_H_$ include/linux/*.h |wc -l
110
willy@bobo:~/kernel/linux$ git grep define.*_H$ include/linux/*.h |wc -l
885

No trailing underscore is 8x as common as one trailing underscore.

Re: [PATCH 2/6] genalloc: selftest

[Matthew Wilcox]

On Sun, Feb 04, 2018 at 02:19:22PM -0800, Randy Dunlap wrote:
> > +#ifndef __GENALLOC_SELFTEST_H__
> > +#define __GENALLOC_SELFTEST_H__
> 
> Please use _LINUX_GENALLOC_SELFTEST_H_

willy@bobo:~/kernel/linux$ git grep define.*_H__$ include/linux/*.h |wc -l
98
willy@bobo:~/kernel/linux$ git grep define.*_H_$ include/linux/*.h |wc -l
110
willy@bobo:~/kernel/linux$ git grep define.*_H$ include/linux/*.h |wc -l
885

No trailing underscore is 8x as common as one trailing underscore.

Re: [PATCH 2/6] genalloc: selftest

[Randy Dunlap]

On 02/04/2018 08:47 AM, Igor Stoppa wrote:
> Introduce a set of macros for writing concise test cases for genalloc.
> 
> The test cases are meant to provide regression testing, when working on
> new functionality for genalloc.
> 
> Primarily they are meant to confirm that the various allocation strategy
> will continue to work as expected.
> 
> The execution of the self testing is controlled through a Kconfig option.
> 
> Signed-off-by: Igor Stoppa 
> ---
>  include/linux/genalloc-selftest.h |  30 +++
>  init/main.c   |   2 +
>  lib/Kconfig   |  15 ++
>  lib/Makefile  |   1 +
>  lib/genalloc-selftest.c   | 402 
> ++
>  5 files changed, 450 insertions(+)
>  create mode 100644 include/linux/genalloc-selftest.h
>  create mode 100644 lib/genalloc-selftest.c
> 
> diff --git a/include/linux/genalloc-selftest.h 
> b/include/linux/genalloc-selftest.h
> new file mode 100644
> index ..7af1901e57dc
> --- /dev/null
> +++ b/include/linux/genalloc-selftest.h
> @@ -0,0 +1,30 @@
> +/*
> + * genalloc-selftest.h
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +
> +#ifndef __GENALLOC_SELFTEST_H__
> +#define __GENALLOC_SELFTEST_H__

Please use _LINUX_GENALLOC_SELFTEST_H_

> +
> +
> +#ifdef CONFIG_GENERIC_ALLOCATOR_SELFTEST
> +
> +#include 
> +
> +void genalloc_selftest(void);
> +
> +#else
> +
> +static inline void genalloc_selftest(void){};
> +
> +#endif
> +
> +#endif


> diff --git a/lib/genalloc-selftest.c b/lib/genalloc-selftest.c
> new file mode 100644
> index ..007a0cfb3d77
> --- /dev/null
> +++ b/lib/genalloc-selftest.c
> @@ -0,0 +1,402 @@
> +/*
> + * genalloc-selftest.c
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +
> +
> +/* Keep the bitmap small, while including case of cross-ulong mapping.
> + * For simplicity, the test cases use only 1 chunk of memory.
> + */
> +#define BITMAP_SIZE_C 16
> +#define ALLOC_ORDER 0
> +
> +#define ULONG_SIZE (sizeof(unsigned long))
> +#define BITMAP_SIZE_UL (BITMAP_SIZE_C / ULONG_SIZE)
> +#define MIN_ALLOC_SIZE (1 << ALLOC_ORDER)
> +#define ENTRIES (BITMAP_SIZE_C * 8)
> +#define CHUNK_SIZE  (MIN_ALLOC_SIZE * ENTRIES)
> +
> +#ifndef CONFIG_GENERIC_ALLOCATOR_SELFTEST_VERBOSE
> +
> +static inline void print_first_chunk_bitmap(struct gen_pool *pool) {}
> +
> +#else
> +
> +static void print_first_chunk_bitmap(struct gen_pool *pool)
> +{
> + struct gen_pool_chunk *chunk;
> + char bitmap[BITMAP_SIZE_C * 2 + 1];
> + unsigned long i;
> + char *bm = bitmap;
> + char *entry;
> +
> + if (unlikely(pool == NULL || pool->chunks.next == NULL))
> + return;
> +
> + chunk = container_of(pool->chunks.next, struct gen_pool_chunk,
> +  next_chunk);
> + entry = (void *)chunk->entries;
> + for (i = 1; i <= BITMAP_SIZE_C; i++)
> + bm += snprintf(bm, 3, "%02hhx", entry[BITMAP_SIZE_C - i]);
> + *bm = '\0';
> + pr_notice("chunk: %pbitmap: 0x%s\n", chunk, bitmap);
> +
> +}
> +
> +#endif
> +
> +enum test_commands {
> + CMD_ALLOCATOR,
> + CMD_ALLOCATE,
> + CMD_FLUSH,
> + CMD_FREE,
> + CMD_NUMBER,
> + CMD_END = CMD_NUMBER,
> +};
> +
> +struct null_struct {
> + void *null;
> +};
> +
> +struct test_allocator {
> + genpool_algo_t algo;
> + union {
> + struct genpool_data_align align;
> + struct genpool_data_fixed offset;
> + struct null_struct null;
> + } data;
> +};
> +
> +struct test_action {
> + unsigned int location;
> + char pattern[BITMAP_SIZE_C];
> + unsigned int size;
> +};
> +
> +
> +struct test_command {
> + enum test_commands command;
> + union {
> + struct test_allocator allocator;
> + struct test_action action;
> + };
> +};
> +
> +
> +/* To pass an array literal as parameter to a macro, it must go through
> + * this one, first.
> + */

Please use kernel multi-line comment style.

> +#define ARR(...) __VA_ARGS__
> +
> +#define SET_DATA(parameter, value)   \
> + .parameter = {  \
> + .parameter = value, \
> + }   \
> +
> +#define SET_ALLOCATOR(alloc, parameter, value) 

Re: [PATCH 2/6] genalloc: selftest

[Randy Dunlap]

On 02/04/2018 08:47 AM, Igor Stoppa wrote:
> Introduce a set of macros for writing concise test cases for genalloc.
> 
> The test cases are meant to provide regression testing, when working on
> new functionality for genalloc.
> 
> Primarily they are meant to confirm that the various allocation strategy
> will continue to work as expected.
> 
> The execution of the self testing is controlled through a Kconfig option.
> 
> Signed-off-by: Igor Stoppa 
> ---
>  include/linux/genalloc-selftest.h |  30 +++
>  init/main.c   |   2 +
>  lib/Kconfig   |  15 ++
>  lib/Makefile  |   1 +
>  lib/genalloc-selftest.c   | 402 
> ++
>  5 files changed, 450 insertions(+)
>  create mode 100644 include/linux/genalloc-selftest.h
>  create mode 100644 lib/genalloc-selftest.c
> 
> diff --git a/include/linux/genalloc-selftest.h 
> b/include/linux/genalloc-selftest.h
> new file mode 100644
> index ..7af1901e57dc
> --- /dev/null
> +++ b/include/linux/genalloc-selftest.h
> @@ -0,0 +1,30 @@
> +/*
> + * genalloc-selftest.h
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +
> +#ifndef __GENALLOC_SELFTEST_H__
> +#define __GENALLOC_SELFTEST_H__

Please use _LINUX_GENALLOC_SELFTEST_H_

> +
> +
> +#ifdef CONFIG_GENERIC_ALLOCATOR_SELFTEST
> +
> +#include 
> +
> +void genalloc_selftest(void);
> +
> +#else
> +
> +static inline void genalloc_selftest(void){};
> +
> +#endif
> +
> +#endif


> diff --git a/lib/genalloc-selftest.c b/lib/genalloc-selftest.c
> new file mode 100644
> index ..007a0cfb3d77
> --- /dev/null
> +++ b/lib/genalloc-selftest.c
> @@ -0,0 +1,402 @@
> +/*
> + * genalloc-selftest.c
> + *
> + * (C) Copyright 2017 Huawei Technologies Co. Ltd.
> + * Author: Igor Stoppa 
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +
> +
> +/* Keep the bitmap small, while including case of cross-ulong mapping.
> + * For simplicity, the test cases use only 1 chunk of memory.
> + */
> +#define BITMAP_SIZE_C 16
> +#define ALLOC_ORDER 0
> +
> +#define ULONG_SIZE (sizeof(unsigned long))
> +#define BITMAP_SIZE_UL (BITMAP_SIZE_C / ULONG_SIZE)
> +#define MIN_ALLOC_SIZE (1 << ALLOC_ORDER)
> +#define ENTRIES (BITMAP_SIZE_C * 8)
> +#define CHUNK_SIZE  (MIN_ALLOC_SIZE * ENTRIES)
> +
> +#ifndef CONFIG_GENERIC_ALLOCATOR_SELFTEST_VERBOSE
> +
> +static inline void print_first_chunk_bitmap(struct gen_pool *pool) {}
> +
> +#else
> +
> +static void print_first_chunk_bitmap(struct gen_pool *pool)
> +{
> + struct gen_pool_chunk *chunk;
> + char bitmap[BITMAP_SIZE_C * 2 + 1];
> + unsigned long i;
> + char *bm = bitmap;
> + char *entry;
> +
> + if (unlikely(pool == NULL || pool->chunks.next == NULL))
> + return;
> +
> + chunk = container_of(pool->chunks.next, struct gen_pool_chunk,
> +  next_chunk);
> + entry = (void *)chunk->entries;
> + for (i = 1; i <= BITMAP_SIZE_C; i++)
> + bm += snprintf(bm, 3, "%02hhx", entry[BITMAP_SIZE_C - i]);
> + *bm = '\0';
> + pr_notice("chunk: %pbitmap: 0x%s\n", chunk, bitmap);
> +
> +}
> +
> +#endif
> +
> +enum test_commands {
> + CMD_ALLOCATOR,
> + CMD_ALLOCATE,
> + CMD_FLUSH,
> + CMD_FREE,
> + CMD_NUMBER,
> + CMD_END = CMD_NUMBER,
> +};
> +
> +struct null_struct {
> + void *null;
> +};
> +
> +struct test_allocator {
> + genpool_algo_t algo;
> + union {
> + struct genpool_data_align align;
> + struct genpool_data_fixed offset;
> + struct null_struct null;
> + } data;
> +};
> +
> +struct test_action {
> + unsigned int location;
> + char pattern[BITMAP_SIZE_C];
> + unsigned int size;
> +};
> +
> +
> +struct test_command {
> + enum test_commands command;
> + union {
> + struct test_allocator allocator;
> + struct test_action action;
> + };
> +};
> +
> +
> +/* To pass an array literal as parameter to a macro, it must go through
> + * this one, first.
> + */

Please use kernel multi-line comment style.

> +#define ARR(...) __VA_ARGS__
> +
> +#define SET_DATA(parameter, value)   \
> + .parameter = {  \
> + .parameter = value, \
> + }   \
> +
> +#define SET_ALLOCATOR(alloc, parameter, value)   \
> +{