Re: [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw

[David Hildenbrand]

On 25.08.22 23:35, Rebecca Mckeever wrote:
> On Tue, Aug 23, 2022 at 11:49:46AM +0200, David Hildenbrand wrote:
>> On 19.08.22 10:34, Rebecca Mckeever wrote:
>>> Update memblock_alloc() tests so that they test either memblock_alloc()
>>> or memblock_alloc_raw() depending on the value of alloc_test_flags. Run
>>> through all the existing tests in memblock_alloc_api twice: once for
>>> memblock_alloc() and once for memblock_alloc_raw().
>>>
>>> When the tests run memblock_alloc(), they test that the entire memory
>>> region is zero. When the tests run memblock_alloc_raw(), they test that
>>> the entire memory region is nonzero.
>>
>> Could add a comment stating that we initialize the content to nonzero in
>> that case, and expect it to remain unchanged (== not zeroed).
>>
>>>
>>> Signed-off-by: Rebecca Mckeever <remckee0@xxxxxxxxx>
>>> ---
>>>  tools/testing/memblock/tests/alloc_api.c | 98 ++++++++++++++++--------
>>>  tools/testing/memblock/tests/common.h    | 25 ++++++
>>>  2 files changed, 90 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/tools/testing/memblock/tests/alloc_api.c b/tools/testing/memblock/tests/alloc_api.c
>>> index 65bff77dd55b..cf67687ae044 100644
>>> --- a/tools/testing/memblock/tests/alloc_api.c
>>> +++ b/tools/testing/memblock/tests/alloc_api.c
>>> @@ -1,6 +1,29 @@
>>>  // SPDX-License-Identifier: GPL-2.0-or-later
>>>  #include "alloc_api.h"
>>>  
>>> +static const char * const func_testing[] = {
>>> +	"memblock_alloc",
>>> +	"memblock_alloc_raw"
>>> +};
>>> +
>>> +static int alloc_test_flags = TEST_ZEROED;
>>> +
>>> +static inline const char * const get_func_testing(int flags)
>>> +{
>>> +	if (flags & TEST_RAW)
>>> +		return func_testing[1];
>>> +	else
>>> +		return func_testing[0];
>>
>> No need for the else, you can return directly.
>>
>> Can we avoid the func_testing array?
>>
>>
>> Persoally, I consider the "get_func_testing()" name a bit confusing.
>>
>> get_memblock_alloc_name() ?
>>
>>
>>> diff --git a/tools/testing/memblock/tests/common.h b/tools/testing/memblock/tests/common.h
>>> index 58f84bf2c9ae..4fd3534ff955 100644
>>> --- a/tools/testing/memblock/tests/common.h
>>> +++ b/tools/testing/memblock/tests/common.h
>>> @@ -12,6 +12,11 @@
>>>  
>>>  #define MEM_SIZE SZ_16K
>>>  
>>> +enum test_flags {
>>> +	TEST_ZEROED = 0x0,
>>> +	TEST_RAW = 0x1
>>> +};
>>
>> I'd have called this
>>
>> enum test_flags {
>> 	/* No special request. */
>> 	TEST_F_NONE = 0x0,
>> 	/* Perform raw allocations (no zeroing of memory).
>> 	TEST_F_RAW = 0x1,
>> };
>>
>> Further, I'd just have use #define for the flags.
>>
> Do you mean use two #defines instead of the enum? I thought enums were
> preferred when defining related constants.

I guess we have a wild mixture of raw define, enums and __bitwise +
defines nowdays.

E.g., take a look at include/linux/rmap.h "typedef int __bitwise rmap_t"
and how it's used --  that seems to be the new "best" solution for use
in the kernel.

Having that said, feel free to just let it be an enum :)

-- 
Thanks,

David / dhildenb