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
