Re: [PATCH v3 3/5] memblock tests: update style of comments for memblock_remove_*() functions

From: David Hildenbrand <david@redhat.com>
To: Rebecca Mckeever <remckee0@gmail.com>, outreachy@lists.linux.dev
Cc: Mike Rapoport <rppt@kernel.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 3/5] memblock tests: update style of comments for memblock_remove_*() functions
Date: Mon, 25 Apr 2022 15:45:02 +0200	[thread overview]
Message-ID: <a72036bf-0708-c1bc-2511-64513a130271@redhat.com> (raw)
In-Reply-To: <3b83322c070bdd43a33c336d135f485244f107d3.1650633253.git.remckee0@gmail.com>

On 22.04.22 15:33, Rebecca Mckeever wrote:
> Update comments in memblock_remove_*() functions to match the style used
> in tests/alloc_*.c by rewording to make the expected outcome more apparent
> and, if more than one memblock is involved, adding a visual of the
> memory blocks.
> 
> If the comment has an extra column of spaces, remove the extra space at
> the beginning of each line for consistency and to conform to Linux kernel
> coding style.
> 
> Signed-off-by: Rebecca Mckeever <remckee0@gmail.com>
> ---
>  tools/testing/memblock/tests/basic_api.c | 101 +++++++++++++++++------
>  1 file changed, 75 insertions(+), 26 deletions(-)
> 
> diff --git a/tools/testing/memblock/tests/basic_api.c b/tools/testing/memblock/tests/basic_api.c
> index 75cd7479ee54..08847dc5065e 100644
> --- a/tools/testing/memblock/tests/basic_api.c
> +++ b/tools/testing/memblock/tests/basic_api.c
> @@ -550,14 +550,21 @@ static int memblock_reserve_checks(void)
>  	return 0;
>  }
>  
> - /*
> -  * A simple test that tries to remove the first entry of the array of
> -  * available memory regions. By "removing" a region we mean overwriting it
> -  * with the next region in memblock.memory. To check this is the case, the
> -  * test adds two memory blocks and verifies that the value of the latter
> -  * was used to erase r1 region.  It also checks if the region counter and
> -  * total size were updated to expected values.
> -  */
> +/*
> + * A simple test that tries to remove the first entry of the array of
> + * with the next region in memblock.memory:

I failed to parse this sentence "of the array of with".

> + *
> + *  |  ......          +----------------+  |
> + *  |  : r1 :          |       r2       |  |
> + *  +--+----+----------+----------------+--+
> + *                     ^
> + *                     |
> + *                     rgn.base
> + *
> + * Expect to add two memory blocks r1 and r2 and then remove r1 region
> + * so that r2 is the first available region. The region counter and
> + * total size are updated.
> + */
>  static int memblock_remove_simple_check(void)
>  {
>  	struct memblock_region *rgn;
> @@ -587,11 +594,22 @@ static int memblock_remove_simple_check(void)
>  	return 0;
>  }
>  
> - /*
> -  * A test that tries to remove a region that was not registered as available
> -  * memory (i.e. has no corresponding entry in memblock.memory). It verifies
> -  * that array, regions counter and total size were not modified.
> -  */
> +/*
> + * A test that tries to remove a region that was not registered as available
> + * memory (i.e. has no corresponding entry in memblock.memory):
> + *
> + *                     +----------------+
> + *                     |       r2       |
> + *                     +----------------+
> + *  |  +----+                              |
> + *  |  | r1 |                              |
> + *  +--+----+------------------------------+
> + *     ^
> + *     |
> + *     rgn.base
> + *
> + * Expect the array, regions counter and total size to not be modified.
> + */
>  static int memblock_remove_absent_check(void)
>  {
>  	struct memblock_region *rgn;
> @@ -622,10 +640,21 @@ static int memblock_remove_absent_check(void)
>  
>  /*
>   * A test that tries to remove a region which overlaps with the beginning of

Wonder if it makes sense to mention r2 here. ("a region r2 ...") -- to
be precise, to always reference r1 and r2 accordingly instead of e.g.,
"the first entry". Same applies to the other function documentation you
touch in this patch.

-- 
Thanks,

David / dhildenb