Re: [PATCH 03/19] mm: document grub internal memory management structures

[Glenn Washburn]

On Tue, 12 Oct 2021 18:29:52 +1100
Daniel Axtens <dja@axtens.net> wrote:

> I spent more than a trivial quantity of time figuring out pre_size and
> whether a memory region's size contains the header cell or not.
> 
> Document the meanings of all the properties. Hopefully now no-one else
> has to figure it out!
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
>  include/grub/mm_private.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/include/grub/mm_private.h b/include/grub/mm_private.h
> index c2c4cb1511c6..e80a059dd4e4 100644
> --- a/include/grub/mm_private.h
> +++ b/include/grub/mm_private.h
> @@ -25,11 +25,18 @@
>  #define GRUB_MM_FREE_MAGIC   0x2d3c2808
>  #define GRUB_MM_ALLOC_MAGIC  0x6db08fa4
>  
> +/* A header describing a block of memory - either allocated or free */
>  typedef struct grub_mm_header
>  {
> +  /* The 'next' free block in this region. A circular list.
> +     Only meaningful if the block is free.
> +   */
>    struct grub_mm_header *next;

This and the subsequent comments do not follow the multiline comment
style[1].

One nit, "region. A circular" -> "region's circular".

This comment leads me to wonder if the block is not free, what is the
value of next? Seems like it should be NULL, if its not meaningful.

https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Multi_002dLine-Comments

> +  /* The region size in cells (not bytes). Includes the header cell. */

What exactly does "cell" mean in this context? I'm gathering cell is
not defined as in this link[2], which is equivalent to "bit". Perhaps
"size" should be renamed to "ncells" or something more descriptive.

[2] https://en.wikipedia.org/wiki/Memory_cell_(computing)

>    grub_size_t size;
> +  /* either free or alloc magic, depending on the block type. */
>    grub_size_t magic;
> +  /* pad to cell size: see the top of mm.c. */
>  #if GRUB_CPU_SIZEOF_VOID_P == 4
>    char padding[4];
>  #elif GRUB_CPU_SIZEOF_VOID_P == 8
> @@ -48,11 +55,25 @@ typedef struct grub_mm_header
>  
>  #define GRUB_MM_ALIGN        (1 << GRUB_MM_ALIGN_LOG2)
>  
> +/* A region from which we can make allocations. */
>  typedef struct grub_mm_region
>  {
> +  /* The first free block in this region. */
>    struct grub_mm_header *first;
> +
> +  /* The next region in the linked list of regions. Regions are initially
> +     sorted in order of increasing size, but can grow, in which case the
> +     ordering may not be preserved.
> +   */
>    struct grub_mm_region *next;
> +
> +  /* A grub_mm_region will always be aligned to cell size. The pre-size is
> +     the number of bytes we were given but had to skip in order to get that
> +     alignment.
> +   */
>    grub_size_t pre_size;
> +
> +  /* How many bytes are in this region? (free and allocated) */
>    grub_size_t size;
>  }
>  *grub_mm_region_t;

Glenn