[PATCH v4 0/4] tracing: Clean up persistent ring buffer code (was: ring-buffer: Allow persistent memory to be user space mmapped)

[PATCH v4 0/4] tracing: Clean up persistent ring buffer code (was: ring-buffer: Allow persistent memory to be user space mmapped)

[Steven Rostedt]

[
  Note, this does not include the update to allow the persistent
  ring buffer to be memory mapped to user space. This is only the
  4 patches that I feel makes the code more correct and also
  has a bug fix.
]

Now that I learned that the memory passed back from reserve_mem is part
of the memory allocator and just "reserved" and the memory is already
virtually mapped, it can simply use phys_to_virt() on the physical memory
that is returned to get the virtual mapping for that memory!
  (Thanks Mike!)

That makes things much easier, especially since it means that the memory
returned by reserve_mem is no different than the memory retrieved by
page_alloc(). This allows that memory to be memory mapped to user space
no differently than it is mapped by the normal buffer.

This new series does the following:

- Enforce the memory mapping is page aligned (both the address and the
  size). If not, it errors out.

- Use phys_to_virt() to get to the virtual memory from the reserve_mem
  returned addresses. Also use free_reserved_area() to give it
  back to the buddy allocator when it is freed.

- Treat the buffer allocated via memmap differently. It still needs to
  be virtually mapped (cannot use phys_to_virt) and it must not be
  freed nor memory mapped to user space. A new flag is added when a buffer
  is created this way to prevent it from ever being memory mapped to user
  space and the ref count is upped so that it can never be freed.

- Use vmap_page_range() instead of using kmalloc_array() to create an array
  of struct pages for vmap().

- Use flush_kernel_vmap_range() instead of flush_dcache_folio()

Changes since v3: https://lore.kernel.org/linux-trace-kernel/20250401202549.409271454@goodmis.org/

- Fix whitespace in patch 3

- Updated change log of patch 4 to include bug fix comment

- Removed the patch that enables persistent ring buffer from being
  memory mapped to user space. That will be done in the next merge window.

Steven Rostedt (4):
      tracing: Enforce the persistent ring buffer to be page aligned
      tracing: Have reserve_mem use phys_to_virt() and separate from memmap buffer
      tracing: Use vmap_page_range() to map memmap ring buffer
      ring-buffer: Use flush_kernel_vmap_range() over flush_dcache_folio()

----
 Documentation/admin-guide/kernel-parameters.txt |  2 +
 Documentation/trace/debugging.rst               |  2 +
 kernel/trace/ring_buffer.c                      |  5 +-
 kernel/trace/trace.c                            | 73 +++++++++++++++++--------
 kernel/trace/trace.h                            |  1 +
 5 files changed, 58 insertions(+), 25 deletions(-)

[PATCH v4 1/4] tracing: Enforce the persistent ring buffer to be page aligned

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

Enforce that the address and the size of the memory used by the persistent
ring buffer is page aligned. Also update the documentation to reflect this
requirement.

Link: https://lore.kernel.org/all/CAHk-=whUOfVucfJRt7E0AH+GV41ELmS4wJqxHDnui6Giddfkzw@mail.gmail.com/

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 ++
 Documentation/trace/debugging.rst               |  2 ++
 kernel/trace/trace.c                            | 12 ++++++++++++
 3 files changed, 16 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 3435a062a208..f904fd8481bd 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -7266,6 +7266,8 @@
 			This is just one of many ways that can clear memory. Make sure your system
 			keeps the content of memory across reboots before relying on this option.
 
+			NB: Both the mapped address and size must be page aligned for the architecture.
+
 			See also Documentation/trace/debugging.rst
 
 
diff --git a/Documentation/trace/debugging.rst b/Documentation/trace/debugging.rst
index 54fb16239d70..d54bc500af80 100644
--- a/Documentation/trace/debugging.rst
+++ b/Documentation/trace/debugging.rst
@@ -136,6 +136,8 @@ kernel, so only the same kernel is guaranteed to work if the mapping is
 preserved. Switching to a different kernel version may find a different
 layout and mark the buffer as invalid.
 
+NB: Both the mapped address and size must be page aligned for the architecture.
+
 Using trace_printk() in the boot instance
 -----------------------------------------
 By default, the content of trace_printk() goes into the top level tracing
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index de6d7f0e6206..de9c237e5826 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -10788,6 +10788,18 @@ __init static void enable_instances(void)
 		}
 
 		if (start) {
+			/* Start and size must be page aligned */
+			if (start & ~PAGE_MASK) {
+				pr_warn("Tracing: mapping start addr %lx is not page aligned\n",
+					(unsigned long)start);
+				continue;
+			}
+			if (size & ~PAGE_MASK) {
+				pr_warn("Tracing: mapping size %lx is not page aligned\n",
+					(unsigned long)size);
+				continue;
+			}
+
 			addr = map_pages(start, size);
 			if (addr) {
 				pr_info("Tracing: mapped boot instance %s at physical memory %pa of size 0x%lx\n",
-- 
2.47.2

[PATCH v4 2/4] tracing: Have reserve_mem use phys_to_virt() and separate from memmap buffer

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The reserve_mem kernel command line option may pass back a physical
address, but the memory is still part of the normal memory just like
using memblock_reserve() would be. This means that the physical memory
returned by the reserve_mem command line option can be converted directly
to virtual memory by simply using phys_to_virt().

When freeing the buffer allocated by reserve_mem, use free_reserved_area().

Because the persistent ring buffer can also be allocated via the memmap
option, which *is* different than normal memory as it cannot be added back
to the buddy system, it must be treated differently. It still needs to be
virtually mapped to have access to it. It also can not be freed nor can it
ever be memory mapped to user space.

Create a new trace_array flag called TRACE_ARRAY_FL_MEMMAP which gets set
if the buffer is created by the memmap option, and this will prevent the
buffer from being memory mapped by user space.

Also increment the ref count for memmap'ed buffers so that they can never
be freed.

Link: https://lore.kernel.org/all/Z-wFszhJ_9o4dc8O@kernel.org/

Suggested-by: Mike Rapoport <rppt@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 28 ++++++++++++++++++++++------
 kernel/trace/trace.h |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index de9c237e5826..d960f80701bd 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8505,6 +8505,10 @@ static int tracing_buffers_mmap(struct file *filp, struct vm_area_struct *vma)
 	struct trace_iterator *iter = &info->iter;
 	int ret = 0;
 
+	/* A memmap'ed buffer is not supported for user space mmap */
+	if (iter->tr->flags & TRACE_ARRAY_FL_MEMMAP)
+		return -ENODEV;
+
 	/* Currently the boot mapped buffer is not supported for mmap */
 	if (iter->tr->flags & TRACE_ARRAY_FL_BOOT)
 		return -ENODEV;
@@ -9615,8 +9619,12 @@ static void free_trace_buffers(struct trace_array *tr)
 	free_trace_buffer(&tr->max_buffer);
 #endif
 
-	if (tr->range_addr_start)
-		vunmap((void *)tr->range_addr_start);
+	if (tr->range_addr_start) {
+		void *start = (void *)tr->range_addr_start;
+		void *end = start + tr->range_addr_size;
+
+		free_reserved_area(start, end, 0, tr->range_name);
+	}
 }
 
 static void init_trace_flags_index(struct trace_array *tr)
@@ -10710,6 +10718,7 @@ static inline void do_allocate_snapshot(const char *name) { }
 __init static void enable_instances(void)
 {
 	struct trace_array *tr;
+	bool memmap_area = false;
 	char *curr_str;
 	char *name;
 	char *str;
@@ -10778,6 +10787,7 @@ __init static void enable_instances(void)
 					name);
 				continue;
 			}
+			memmap_area = true;
 		} else if (tok) {
 			if (!reserve_mem_find_by_name(tok, &start, &size)) {
 				start = 0;
@@ -10800,7 +10810,10 @@ __init static void enable_instances(void)
 				continue;
 			}
 
-			addr = map_pages(start, size);
+			if (memmap_area)
+				addr = map_pages(start, size);
+			else
+				addr = (unsigned long)phys_to_virt(start);
 			if (addr) {
 				pr_info("Tracing: mapped boot instance %s at physical memory %pa of size 0x%lx\n",
 					name, &start, (unsigned long)size);
@@ -10827,10 +10840,13 @@ __init static void enable_instances(void)
 			update_printk_trace(tr);
 
 		/*
-		 * If start is set, then this is a mapped buffer, and
-		 * cannot be deleted by user space, so keep the reference
-		 * to it.
+		 * memmap'd buffers can not be freed.
 		 */
+		if (memmap_area) {
+			tr->flags |= TRACE_ARRAY_FL_MEMMAP;
+			tr->ref++;
+		}
+
 		if (start) {
 			tr->flags |= TRACE_ARRAY_FL_BOOT | TRACE_ARRAY_FL_LAST_BOOT;
 			tr->range_name = no_free_ptr(rname);
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index c20f6bcc200a..f9513dc14c37 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -447,6 +447,7 @@ enum {
 	TRACE_ARRAY_FL_BOOT		= BIT(1),
 	TRACE_ARRAY_FL_LAST_BOOT	= BIT(2),
 	TRACE_ARRAY_FL_MOD_INIT		= BIT(3),
+	TRACE_ARRAY_FL_MEMMAP		= BIT(4),
 };
 
 #ifdef CONFIG_MODULES
-- 
2.47.2

[PATCH v4 3/4] tracing: Use vmap_page_range() to map memmap ring buffer

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

The code to map the physical memory retrieved by memmap currently
allocates an array of pages to cover the physical memory and then calls
vmap() to map it to a virtual address. Instead of using this temporary
array of struct page descriptors, simply use vmap_page_range() that can
directly map the contiguous physical memory to a virtual address.

Link: https://lore.kernel.org/all/CAHk-=whUOfVucfJRt7E0AH+GV41ELmS4wJqxHDnui6Giddfkzw@mail.gmail.com/

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/trace.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index d960f80701bd..24852bd4fc01 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -50,6 +50,7 @@
 #include <linux/irq_work.h>
 #include <linux/workqueue.h>
 #include <linux/sort.h>
+#include <linux/io.h> /* vmap_page_range() */
 
 #include <asm/setup.h> /* COMMAND_LINE_SIZE */
 
@@ -9817,29 +9818,27 @@ static int instance_mkdir(const char *name)
 	return ret;
 }
 
-static u64 map_pages(u64 start, u64 size)
+static u64 map_pages(unsigned long start, unsigned long size)
 {
-	struct page **pages;
-	phys_addr_t page_start;
-	unsigned int page_count;
-	unsigned int i;
-	void *vaddr;
-
-	page_count = DIV_ROUND_UP(size, PAGE_SIZE);
+	unsigned long vmap_start, vmap_end;
+	struct vm_struct *area;
+	int ret;
 
-	page_start = start;
-	pages = kmalloc_array(page_count, sizeof(struct page *), GFP_KERNEL);
-	if (!pages)
+	area = get_vm_area(size, VM_IOREMAP);
+	if (!area)
 		return 0;
 
-	for (i = 0; i < page_count; i++) {
-		phys_addr_t addr = page_start + i * PAGE_SIZE;
-		pages[i] = pfn_to_page(addr >> PAGE_SHIFT);
+	vmap_start = (unsigned long) area->addr;
+	vmap_end = vmap_start + size;
+
+	ret = vmap_page_range(vmap_start, vmap_end,
+			      start, pgprot_nx(PAGE_KERNEL));
+	if (ret < 0) {
+		free_vm_area(area);
+		return 0;
 	}
-	vaddr = vmap(pages, page_count, VM_MAP, PAGE_KERNEL);
-	kfree(pages);
 
-	return (u64)(unsigned long)vaddr;
+	return (u64)vmap_start;
 }
 
 /**
-- 
2.47.2

[PATCH v4 4/4] ring-buffer: Use flush_kernel_vmap_range() over flush_dcache_folio()

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

Some architectures do not have data cache coherency between user and
kernel space. For these architectures, the cache needs to be flushed on
both the kernel and user addresses so that user space can see the updates
the kernel has made.

Instead of using flush_dcache_folio() and playing with virt_to_folio()
within the call to that function, use flush_kernel_vmap_range() which
takes the virtual address and does the work for those architectures that
need it.

This also fixes a bug where the flush of the reader page only flushed one
page. If the sub-buffer order is 1 or more, where the sub-buffer size
would be greater than a page, it would miss the rest of the sub-buffer
content, as the "reader page" is not just a page, but the size of a
sub-buffer.

Link: https://lore.kernel.org/all/CAG48ez3w0my4Rwttbc5tEbNsme6tc0mrSN95thjXUFaJ3aQ6SA@mail.gmail.com/

Cc: stable@vger.kernel.org
Fixes: 117c39200d9d7 ("ring-buffer: Introducing ring-buffer mapping functions");
Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ring_buffer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index d8d7b28e2c2f..c0f877d39a24 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -6016,7 +6016,7 @@ static void rb_update_meta_page(struct ring_buffer_per_cpu *cpu_buffer)
 	meta->read = cpu_buffer->read;
 
 	/* Some archs do not have data cache coherency between kernel and user-space */
-	flush_dcache_folio(virt_to_folio(cpu_buffer->meta_page));
+	flush_kernel_vmap_range(cpu_buffer->meta_page, PAGE_SIZE);
 }
 
 static void
@@ -7319,7 +7319,8 @@ int ring_buffer_map_get_reader(struct trace_buffer *buffer, int cpu)
 
 out:
 	/* Some archs do not have data cache coherency between kernel and user-space */
-	flush_dcache_folio(virt_to_folio(cpu_buffer->reader_page->page));
+	flush_kernel_vmap_range(cpu_buffer->reader_page->page,
+				buffer->subbuf_size + BUF_PAGE_HDR_SIZE);
 
 	rb_update_meta_page(cpu_buffer);
 
-- 
2.47.2

Re: [PATCH v4 2/4] tracing: Have reserve_mem use phys_to_virt() and separate from memmap buffer

[Steven Rostedt]

On Tue, 01 Apr 2025 17:51:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> @@ -9615,8 +9619,12 @@ static void free_trace_buffers(struct trace_array *tr)
>  	free_trace_buffer(&tr->max_buffer);
>  #endif
>  
> -	if (tr->range_addr_start)
> -		vunmap((void *)tr->range_addr_start);
> +	if (tr->range_addr_start) {
> +		void *start = (void *)tr->range_addr_start;
> +		void *end = start + tr->range_addr_size;
> +
> +		free_reserved_area(start, end, 0, tr->range_name);
> +	}
>  }
>  
>  static void init_trace_flags_index(struct trace_array *tr)

Masami,

Note, your patch to free the persistent ring buffer wasn't fully
functional, as it only did the "vunmap()". That doesn't return the buffer
back to the buddy allocator. What you saw was just the freeing of all the
other descriptors that make up a trace instance.

Before this patch:

  ~# free
                 total        used        free      shared  buff/cache   available
  Mem:         8185908      297404     7825896         916      162288     7888504
  Swap:        7812092           0     7812092
  ~# rmdir /sys/kernel/tracing/instances/boot_mapped
  ~# free
                 total        used        free      shared  buff/cache   available
  Mem:         8206384      297956     7845904         916      162260     7908428
  Swap:        7812092           0     7812092

Amount freed: 7845904 - 7825896 = 20008 (20M)

After this patch:

  ~# free
                 total        used        free      shared  buff/cache   available
  Mem:         8185912      301808     7820696         920      162860     7884104
  Swap:        7812092           0     7812092
  ~# rmdir /sys/kernel/tracing/instances/boot_mapped
  ~# free
                 total        used        free      shared  buff/cache   available
  Mem:         8226868      295968     7867644         920      162836     7930900
  Swap:        7812092           0     7812092

Amount freed: 7867644 - 7820696 = 46948 (46M)!

-- Steve

Re: [PATCH v4 2/4] tracing: Have reserve_mem use phys_to_virt() and separate from memmap buffer

[Steven Rostedt]

On Tue, 1 Apr 2025 18:16:40 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:


> Note, your patch to free the persistent ring buffer wasn't fully
> functional, as it only did the "vunmap()". That doesn't return the buffer
> back to the buddy allocator. What you saw was just the freeing of all the
> other descriptors that make up a trace instance.
> 
> Before this patch:
> 
>   ~# free
>                  total        used        free      shared  buff/cache   available
>   Mem:         8185908      297404     7825896         916      162288     7888504
>   Swap:        7812092           0     7812092
>   ~# rmdir /sys/kernel/tracing/instances/boot_mapped
>   ~# free
>                  total        used        free      shared  buff/cache   available
>   Mem:         8206384      297956     7845904         916      162260     7908428
>   Swap:        7812092           0     7812092
> 
> Amount freed: 7845904 - 7825896 = 20008 (20M)
> 
> After this patch:
> 
>   ~# free
>                  total        used        free      shared  buff/cache   available
>   Mem:         8185912      301808     7820696         920      162860     7884104
>   Swap:        7812092           0     7812092
>   ~# rmdir /sys/kernel/tracing/instances/boot_mapped
>   ~# free
>                  total        used        free      shared  buff/cache   available
>   Mem:         8226868      295968     7867644         920      162836     7930900
>   Swap:        7812092           0     7812092
> 
> Amount freed: 7867644 - 7820696 = 46948 (46M)!

Bah! My patch is buggy. Yeah, your code released the memory properly, as you had:

	reserve_mem_release_by_name()

which does do free_reserve_area() on the memory mapping.

Which means this patch is buggy and freed the same memory twice.

OK, time for v5 to fix that :-p

-- Steve