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

[Steven Rostedt <rostedt@goodmis.org>]

On Tue, 11 Jun 2024 14:40:29 +0000
Wei Yang <richard.weiyang@gmail.com> wrote:

Missed this just before sending out v3 :-p

> >diff --git a/mm/memblock.c b/mm/memblock.c
> >index d09136e040d3..a8bf0ee9e2b4 100644
> >--- a/mm/memblock.c
> >+++ b/mm/memblock.c
> >@@ -2244,6 +2244,103 @@ 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  
>                                         ^
> Suggest to align with previous line.

It is. But because the patch adds a "+", it pushed the "8" out another tab.

> 
> >+struct reserve_mem_table {
> >+	char			name[RESERVE_MEM_NAME_SIZE];
> >+	unsigned long		start;
> >+	unsigned long		size;  
> 
> phys_addr_t looks more precise?

For just the start variable, correct? I'm OK with updating that.

> 
> >+};
> >+static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
> >+static int reserved_mem_count;  
> 
> Seems no matter we use this feature or not, these memory would be occupied?

Yes, because allocation may screw it up as well. I could add a CONFIG
around it, so that those that do not want this could configure it out. But
since it's just a total of (16 + 8 + 8) * 8 = 256 bytes, I'm not sure it's
much of a worry to add the complexities to save that much space. As the
code to save it may likely be bigger.

> 
> >+
> >+/* Add wildcard region with a lookup name */
> >+static int __init reserved_mem_add(unsigned long start, unsigned long size,
> >+				   const char *name)
> >+{
> >+	struct reserve_mem_table *map;
> >+
> >+	if (!name || !name[0] || strlen(name) >= RESERVE_MEM_NAME_SIZE)
> >+		return -EINVAL;
> >+
> >+	if (reserved_mem_count >= RESERVE_MEM_MAX_ENTRIES)
> >+		return -1;  
> 
> return ENOSPC? Not good at it, but a raw value maybe not a good practice.

This is what gets returned by the command line parser. It only cares if it
is zero or not.

> 
> Also, we'd better do this check before allocation.

What allocation?

> 
> >+
> >+	map = &reserved_mem_table[reserved_mem_count++];
> >+	map->start = start;
> >+	map->size = size;
> >+	strscpy(map->name, name);
> >+	return 0;
> >+}
> >+
> >+/**
> >+ * reserve_mem_find_by_name - Find reserved memory region with a given name
> >+ * @name: The name that is attached to a reserved memory region
> >+ * @start: If found, holds the start address
> >+ * @size: If found, holds the size of the address.
> >+ *
> >+ * Returns: 1 if found or 0 if not found.
> >+ */
> >+int reserve_mem_find_by_name(const char *name, unsigned long *start, unsigned long *size)
> >+{
> >+	struct reserve_mem_table *map;
> >+	int i;
> >+
> >+	for (i = 0; i < reserved_mem_count; i++) {
> >+		map = &reserved_mem_table[i];
> >+		if (!map->size)
> >+			continue;
> >+		if (strcmp(name, map->name) == 0) {
> >+			*start = map->start;
> >+			*size = map->size;
> >+			return 1;
> >+		}
> >+	}
> >+	return 0;
> >+}
> >+
> >+/*
> >+ * Parse early_reserve_mem=nn:align:name  
> 
> early_reserve_mem or reserve_mem ?

Oops, that was the original name. I'll change that.

> 
> >+ */
> >+static int __init reserve_mem(char *p)
> >+{
> >+	phys_addr_t start, size, align;

Hmm, I wonder if I should change size and align to unsigned long?

> >+	char *oldp;
> >+	int err;
> >+
> >+	if (!p)
> >+		return -EINVAL;
> >+
> >+	oldp = p;
> >+	size = memparse(p, &p);
> >+	if (p == oldp)
> >+		return -EINVAL;
> >+
> >+	if (*p != ':')
> >+		return -EINVAL;
> >+
> >+	align = memparse(p+1, &p);
> >+	if (*p != ':')
> >+		return -EINVAL;
> >+  
> 
> Better to check if the name is valid here. 

You mean that it has text and is not blank?

> 
> Make sure command line parameters are valid before doing the allocation.

You mean that size is non zero?

I don't know if we care what the align is. Zero is valid.

> 
> >+	start = memblock_phys_alloc(size, align);
> >+	if (!start)
> >+		return -ENOMEM;
> >+
> >+	p++;
> >+	err = reserved_mem_add(start, size, p);
> >+	if (err) {
> >+		memblock_phys_free(start, size);
> >+		return err;
> >+	}
> >+
> >+	p += strlen(p);
> >+
> >+	return *p == '\0' ? 0: -EINVAL;  
> 
> We won't free the memory if return -EINVAL?

I guess we can do this check before the allocation, like you suggested.

Thanks for the review.

-- Steve


> 
> >+}
> >+__setup("reserve_mem=", reserve_mem);
> >+
> > #if defined(CONFIG_DEBUG_FS) && defined(CONFIG_ARCH_KEEP_MEMBLOCK)
> > static const char * const flagname[] = {
> > 	[ilog2(MEMBLOCK_HOTPLUG)] = "HOTPLUG",
> >-- 
> >2.43.0
> >
> >  
>