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

[Daniel Axtens]

Glenn Washburn <development@efficientek.com> writes:

> 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].

Will fix, thanks.

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

Sure, will do.

> 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.

In practice the value will be:

 - if the block was converted from an existing free block: whatever the
   'next' pointer was when the block was free

 - if the block was carved out of the middle or end of an existing free
   block: whatever happened to be at that offset in memory.

We certainly _could_ null it out. I don't think there's any real value
in doing so but I'm open to being convinced. I suppose you could make an
argument that it reduces the chance that we'll follow a pointer to
somewhere random in memory but we check the magic before we check next
so we're fairly safe.

> 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)

The definition of cell is from mm.c:

  The memory space is used as cells instead of bytes for simplicity. This
  is important for some CPUs which may not access multiple bytes at a time
  when the first byte is not aligned at a certain boundary (typically,
  4-byte or 8-byte). The size of each cell is equal to the size of struct
  grub_mm_header, so the header of each allocated/free block fits into one
  cell precisely. One cell is 16 bytes on 32-bit platforms and 32 bytes
  on 64-bit platforms.

I don't want to change variable names in this patch but I will update
the comment.

Kind regards,
Daniel