Re: [PATCH v4 1/2] mm/memblock: Add "reserve_mem" to reserved named memory at boot up

[Steven Rostedt]

On Tue, 11 Jun 2024 15:26:05 -0700
Guenter Roeck <linux@xxxxxxxxxxxx> wrote:

> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index d09136e040d3..044ddce8f085 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -2244,6 +2244,121 @@ void __init memblock_free_all(void)
> >   	totalram_pages_add(pages);
> >   }
> >   
> > +/* Keep a table to reserve named memory */
> > +#define RESERVE_MEM_MAX_ENTRIES		8
> > +#define RESERVE_MEM_NAME_SIZE		16
> > +struct reserve_mem_table {
> > +	char			name[RESERVE_MEM_NAME_SIZE];
> > +	phys_addr_t		start;
> > +	phys_addr_t		size;
> > +};
> > +static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
> > +static int reserved_mem_count;
> > +
> > +/* Add wildcard region with a lookup name */
> > +static int __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> > +				   const char *name)
> > +{
> > +	struct reserve_mem_table *map;
> > +
> > +	if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
> > +		return -EINVAL;
> > +  
> 
> I know I am picky, but name is never NULL, and strlen(name) is guaranteed to be > 0.
> Personally I'd suggest to check for strlen(name) >= RESERVE_MEM_NAME_SIZE together
> with !strlen(name) and drop the duplicate checks here (and, as side effect, avoid
> the pointless memory allocation if the name is invalid).

Yeah, it's now checked before hand. I'll remove it for v5.

> 
> > +	if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
> > +		return -1;
> > +
> > +	map = &reserved_mem_table[reserved_mem_count++];
> > +	map->start = start;
> > +	map->size = size;
> > +	strscpy(map->name, name);
> > +	return 0;
> > +}
> > +


> > +/*
> > + * Parse reserve_mem=nn:align:name
> > + */
> > +static int __init reserve_mem(char *p)
> > +{
> > +	phys_addr_t start, size, align;
> > +	char *name;
> > +	char *oldp;
> > +	int err;
> > +
> > +	if (!p)
> > +		return -EINVAL;
> > +
> > +	oldp = p;
> > +	size = memparse(p, &p);
> > +	if (!size || p == oldp)
> > +		return -EINVAL;
> > +
> > +	if (*p != ':')
> > +		return -EINVAL;
> > +
> > +	align = memparse(p+1, &p);
> > +	if (*p != ':')
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * memblock_phys_alloc() doesn't like a zero size align,
> > +	 * but it is OK for this command to have it.
> > +	 */
> > +	if (align <= SMP_CACHE_BYTES)  
> 
> Any reason for using <= instead of < ?

Nope. Not sure why I did that. :-/

I'll fix that too.

Thanks,

-- Steve

> 
> Guenter
> 
> > +		align = SMP_CACHE_BYTES;
> > +
> > +	name = p + 1;
> > +	if (!strlen(name))
> > +		return -EINVAL;
> > +
> > +	/* Make sure that name has text */
> > +	for (p = name; *p; p++) {
> > +		if (!isspace(*p))
> > +			break;
> > +	}
> > +	if (!*p)
> > +		return -EINVAL;
> > +
> > +	start = memblock_phys_alloc(size, align);
> > +	if (!start)
> > +		return -ENOMEM;
> > +
> > +	err = reserved_mem_add(start, size, name);
> > +	if (err) {
> > +		memblock_phys_free(start, size);
> > +		return err;
> > +	}
> > +
> > +	return 0;
> > +}
> > +__setup("reserve_mem=", reserve_mem);
> > +
> >   #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> >   static const char * const flagname[] = {
> >   	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",