[RFC PATCH v2 0/3] tracing: Improve persistent ring buffer

[RFC PATCH v2 0/3] tracing: Improve persistent ring buffer

[Masami Hiramatsu (Google)]

Hi,

Here is the 2nd version of the patch and additional improvement for ring
buffer on reseved memory. This depends on the series of "ring-buffer/tracing:
Save module information in persistent memory" here;

https://lore.kernel.org/all/20250205225031.799739376@goodmis.org/

In v2, I removed my first 2 patches, and fixes the 3rd patch. It also add
module LRU override feature which replaces the oldest removed module if
the module table is full.

I also added 2 patches to release the memory of persistent ring buffer
if user does not need it anymore. Currently persistent ring buffer has
to be enabled by kernel cmdline, user can not remove it without
changing the cmdline and rebooting. This feature allows user to remove
persistent ring buffer and release the memory without rebooting. Thus
user can set it up by default and remove it if it is not used anymore.

Thank you,
---

Masami Hiramatsu (Google) (3):
      tracing: Show last module text symbols in the stacktrace
      mm/memblock: Add reserved memory release function
      tracing: Freeable reserved ring buffer


 include/linux/mm.h          |    1 
 kernel/trace/trace.c        |  190 ++++++++++++++++++++++++++++++++++++++++---
 kernel/trace/trace.h        |    5 +
 kernel/trace/trace_output.c |    3 -
 mm/memblock.c               |   72 ++++++++++++++--
 5 files changed, 244 insertions(+), 27 deletions(-)

--
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[RFC PATCH v2 1/3] tracing: Show last module text symbols in the stacktrace

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Since the previous boot trace buffer can include module text address in
the stacktrace. As same as the kernel text address, convert the module
text address using the module address information.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 Changes in v2:
  - Add LRU based removed module information override.
  - Fix to move module_delta initialization after tr->scratch check.
  - Fix to make mod_delta on all loaded modules correctly.
  - (Note, I don't add uname check yet)
  - Fix mod_addr_comp() overflow issue.
  - Add pr_info() when failed to allocate module_delta.
---
 kernel/trace/trace.c        |  173 ++++++++++++++++++++++++++++++++++++++++---
 kernel/trace/trace.h        |    4 +
 kernel/trace/trace_output.c |    3 -
 3 files changed, 167 insertions(+), 13 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 443f2bc5b856..dde8000a5614 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -49,6 +49,7 @@
 #include <linux/fsnotify.h>
 #include <linux/irq_work.h>
 #include <linux/workqueue.h>
+#include <linux/sort.h>
 
 #include <asm/setup.h> /* COMMAND_LINE_SIZE and kaslr_offset() */
 
@@ -6001,12 +6002,73 @@ struct trace_mod_entry {
 
 struct trace_scratch {
 	unsigned long		kaslr_addr;
-	unsigned long		nr_entries;
+	unsigned int		nr_entries;
+	int			oldest_removed;
 	struct trace_mod_entry	entries[];
 };
 
 static DEFINE_MUTEX(scratch_mutex);
 
+/* Encode LRU list at the lowest 8bits and use 9th bit as removed flag. */
+#define MOD_INDEX_MASK		GENMASK(7, 0)
+#define MOD_REMOVED_MASK	BIT(8)
+#define MOD_INDEX_LAST		MOD_INDEX_MASK
+
+static int decode_next_removed(struct trace_mod_entry *entry)
+{
+	if (WARN_ON_ONCE(!(entry->mod_addr & MOD_REMOVED_MASK)))
+		return -1;
+
+	if ((entry->mod_addr & MOD_INDEX_MASK) == MOD_INDEX_MASK)
+		return -1;
+
+	return (int)(entry->mod_addr & MOD_INDEX_MASK);
+}
+
+static int encode_last_removed(struct trace_scratch *tscratch, int last)
+{
+	struct trace_mod_entry *entry;
+	int idx = tscratch->oldest_removed;
+
+	if (WARN_ON_ONCE(last < 0 || last >= tscratch->nr_entries))
+		return -1;
+
+	if (idx < 0) {
+		tscratch->oldest_removed = last;
+	} else {
+		do {
+			entry = &tscratch->entries[idx];
+			idx = entry->mod_addr & MOD_INDEX_MASK;
+		} while (idx != MOD_INDEX_LAST);
+		entry->mod_addr &= ~MOD_INDEX_MASK;
+		entry->mod_addr |= last;
+	}
+	entry = &tscratch->entries[last];
+	entry->mod_addr |= MOD_REMOVED_MASK | MOD_INDEX_LAST;
+	return 0;
+}
+
+unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr)
+{
+	struct trace_scratch *tscratch;
+	int i;
+
+	/* If we don't have last boot delta, return the address */
+	if (!(tr->flags & TRACE_ARRAY_FL_LAST_BOOT))
+		return addr;
+
+	tscratch = tr->scratch;
+	if (tscratch && tr->module_delta && tscratch->entries[0].mod_addr < addr) {
+		/* Note that entries are sorted */
+		for (i = 0; i < tr->nr_modules; i++)
+			if (addr < tscratch->entries[i].mod_addr)
+				break;
+		return addr + tr->module_delta[i - 1];
+	}
+
+	return addr + tr->text_delta;
+}
+
 static int save_mod(struct module *mod, void *data)
 {
 	struct trace_array *tr = data;
@@ -6021,12 +6083,18 @@ static int save_mod(struct module *mod, void *data)
 		return -1;
 	size = tr->scratch_size;
 
-	if (struct_size(tscratch, entries, tscratch->nr_entries + 1) > size)
-		return -1;
+	if (struct_size(tscratch, entries, tscratch->nr_entries + 1) > size) {
+		int idx = tscratch->oldest_removed;
 
-	entry = &tscratch->entries[tscratch->nr_entries];
+		if (idx < 0)
+			return -1;
+		entry = &tscratch->entries[idx];
+		tscratch->oldest_removed = decode_next_removed(entry);
+	} else {
+		entry = &tscratch->entries[tscratch->nr_entries];
 
-	tscratch->nr_entries++;
+		tscratch->nr_entries++;
+	}
 
 	entry->mod_addr = (unsigned long)mod->mem[MOD_TEXT].base;
 	strscpy(entry->mod_name, mod->name);
@@ -6063,14 +6131,17 @@ static void update_last_data(struct trace_array *tr)
 	 */
 	tracing_reset_all_cpus(&tr->array_buffer);
 
-	/* Using current data now */
-	tr->text_delta = 0;
-
 	if (!tr->scratch)
 		return;
 
 	tscratch = tr->scratch;
 
+	/* Using current data now */
+	tr->text_delta = 0;
+	kfree(tr->module_delta);
+	tr->module_delta = NULL;
+	tr->nr_modules = 0;
+
 	/* Set the persistent ring buffer meta data to this address */
 #ifdef CONFIG_RANDOMIZE_BASE
 	tscratch->kaslr_addr = kaslr_offset();
@@ -9349,10 +9420,37 @@ static struct dentry *trace_instance_dir;
 static void
 init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer);
 
+static int make_mod_delta(struct module *mod, void *data)
+{
+	struct trace_scratch *tscratch;
+	struct trace_mod_entry *entry;
+	struct trace_array *tr = data;
+	int i;
+
+	tscratch = tr->scratch;
+	for (i = 0; i < tr->nr_modules; i++) {
+		entry = &tscratch->entries[i];
+		if (!strcmp(mod->name, entry->mod_name)) {
+			tr->module_delta[i] = (unsigned long)mod->mem[MOD_TEXT].base - entry->mod_addr;
+			break;
+		}
+	}
+	return 0;
+}
+
+static int mod_addr_comp(const void *a, const void *b, const void *data)
+{
+	const struct trace_mod_entry *e1 = a;
+	const struct trace_mod_entry *e2 = b;
+
+	return e1->mod_addr > e2->mod_addr ? 1 : -1;
+}
+
 static void setup_trace_scratch(struct trace_array *tr, void *scratch, unsigned int size)
 {
 	struct trace_scratch *tscratch = scratch;
 	struct trace_mod_entry *entry;
+	int i, nr_entries;
 
 	if (!scratch)
 		return;
@@ -9369,7 +9467,7 @@ static void setup_trace_scratch(struct trace_array *tr, void *scratch, unsigned
 		goto reset;
 
 	/* Check if each module name is a valid string */
-	for (int i = 0; i < tscratch->nr_entries; i++) {
+	for (i = 0; i < tscratch->nr_entries; i++) {
 		int n;
 
 		entry = &tscratch->entries[i];
@@ -9383,6 +9481,22 @@ static void setup_trace_scratch(struct trace_array *tr, void *scratch, unsigned
 		if (n == MODULE_NAME_LEN)
 			goto reset;
 	}
+	nr_entries = i;
+	/* Allocate module delta array */
+	tr->module_delta = kcalloc(nr_entries, sizeof(long), GFP_KERNEL);
+	if (!tr->module_delta) {
+		pr_info("module_delta allocation failed. Not able to decode module address.");
+		goto reset;
+	}
+	tr->nr_modules = nr_entries;
+
+	/* Sort module table by base address. */
+	sort_r(tscratch->entries, nr_entries, sizeof(struct trace_mod_entry),
+		mod_addr_comp, NULL, NULL);
+
+	/* Scan modules */
+	module_for_each_mod(make_mod_delta, tr);
+
 	return;
  reset:
 	/* Invalid trace modules */
@@ -10100,14 +10214,50 @@ static bool trace_array_active(struct trace_array *tr)
 static void trace_module_record(struct module *mod)
 {
 	struct trace_array *tr;
+	unsigned long flags;
 
 	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		flags = tr->flags & (TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT);
 		/* Update any persistent trace array that has already been started */
-		if ((tr->flags & (TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT)) ==
-		    TRACE_ARRAY_FL_BOOT) {
+		if (flags == TRACE_ARRAY_FL_BOOT) {
 			/* Only update if the trace array is active */
 			if (trace_array_active(tr))
 				save_mod(mod, tr);
+		} else if (flags & TRACE_ARRAY_FL_LAST_BOOT) {
+			/* Update delta if the module loaded in previous boot */
+			make_mod_delta(mod, tr);
+		}
+	}
+}
+
+static void mark_mod_removed(struct trace_array *tr, struct module *mod)
+{
+	struct trace_scratch *tscratch;
+	struct trace_mod_entry *entry;
+	int i;
+
+	tscratch = tr->scratch;
+	for (i = 0; i < tscratch->nr_entries; i++) {
+		entry = &tscratch->entries[i];
+		if (!strcmp(entry->mod_name, mod->name)) {
+			encode_last_removed(tscratch, i);
+			break;
+		}
+	}
+}
+
+static void trace_module_mark_remove(struct module *mod)
+{
+	struct trace_array *tr;
+	unsigned long flags;
+
+	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+		flags = tr->flags & (TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT);
+		/* Update any persistent trace array that has already been started */
+		if (flags == TRACE_ARRAY_FL_BOOT) {
+			/* Only update if the trace array is active */
+			if (trace_array_active(tr))
+				mark_mod_removed(tr, mod);
 		}
 	}
 }
@@ -10124,6 +10274,7 @@ static int trace_module_notify(struct notifier_block *self,
 		break;
 	case MODULE_STATE_GOING:
 		trace_module_remove_evals(mod);
+		trace_module_mark_remove(mod);
 		break;
 	}
 
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 90493220c362..47c0742fe9ec 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -349,6 +349,8 @@ struct trace_array {
 	unsigned long		range_addr_start;
 	unsigned long		range_addr_size;
 	long			text_delta;
+	int			nr_modules;
+	long			*module_delta;
 	void			*scratch; /* pointer in persistent memory */
 	int			scratch_size;
 
@@ -465,6 +467,8 @@ extern int tracing_set_clock(struct trace_array *tr, const char *clockstr);
 
 extern bool trace_clock_in_ns(struct trace_array *tr);
 
+extern unsigned long trace_adjust_address(struct trace_array *tr, unsigned long addr);
+
 /*
  * The global tracer (top) should be the first trace array added,
  * but we check the flag anyway.
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index 03d56f711ad1..a5336c4ece8e 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -1248,7 +1248,6 @@ static enum print_line_t trace_stack_print(struct trace_iterator *iter,
 	struct trace_seq *s = &iter->seq;
 	unsigned long *p;
 	unsigned long *end;
-	long delta = iter->tr->text_delta;
 
 	trace_assign_type(field, iter->ent);
 	end = (unsigned long *)((long)iter->ent + iter->ent_size);
@@ -1265,7 +1264,7 @@ static enum print_line_t trace_stack_print(struct trace_iterator *iter,
 			trace_seq_puts(s, "[FTRACE TRAMPOLINE]\n");
 			continue;
 		}
-		seq_print_ip_sym(s, (*p) + delta, flags);
+		seq_print_ip_sym(s, trace_adjust_address(iter->tr, *p), flags);
 		trace_seq_putc(s, '\n');
 	}
 

Re: [RFC PATCH v2 1/3] tracing: Show last module text symbols in the stacktrace

[Steven Rostedt]

On Tue, 11 Feb 2025 00:43:57 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Since the previous boot trace buffer can include module text address in
> the stacktrace. As same as the kernel text address, convert the module
> text address using the module address information.
> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  Changes in v2:
>   - Add LRU based removed module information override.

I don't think we should bother supporting removal of modules yet.

It also makes this patch way too complex for what it should do.

>   - Fix to move module_delta initialization after tr->scratch check.

>   - Fix to make mod_delta on all loaded modules correctly.

Should be a separate patch.

>   - (Note, I don't add uname check yet)
>   - Fix mod_addr_comp() overflow issue.
>   - Add pr_info() when failed to allocate module_delta.

So basically this patch should just add the code to handle finding the
module for stack traces. Not to mention, I have code that depends on that,
but this patch currently does way to much.

Can you just send a patch that adds the trace_adjust_address() (although, I
would shorten it to "trace_addr_adjust()").

And add what is needed to update the trace_stack_print().

You can base it off of the ring-buffer/for-next branch.

Thanks,

-- Steve


> ---
>  kernel/trace/trace.c        |  173 ++++++++++++++++++++++++++++++++++++++++---
>  kernel/trace/trace.h        |    4 +
>  kernel/trace/trace_output.c |    3 -
>  3 files changed, 167 insertions(+), 13 deletions(-)
> 

Re: [RFC PATCH v2 1/3] tracing: Show last module text symbols in the stacktrace

[Masami Hiramatsu]

On Thu, 6 Mar 2025 19:46:27 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Feb 2025 00:43:57 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Since the previous boot trace buffer can include module text address in
> > the stacktrace. As same as the kernel text address, convert the module
> > text address using the module address information.
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  Changes in v2:
> >   - Add LRU based removed module information override.
> 
> I don't think we should bother supporting removal of modules yet.
> 
> It also makes this patch way too complex for what it should do.
> 
> >   - Fix to move module_delta initialization after tr->scratch check.
> 
> >   - Fix to make mod_delta on all loaded modules correctly.
> 
> Should be a separate patch.
> 
> >   - (Note, I don't add uname check yet)
> >   - Fix mod_addr_comp() overflow issue.
> >   - Add pr_info() when failed to allocate module_delta.
> 
> So basically this patch should just add the code to handle finding the
> module for stack traces. Not to mention, I have code that depends on that,
> but this patch currently does way to much.
> 
> Can you just send a patch that adds the trace_adjust_address() (although, I
> would shorten it to "trace_addr_adjust()").
> 
> And add what is needed to update the trace_stack_print().
> 
> You can base it off of the ring-buffer/for-next branch.

OK, BTW, if we always need tscratch, we can move module_delta in it.
Let's reimplement it.

Thanks,

> 
> Thanks,
> 
> -- Steve
> 
> 
> > ---
> >  kernel/trace/trace.c        |  173 ++++++++++++++++++++++++++++++++++++++++---
> >  kernel/trace/trace.h        |    4 +
> >  kernel/trace/trace_output.c |    3 -
> >  3 files changed, 167 insertions(+), 13 deletions(-)
> > 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[RFC PATCH v2 2/3] mm/memblock: Add reserved memory release function

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Add reserved_mem_release_by_name() to release a reserved memory region
with a given name. This allows us to release reserved memory which is
defined by kernel cmdline, after boot.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 include/linux/mm.h |    1 +
 mm/memblock.c      |   72 +++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 61 insertions(+), 12 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index f02925447e59..a7201824c1fc 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4197,6 +4197,7 @@ void vma_pgtable_walk_begin(struct vm_area_struct *vma);
 void vma_pgtable_walk_end(struct vm_area_struct *vma);
 
 int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *size);
+int reserved_mem_release_by_name(const char *name);
 
 #ifdef CONFIG_64BIT
 int do_mseal(unsigned long start, size_t len_in, unsigned long flags);
diff --git a/mm/memblock.c b/mm/memblock.c
index 095c18b5c430..9962fbb90597 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -16,6 +16,7 @@
 #include <linux/kmemleak.h>
 #include <linux/seq_file.h>
 #include <linux/memblock.h>
+#include <linux/mutex.h>
 
 #include <asm/sections.h>
 #include <linux/io.h>
@@ -2263,6 +2264,7 @@ struct reserve_mem_table {
 };
 static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
 static int reserved_mem_count;
+static DEFINE_MUTEX(reserved_mem_lock);
 
 /* Add wildcard region with a lookup name */
 static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
@@ -2276,6 +2278,21 @@ static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
 	strscpy(map->name, name);
 }
 
+static struct reserve_mem_table *reserve_mem_find_by_name_nolock(const char *name)
+{
+	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)
+			return map;
+	}
+	return NULL;
+}
+
 /**
  * reserve_mem_find_by_name - Find reserved memory region with a given name
  * @name: The name that is attached to a reserved memory region
@@ -2289,22 +2306,53 @@ static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
 int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *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;
+	guard(mutex)(&reserved_mem_lock);
+	map = reserve_mem_find_by_name_nolock(name);
+	if (!map)
+		return 0;
+
+	*start = map->start;
+	*size = map->size;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(reserve_mem_find_by_name);
 
+/**
+ * reserved_mem_release_by_name - Release reserved memory region with a given name
+ * @name: The name that is attatched to a reserved memory region
+ *
+ * Forcibly release the pages in the reserved memory region so that those memory
+ * can be used as free memory. After released the reserved region size becomes 0.
+ *
+ * Returns: 1 if released or 0 if not found.
+ */
+int reserved_mem_release_by_name(const char *name)
+{
+	struct reserve_mem_table *map;
+	unsigned int page_count;
+	phys_addr_t start;
+
+	guard(mutex)(&reserved_mem_lock);
+	map = reserve_mem_find_by_name_nolock(name);
+	if (!map)
+		return 0;
+
+	start = map->start;
+	page_count = DIV_ROUND_UP(map->size, PAGE_SIZE);
+
+	for (int i = 0; i < page_count; i++) {
+		phys_addr_t addr = start + i * PAGE_SIZE;
+		struct page *page = pfn_to_page(addr >> PAGE_SHIFT);
+
+		page->flags &= ~BIT(PG_reserved);
+		__free_page(page);
+	}
+	map->size = 0;
+
+	return 1;
+}
+
 /*
  * Parse reserve_mem=nn:align:name
  */

Re: [RFC PATCH v2 2/3] mm/memblock: Add reserved memory release function

[Steven Rostedt]

On Tue, 11 Feb 2025 00:44:08 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Add reserved_mem_release_by_name() to release a reserved memory region
> with a given name. This allows us to release reserved memory which is
> defined by kernel cmdline, after boot.

Since Mike wrote the reserve_mem code, we should Cc him on changes.

Also, I would make patches 2 and 3 a separate patchset, as it is unrelated
to the other code.

> 
> Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> ---
>  include/linux/mm.h |    1 +
>  mm/memblock.c      |   72 +++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index f02925447e59..a7201824c1fc 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4197,6 +4197,7 @@ void vma_pgtable_walk_begin(struct vm_area_struct *vma);
>  void vma_pgtable_walk_end(struct vm_area_struct *vma);
>  
>  int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *size);
> +int reserved_mem_release_by_name(const char *name);

Should be "reserve_mem_release_by_name()" and not "reserved_mem" to stay consistent.

>  
>  #ifdef CONFIG_64BIT
>  int do_mseal(unsigned long start, size_t len_in, unsigned long flags);
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 095c18b5c430..9962fbb90597 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -16,6 +16,7 @@
>  #include <linux/kmemleak.h>
>  #include <linux/seq_file.h>
>  #include <linux/memblock.h>
> +#include <linux/mutex.h>
>  
>  #include <asm/sections.h>
>  #include <linux/io.h>
> @@ -2263,6 +2264,7 @@ struct reserve_mem_table {
>  };
>  static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
>  static int reserved_mem_count;
> +static DEFINE_MUTEX(reserved_mem_lock);

"reserve_mem_lock".

>  
>  /* Add wildcard region with a lookup name */
>  static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> @@ -2276,6 +2278,21 @@ static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
>  	strscpy(map->name, name);
>  }
>  
> +static struct reserve_mem_table *reserve_mem_find_by_name_nolock(const char *name)
> +{
> +	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)
> +			return map;
> +	}
> +	return NULL;
> +}
> +
>  /**
>   * reserve_mem_find_by_name - Find reserved memory region with a given name
>   * @name: The name that is attached to a reserved memory region
> @@ -2289,22 +2306,53 @@ static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
>  int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *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;
> +	guard(mutex)(&reserved_mem_lock);
> +	map = reserve_mem_find_by_name_nolock(name);
> +	if (!map)
> +		return 0;
> +
> +	*start = map->start;
> +	*size = map->size;
> +	return 1;
>  }
>  EXPORT_SYMBOL_GPL(reserve_mem_find_by_name);
>  
> +/**
> + * reserved_mem_release_by_name - Release reserved memory region with a given name
> + * @name: The name that is attatched to a reserved memory region
> + *
> + * Forcibly release the pages in the reserved memory region so that those memory
> + * can be used as free memory. After released the reserved region size becomes 0.
> + *
> + * Returns: 1 if released or 0 if not found.
> + */
> +int reserved_mem_release_by_name(const char *name)
> +{
> +	struct reserve_mem_table *map;
> +	unsigned int page_count;
> +	phys_addr_t start;
> +
> +	guard(mutex)(&reserved_mem_lock);
> +	map = reserve_mem_find_by_name_nolock(name);
> +	if (!map)
> +		return 0;
> +
> +	start = map->start;
> +	page_count = DIV_ROUND_UP(map->size, PAGE_SIZE);
> +
> +	for (int i = 0; i < page_count; i++) {
> +		phys_addr_t addr = start + i * PAGE_SIZE;
> +		struct page *page = pfn_to_page(addr >> PAGE_SHIFT);
> +
> +		page->flags &= ~BIT(PG_reserved);
> +		__free_page(page);
> +	}
> +	map->size = 0;
> +
> +	return 1;
> +}
> +
>  /*
>   * Parse reserve_mem=nn:align:name
>   */


Thanks,

-- Steve

Re: [RFC PATCH v2 2/3] mm/memblock: Add reserved memory release function

[Masami Hiramatsu]

On Mon, 10 Feb 2025 19:03:06 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Feb 2025 00:44:08 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > Add reserved_mem_release_by_name() to release a reserved memory region
> > with a given name. This allows us to release reserved memory which is
> > defined by kernel cmdline, after boot.
> 
> Since Mike wrote the reserve_mem code, we should Cc him on changes.
> 
> Also, I would make patches 2 and 3 a separate patchset, as it is unrelated
> to the other code.

OK, let me make a separate series and sending Cc mm maintainers too.


> 
> > 
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> >  include/linux/mm.h |    1 +
> >  mm/memblock.c      |   72 +++++++++++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 61 insertions(+), 12 deletions(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index f02925447e59..a7201824c1fc 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -4197,6 +4197,7 @@ void vma_pgtable_walk_begin(struct vm_area_struct *vma);
> >  void vma_pgtable_walk_end(struct vm_area_struct *vma);
> >  
> >  int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *size);
> > +int reserved_mem_release_by_name(const char *name);
> 
> Should be "reserve_mem_release_by_name()" and not "reserved_mem" to stay consistent.

Ahh, good catch. I confused reserved_mem and reserve_mem.

> 
> >  
> >  #ifdef CONFIG_64BIT
> >  int do_mseal(unsigned long start, size_t len_in, unsigned long flags);
> > diff --git a/mm/memblock.c b/mm/memblock.c
> > index 095c18b5c430..9962fbb90597 100644
> > --- a/mm/memblock.c
> > +++ b/mm/memblock.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/kmemleak.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/memblock.h>
> > +#include <linux/mutex.h>
> >  
> >  #include <asm/sections.h>
> >  #include <linux/io.h>
> > @@ -2263,6 +2264,7 @@ struct reserve_mem_table {
> >  };
> >  static struct reserve_mem_table reserved_mem_table[RESERVE_MEM_MAX_ENTRIES];
> >  static int reserved_mem_count;
> > +static DEFINE_MUTEX(reserved_mem_lock);
> 
> "reserve_mem_lock".

OK.

Thanks!

> 
> >  
> >  /* Add wildcard region with a lookup name */
> >  static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> > @@ -2276,6 +2278,21 @@ static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> >  	strscpy(map->name, name);
> >  }
> >  
> > +static struct reserve_mem_table *reserve_mem_find_by_name_nolock(const char *name)
> > +{
> > +	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)
> > +			return map;
> > +	}
> > +	return NULL;
> > +}
> > +
> >  /**
> >   * reserve_mem_find_by_name - Find reserved memory region with a given name
> >   * @name: The name that is attached to a reserved memory region
> > @@ -2289,22 +2306,53 @@ static void __init reserved_mem_add(phys_addr_t start, phys_addr_t size,
> >  int reserve_mem_find_by_name(const char *name, phys_addr_t *start, phys_addr_t *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;
> > +	guard(mutex)(&reserved_mem_lock);
> > +	map = reserve_mem_find_by_name_nolock(name);
> > +	if (!map)
> > +		return 0;
> > +
> > +	*start = map->start;
> > +	*size = map->size;
> > +	return 1;
> >  }
> >  EXPORT_SYMBOL_GPL(reserve_mem_find_by_name);
> >  
> > +/**
> > + * reserved_mem_release_by_name - Release reserved memory region with a given name
> > + * @name: The name that is attatched to a reserved memory region
> > + *
> > + * Forcibly release the pages in the reserved memory region so that those memory
> > + * can be used as free memory. After released the reserved region size becomes 0.
> > + *
> > + * Returns: 1 if released or 0 if not found.
> > + */
> > +int reserved_mem_release_by_name(const char *name)
> > +{
> > +	struct reserve_mem_table *map;
> > +	unsigned int page_count;
> > +	phys_addr_t start;
> > +
> > +	guard(mutex)(&reserved_mem_lock);
> > +	map = reserve_mem_find_by_name_nolock(name);
> > +	if (!map)
> > +		return 0;
> > +
> > +	start = map->start;
> > +	page_count = DIV_ROUND_UP(map->size, PAGE_SIZE);
> > +
> > +	for (int i = 0; i < page_count; i++) {
> > +		phys_addr_t addr = start + i * PAGE_SIZE;
> > +		struct page *page = pfn_to_page(addr >> PAGE_SHIFT);
> > +
> > +		page->flags &= ~BIT(PG_reserved);
> > +		__free_page(page);
> > +	}
> > +	map->size = 0;
> > +
> > +	return 1;
> > +}
> > +
> >  /*
> >   * Parse reserve_mem=nn:align:name
> >   */
> 
> 
> Thanks,
> 
> -- Steve


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

[RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

[Masami Hiramatsu (Google)]

From: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Make the ring buffer on reserved memory to be freeable. This allows us
to free the trace instance on the reserved memory without changing
cmdline and rebooting. Even if we can not change the kernel cmdline
for security reason, we can release the reserved memory for the ring
buffer as free (available) memory.

For example, boot kernel with reserved memory;
"reserve_mem=20M:2M:trace trace_instance=boot_mapped^traceoff@trace"

 # free
              total        used        free      shared  buff/cache   available
Mem:        1994720       45292     1931960       14908       17468     1915920
Swap:             0           0           0
 # rmdir /sys/kernel/tracing/instances/boot_mapped/
 # free
              total        used        free      shared  buff/cache   available
Mem:        1994720       17204     1960060       14912       17456     1944012
Swap:             0           0           0

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 kernel/trace/trace.c |   17 +++++++++++++++--
 kernel/trace/trace.h |    1 +
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index dde8000a5614..113ad3179ecd 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -9594,6 +9594,9 @@ static void free_trace_buffers(struct trace_array *tr)
 #ifdef CONFIG_TRACER_MAX_TRACE
 	free_trace_buffer(&tr->max_buffer);
 #endif
+
+	if (tr->range_addr_start)
+		vunmap((void *)tr->range_addr_start);
 }
 
 static void init_trace_flags_index(struct trace_array *tr)
@@ -9755,6 +9758,7 @@ trace_array_create_systems(const char *name, const char *systems,
 	free_cpumask_var(tr->pipe_cpumask);
 	free_cpumask_var(tr->tracing_cpumask);
 	kfree_const(tr->system_names);
+	kfree(tr->range_name);
 	kfree(tr->name);
 	kfree(tr);
 
@@ -9857,8 +9861,10 @@ static int __remove_instance(struct trace_array *tr)
 	int i;
 
 	/* Reference counter for a newly created trace array = 1. */
-	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref))
+	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref)) {
+		pr_info("Instance is busy: ref %d trace_ref %d\n", tr->ref, tr->trace_ref);
 		return -EBUSY;
+	}
 
 	list_del(&tr->list);
 
@@ -9881,6 +9887,11 @@ static int __remove_instance(struct trace_array *tr)
 	free_trace_buffers(tr);
 	clear_tracing_err_log(tr);
 
+	if (tr->range_name) {
+		reserved_mem_release_by_name(tr->range_name);
+		kfree(tr->range_name);
+	}
+
 	for (i = 0; i < tr->nr_topts; i++) {
 		kfree(tr->topts[i].topts);
 	}
@@ -10740,6 +10751,7 @@ __init static void enable_instances(void)
 		bool traceoff = false;
 		char *flag_delim;
 		char *addr_delim;
+		char *rname __free(kfree) = NULL;
 
 		tok = strsep(&curr_str, ",");
 
@@ -10796,6 +10808,7 @@ __init static void enable_instances(void)
 				pr_warn("Failed to map boot instance %s to %s\n", name, tok);
 				continue;
 			}
+			rname = kstrdup(tok, GFP_KERNEL);
 		}
 
 		if (start) {
@@ -10832,7 +10845,7 @@ __init static void enable_instances(void)
 		 */
 		if (start) {
 			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
-			tr->ref++;
+			tr->range_name = no_free_ptr(rname);
 		}
 
 		while ((tok = strsep(&curr_str, ","))) {
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 47c0742fe9ec..ae8f7fac6592 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -348,6 +348,7 @@ struct trace_array {
 	unsigned int		mapped;
 	unsigned long		range_addr_start;
 	unsigned long		range_addr_size;
+	char			*range_name;
 	long			text_delta;
 	int			nr_modules;
 	long			*module_delta;

Re: [RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

[Steven Rostedt]

On Tue, 11 Feb 2025 00:44:17 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:

> @@ -9857,8 +9861,10 @@ static int __remove_instance(struct trace_array *tr)
>  	int i;
>  
>  	/* Reference counter for a newly created trace array = 1. */
> -	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref))
> +	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref)) {
> +		pr_info("Instance is busy: ref %d trace_ref %d\n", tr->ref, tr->trace_ref);

I don't think we need the print. The function will error with -EBUSY
letting the user know that it is busy. This is a very common error, where I
found that I had a program that didn't completely die, and still had a file
descriptor open on an instance, and when I go to remove it, I get:

  # cd /sys/kernel/tracing/
  # mkdir instances/foo
  # bash 5< instances/foo/events/enable
  # rmdir instances/foo
  rmdir: failed to remove 'instances/foo': Device or resource busy
  # exit
  # rmdir instances/foo
  #

If you only have it for debugging that is fine, but we should remove it
before we accept it.

-- Steve

>  		return -EBUSY;
> +	}
>  
>  	list_del(&tr->list);
>  
> @@ -9881,6 +9887,11 @@ static int __remove_instance(struct trace_array *tr)
>  	free_trace_buffers(tr);
>  	clear_tracing_err_log(tr);
>  
> +	if (tr->range_name) {
> +		reserved_mem_release_by_name(tr->range_name);
> +		kfree(tr->range_name);
> +	}
> +
>  	for (i = 0; i < tr->nr_topts; i++) {
>  		kfree(tr->topts[i].topts);
>  	}
> @@ -10740,6 +10751,7 @@ __init static void enable_instances(void)
>  		bool traceoff = false;
>  		char *flag_delim;
>  		char *addr_delim;
> +		char *rname __free(kfree) = NULL;
>  
>  		tok = strsep(&curr_str, ",");
>  
> @@ -10796,6 +10808,7 @@ __init static void enable_instances(void)
>  				pr_warn("Failed to map boot instance %s to %s\n", name, tok);
>  				continue;
>  			}
> +			rname = kstrdup(tok, GFP_KERNEL);
>  		}
>  
>  		if (start) {
> @@ -10832,7 +10845,7 @@ __init static void enable_instances(void)
>  		 */
>  		if (start) {
>  			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
> -			tr->ref++;
> +			tr->range_name = no_free_ptr(rname);
>  		}
>  
>  		while ((tok = strsep(&curr_str, ","))) {
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 47c0742fe9ec..ae8f7fac6592 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -348,6 +348,7 @@ struct trace_array {
>  	unsigned int		mapped;
>  	unsigned long		range_addr_start;
>  	unsigned long		range_addr_size;
> +	char			*range_name;
>  	long			text_delta;
>  	int			nr_modules;
>  	long			*module_delta;

Re: [RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

[Masami Hiramatsu]

On Mon, 10 Feb 2025 19:13:50 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 11 Feb 2025 00:44:17 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > @@ -9857,8 +9861,10 @@ static int __remove_instance(struct trace_array *tr)
> >  	int i;
> >  
> >  	/* Reference counter for a newly created trace array = 1. */
> > -	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref))
> > +	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref)) {
> > +		pr_info("Instance is busy: ref %d trace_ref %d\n", tr->ref, tr->trace_ref);
> 
> I don't think we need the print. The function will error with -EBUSY
> letting the user know that it is busy. This is a very common error, where I
> found that I had a program that didn't completely die, and still had a file
> descriptor open on an instance, and when I go to remove it, I get:
> 
>   # cd /sys/kernel/tracing/
>   # mkdir instances/foo
>   # bash 5< instances/foo/events/enable
>   # rmdir instances/foo
>   rmdir: failed to remove 'instances/foo': Device or resource busy
>   # exit
>   # rmdir instances/foo
>   #
> 
> If you only have it for debugging that is fine, but we should remove it
> before we accept it.

Oops, that is my debug code. I missed to remove that! Sorry for confusion.

Thank you!

> 
> -- Steve
> 
> >  		return -EBUSY;
> > +	}
> >  
> >  	list_del(&tr->list);
> >  
> > @@ -9881,6 +9887,11 @@ static int __remove_instance(struct trace_array *tr)
> >  	free_trace_buffers(tr);
> >  	clear_tracing_err_log(tr);
> >  
> > +	if (tr->range_name) {
> > +		reserved_mem_release_by_name(tr->range_name);
> > +		kfree(tr->range_name);
> > +	}
> > +
> >  	for (i = 0; i < tr->nr_topts; i++) {
> >  		kfree(tr->topts[i].topts);
> >  	}
> > @@ -10740,6 +10751,7 @@ __init static void enable_instances(void)
> >  		bool traceoff = false;
> >  		char *flag_delim;
> >  		char *addr_delim;
> > +		char *rname __free(kfree) = NULL;
> >  
> >  		tok = strsep(&curr_str, ",");
> >  
> > @@ -10796,6 +10808,7 @@ __init static void enable_instances(void)
> >  				pr_warn("Failed to map boot instance %s to %s\n", name, tok);
> >  				continue;
> >  			}
> > +			rname = kstrdup(tok, GFP_KERNEL);
> >  		}
> >  
> >  		if (start) {
> > @@ -10832,7 +10845,7 @@ __init static void enable_instances(void)
> >  		 */
> >  		if (start) {
> >  			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
> > -			tr->ref++;
> > +			tr->range_name = no_free_ptr(rname);
> >  		}
> >  
> >  		while ((tok = strsep(&curr_str, ","))) {
> > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> > index 47c0742fe9ec..ae8f7fac6592 100644
> > --- a/kernel/trace/trace.h
> > +++ b/kernel/trace/trace.h
> > @@ -348,6 +348,7 @@ struct trace_array {
> >  	unsigned int		mapped;
> >  	unsigned long		range_addr_start;
> >  	unsigned long		range_addr_size;
> > +	char			*range_name;
> >  	long			text_delta;
> >  	int			nr_modules;
> >  	long			*module_delta;
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [RFC PATCH v2 3/3] tracing: Freeable reserved ring buffer

[Steven Rostedt]

On Tue, 11 Feb 2025 10:41:22 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> On Mon, 10 Feb 2025 19:13:50 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Tue, 11 Feb 2025 00:44:17 +0900
> > "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> >   
> > > @@ -9857,8 +9861,10 @@ static int __remove_instance(struct trace_array *tr)
> > >  	int i;
> > >  
> > >  	/* Reference counter for a newly created trace array = 1. */
> > > -	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref))
> > > +	if (tr->ref > 1 || (tr->current_trace && tr->trace_ref)) {
> > > +		pr_info("Instance is busy: ref %d trace_ref %d\n", tr->ref, tr->trace_ref);  
> > 
> > I don't think we need the print. The function will error with -EBUSY
> > letting the user know that it is busy. This is a very common error, where I
> > found that I had a program that didn't completely die, and still had a file
> > descriptor open on an instance, and when I go to remove it, I get:
> > 
> >   # cd /sys/kernel/tracing/
> >   # mkdir instances/foo
> >   # bash 5< instances/foo/events/enable
> >   # rmdir instances/foo
> >   rmdir: failed to remove 'instances/foo': Device or resource busy
> >   # exit
> >   # rmdir instances/foo
> >   #
> > 
> > If you only have it for debugging that is fine, but we should remove it
> > before we accept it.  
> 
> Oops, that is my debug code. I missed to remove that! Sorry for confusion.

Heh, that's why I use "printk()" and not the "pr_*()" functions. Since I
don't usually have "printk()" in the normal code but use the "pr_*()"
functions there, it makes it easy for me to find the debug statements I put
into my code while I develop it.

-- Steve