From: Rebecca Mckeever <remckee0@gmail.com>
To: David Hildenbrand <david@redhat.com>
Cc: Mike Rapoport <rppt@kernel.org>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw
Date: Thu, 25 Aug 2022 16:35:46 -0500 [thread overview]
Message-ID: <20220825213546.GA13624@sophie> (raw)
In-Reply-To: <d65cf9fe-e22c-7698-0313-879685f1319b@redhat.com>
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@gmail.com>
> > ---
> > 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.
> > +
> > /**
> > * ASSERT_EQ():
> > * Check the condition
> > @@ -63,6 +68,18 @@
> > } \
> > } while (0)
> >
> > +/**
> > + * ASSERT_MEM_NE():
> > + * Check that none of the first @_size bytes of @_seen are equal to @_expected.
> > + * If false, print failed test message (if running with --verbose) and then
> > + * assert.
> > + */
> > +#define ASSERT_MEM_NE(_seen, _expected, _size) do { \
> > + for (int _i = 0; _i < (_size); _i++) { \
> > + ASSERT_NE((_seen)[_i], (_expected)); \
> > + } \
> > +} while (0)
> > +
> > #define PREFIX_PUSH() prefix_push(__func__)
> >
> > /*
> > @@ -116,4 +133,12 @@ static inline void run_bottom_up(int (*func)())
> > prefix_pop();
> > }
> >
> > +static inline void verify_mem_content(void *mem, int size, int flags)
>
> nit: why use verify here when the other functions "assert". I'd have
> called this something like "assert_mem_content()"
>
>
> --
> Thanks,
>
> David / dhildenb
>
>
Thanks,
Rebecca
next prev parent reply other threads:[~2022-08-25 21:35 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-19 8:34 [PATCH v2 0/8] memblock tests: update and extend memblock simulator Rebecca Mckeever
2022-08-19 8:34 ` [PATCH v2 1/8] memblock tests: update tests to check if memblock_alloc zeroed memory Rebecca Mckeever
2022-08-23 9:36 ` David Hildenbrand
2022-08-23 13:25 ` Mike Rapoport
2022-08-19 8:34 ` [PATCH v2 2/8] memblock tests: update zeroed memory check for memblock_alloc_* tests Rebecca Mckeever
2022-08-19 8:34 ` [PATCH v2 3/8] memblock tests: add labels to verbose output for generic alloc tests Rebecca Mckeever
2022-08-23 9:37 ` David Hildenbrand
2022-08-19 8:34 ` [PATCH v2 4/8] memblock tests: add additional tests for basic api and memblock_alloc Rebecca Mckeever
2022-08-23 9:39 ` David Hildenbrand
2022-08-19 8:34 ` [PATCH v2 5/8] memblock tests: update alloc_api to test memblock_alloc_raw Rebecca Mckeever
2022-08-23 9:49 ` David Hildenbrand
2022-08-25 21:35 ` Rebecca Mckeever [this message]
2022-08-26 9:28 ` David Hildenbrand
2022-08-19 8:34 ` [PATCH v2 6/8] memblock tests: update alloc_nid_api to test memblock_alloc_try_nid_raw Rebecca Mckeever
2022-08-23 9:50 ` David Hildenbrand
2022-08-19 8:34 ` [PATCH v2 7/8] memblock tests: add tests for memblock_*bottom_up functions Rebecca Mckeever
2022-08-19 8:34 ` [PATCH v2 8/8] memblock tests: add tests for memblock_trim_memory Rebecca Mckeever
2022-08-23 9:54 ` David Hildenbrand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220825213546.GA13624@sophie \
--to=remckee0@gmail.com \
--cc=david@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=rppt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).