[GIT PULL] ring-buffer: Updates for v6.15

[GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

Linus,

ring-buffer updates for v6.15

- Allow persistent ring buffer to be memory mapped

  The user space memory mapping used virt_to_page() to get the page from
  the virtual address of the ring buffer. This worked because the ring
  buffer was created via alloc_page() and friends. The persistent ring
  buffer is created from reserved memory at boot up and created via vmap().
  The page can not be retrieved by virt_to_page() for vmap() memory, but
  it can be from vmalloc_to_page(). Use that for the persistent memory
  and this allows it to be memory mapped by user space.

- Restructure the persistent memory to have a "scratch" area

  Instead of hard coding the KASLR offset in the persistent memory
  by the ring buffer, push that work up to the callers of the persistent
  memory as they are the ones that need this information. The offsets
  and such is not important to the ring buffer logic and it should
  not be part of that.

  A scratch pad is now created when the caller allocates a ring buffer
  from persistent memory by stating how much memory it needs to save.

- Allow where modules are loaded to be saved in the new scratch pad

  Save the addresses of modules when they are loaded into the persistent
  memory scratch pad.

- A new module_for_each_mod() helper function was created

  With the acknowledgment of the module maintainers a new module helper
  function was created to iterate over all the currently loaded modules.
  This has a callback to be called for each module. This is needed for
  when tracing is started in the persistent buffer and the currently loaded
  modules need to be saved in the scratch area.

- Expose the last boot information where the kernel and modules were loaded

  The last_boot_info file is updated to print out the addresses of where
  the kernel "_text" location was loaded from a previous boot, as well
  as where the modules are loaded. If the buffer is recording the current
  boot, it only prints "# Current" so that it does not expose the KASLR
  offset of the currently running kernel.

- Allow the persistent ring buffer to be released (freed)

  To have this in production environments, where the kernel command line can
  not be changed easily, the ring buffer needs to be freed when it is not
  going to be used. The memory for the buffer will always be allocated at
  boot up, but if the system isn't going to enable tracing, the memory needs
  to be freed. Allow it to be freed and added back to the kernel memory
  pool.

- Allow stack traces to print the function names in the persistent buffer

  Now that the modules are saved in the persistent ring buffer, if the same
  modules are loaded, the printing of the function names will examine the
  saved modules. If the module is found in the scratch area and is also
  loaded, then it will do the offset shift and use kallsyms to display the
  function name. If the address is not found, it simply displays the address
  from the previous boot in hex.


Please pull the latest trace-ringbuffer-v6.15 tree, which can be found at:


  git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace-ringbuffer-v6.15

Tag SHA1: baf5165f33ce4620c17fb927a0bd94fdfc548146
Head SHA1: f20423262b368ff860373d28050f0c4b65e82973


Feng Yang (1):
      ring-buffer: Fix bytes_dropped calculation issue

Jiapeng Chong (1):
      ring-buffer: Remove the unused variable bmeta

Masami Hiramatsu (Google) (5):
      mm/memblock: Add reserved memory release function
      tracing: Freeable reserved ring buffer
      tracing: Fix a compilation error without CONFIG_MODULES
      tracing: Skip update_last_data() if cleared and remove active check for save_mod()
      tracing: Show last module text symbols in the stacktrace

Steven Rostedt (12):
      ring-buffer: Allow persistent ring buffers to be mmapped
      ring-buffer: Use kaslr address instead of text delta
      ring-buffer: Add buffer meta data for persistent ring buffer
      ring-buffer: Add ring_buffer_meta_scratch()
      tracing: Have persistent trace instances save KASLR offset
      module: Add module_for_each_mod() function
      tracing: Have persistent trace instances save module addresses
      tracing: Show module names and addresses of last boot
      tracing: Update modules to persistent instances when loaded
      ring-buffer: Have parameter to virt_addr_valid() cast to pointer
      tracing: Initialize scratch_size to zero to prevent UB
      tracing: Use _text and the kernel offset in last_boot_info

----
 include/linux/mm.h          |   1 +
 include/linux/module.h      |   6 +
 include/linux/ring_buffer.h |   8 +-
 kernel/module/main.c        |  13 ++
 kernel/trace/ring_buffer.c  | 296 ++++++++++++++++++++-------------
 kernel/trace/trace.c        | 393 +++++++++++++++++++++++++++++++++++++++++---
 kernel/trace/trace.h        |  24 ++-
 kernel/trace/trace_events.c |  40 +++--
 kernel/trace/trace_output.c |   4 +-
 mm/memblock.c               |  66 ++++++--
 10 files changed, 681 insertions(+), 170 deletions(-)
---------------------------

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 14:09, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> - Allow persistent ring buffer to be memory mapped

Steven, I'm not going to tell you again.

Next time I see you making crazy mmap changes and trying to push that
shit to me, I'll just put you in my email spam filter and will stop
pulling entirely.

I'm done having to check your pull requests for crap you add randomly.
You have a *VERY* bad habit of writing code that "works", but not
because it should, or because it's well designed, but because it has
random hacks in it.

And dammit, THAT IS NOT OK when it comes to VM or core VFS code.

We don't do random shit that just happens to work. It needs to be
thought out, it needs to be correct, and it needs to be well-designed.

This is garbage. I pulled, I looked at it, and I puked. It is unpulled again.

And you have done this before, and I have complained about your
hackery before. You apparently learnt absolutely nothing.

Taking random kernel addresses, not knowing where they came from,
testing them with virt_addr_valid(), doing two different lookups to
turn them into a 'struct page', and then to add extra insanity, turn
that page into a folio - whether it was one or not - and using a folio
function on the end result, IS NOT OK.

And then repeating the same garbage five times to really *revel* in
rolling in that shit like some kind of animal? WHY? I have a dog that
loves rolling around in some dubious smelly stuff she finds on the
road, and I love her even if she's a disgusting thing and needs a
bath, but I damn well don't let her write kernel code.

Moral of the story: Don't be like my dog.

If this was a one-off thing, that would be one thing. But this kind of
behavior with "this works, but misuses core kernel functionality" has
been a PATTERN for you. To the point where I have to check your pull
requests and I dread going through them during the merge window.

I AM DONE.

Here's the things you DO NOT DO:

 - forget where your pointers came from, and then decide to ask the
core VM about the information *you* should have known about and
tracked

   Hint: keep track of how you damn well allocated the pages in the
first place, so you don't have to go to the VM code and say "how did I
allocate this"

 - repeat the same ugly and mindless pattern in five different places

   Hint: if you do the same ugly thing five times think about what the
heck you are doing

 - take a 'struct page', turn it into a folio with 'page_folio()' even
when it wasn't one, and use flush_dcache_folio() on the result

   Hint: there's a "flush_dcache_page()" function, for chrissake!

Dammit, as far as I can tell, you actually *did* have the information
about how something was allocated, because it's that
TRACE_ARRAY_FL_BOOT flag.

And the patch that implemented this crap actually *removed* the one
piece of code that used that information to make decisions, replacing
it with the crap that repeatedly just asked the VM "can you tell me
how I allocated this page"?

And the code could easily have been made to not look like the
disgusting mess it is by using a helper function to do all of this.
But no. Instead it wrote out that *disgusting* thing by hand five
times.

And that "subbuf_order" thing looks *very* suspicious, since it means
that any non-order-zero case will look up pages using
'vmalloc_to_page()', bvut then happily go on to assume that the
allocations are physically adjacent in those subbof_order chunks.

IOW, code like this:

                if (virt_addr_valid(cpu_buffer->subbuf_ids[s]))
                        page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
                else
                        page = vmalloc_to_page((void
*)cpu_buffer->subbuf_ids[s]);

                for (; off < (1 << (subbuf_order)); off++, page++) {
                        if (p >= nr_pages)
                                break;

                        pages[p++] = page;
                }

is NOT acceptable, because look how it first looks the page up using
vmalloc_to_page() ("because it was vmalloc'ed") and then it just does
"page++" as if it was some physically contiguous thing.

Hint: those two things do not go together.

I'm *hoping* that subbuf_order is always zero for anything that has
been allocated with vmalloc(), and that this is another of those "it
works, even though it looks like crap".

But my point is that the code shouldn't be written in a way where it
just *happens* to work, and "hoping" is not a strategy for long-term
maintenance.

IOW, you should not have code that first says "I have no clue how this
was allocated, so I'll ask the VM", and then in the very next breath
says "oh, I'll just treat this as a physically contiguous allocation
even if it was a vmalloc address".

Yes, I've seen random drivers do this kind of garbage. It's wrong and
bad then too, but at least it's not some core functionality, and
drivers doing "random hacks that happen to work" is just another fact
of life. It's not great, but it is what it is.

Stuff that is under the "kernel/" subdirectory has higher quality requirements.

And the thing that really annoys me is that this isn't even *remotely*
the first time I've had to complain about this kind of behavior.

I'm not willing to have to step in and fix your mis-use of core VM or
VFS functionality, so I'm not pulling this.

And I'm serious: you need to stop doing this kind of crap. Get some
real VM person to help you if you do memory mapping. Do things
*properly*, not this kind of "random hacks that work in testing" crud.

Or don't do memory mapping or subtle VFS stuff at all.

Really. Those are the choices you have.

               Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 17:59:54 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 27 Mar 2025 at 14:09, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > - Allow persistent ring buffer to be memory mapped  
> 
> Steven, I'm not going to tell you again.

Yes, I figured this would be the most controversial pull request I sent
you.

> 
> And that "subbuf_order" thing looks *very* suspicious, since it means
> that any non-order-zero case will look up pages using
> 'vmalloc_to_page()', bvut then happily go on to assume that the
> allocations are physically adjacent in those subbof_order chunks.
> 
> IOW, code like this:
> 
>                 if (virt_addr_valid(cpu_buffer->subbuf_ids[s]))
>                         page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
>                 else
>                         page = vmalloc_to_page((void
> *)cpu_buffer->subbuf_ids[s]);
> 
>                 for (; off < (1 << (subbuf_order)); off++, page++) {
>                         if (p >= nr_pages)
>                                 break;
> 
>                         pages[p++] = page;
>                 }
> 
> is NOT acceptable, because look how it first looks the page up using
> vmalloc_to_page() ("because it was vmalloc'ed") and then it just does
> "page++" as if it was some physically contiguous thing.

The pages are never vmalloc'd, it's only ever vmap()'d on top of
contiguous physical memory or allocated via alloc_page() in the order
given. Thus, we do not support non consecutive physical memory.

And when it is vmap()'d it's still just order of zero, but shouldn't be
an issue even if it wasn't zero as it still is contiguous memory. The
only reason we have vmap() is because the reserved memory code returns
a physical location and not a virtual one.

> 
> Hint: those two things do not go together.
> 
> I'm *hoping* that subbuf_order is always zero for anything that has
> been allocated with vmalloc(), and that this is another of those "it
> works, even though it looks like crap".
> 
> But my point is that the code shouldn't be written in a way where it
> just *happens* to work, and "hoping" is not a strategy for long-term
> maintenance.
> 
> IOW, you should not have code that first says "I have no clue how this
> was allocated, so I'll ask the VM", and then in the very next breath
> says "oh, I'll just treat this as a physically contiguous allocation
> even if it was a vmalloc address".

We act as it is physically contiguous because that's all that is supported.

Now I agree that it isn't the best design to have the tracing code map
the physical address to virtual and pass that in to the ring buffer
allocator. Instead, it should pass in the contiguous physical memory
and have the ring buffer map it to the virtual address. That way
there's no chance of passing in vmalloc memory that is not contiguous.
We could set a flag that states if it was allocated via alloc_page or
was mapped by physical memory and use that flag to determine how to get
to the page. There would be no "hope this is contiguous" issue then.

-- Steve

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 18:24, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> The pages are never vmalloc'd, it's only ever vmap()'d on top of
> contiguous physical memory or allocated via alloc_page() in the order
> given. Thus, we do not support non consecutive physical memory.

Christ, that just makes it EVEN WORSE.

Just keep track of the actual original physical allocation, then!

By all means vmap it too for whoever wants the virtual allocation, but
remember the *real* allocation, and keep it as a 'struct page'
together with the order that you already have.

And then you never use vmalloc_to_page() - or even virt_to_page() - at
all, because you actually know your base allocation, and keep it in
that form that so much of this code wants in the first place.

Having a a nice reliable 'struct page *' (together with that size
order) and keeping it in that form would be *so* much cleaner.

Instead of randomly translating it to (two different!) kinds of
virtual kernel addresses and then translating it back when you wanted
the original proper format.

The random crazy hackery needs to stop. Really.

               Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 18:31:55 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 27 Mar 2025 at 18:24, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > The pages are never vmalloc'd, it's only ever vmap()'d on top of
> > contiguous physical memory or allocated via alloc_page() in the order
> > given. Thus, we do not support non consecutive physical memory.  
> 
> Christ, that just makes it EVEN WORSE.\

Let me explain this better. Yes it uses alloc_page() but that's just an
intermediate function. What is saved is the page_address(page), as that
is what is used by the code. That is, the allocation uses alloc_page()
but then immediately converted to page_address() to use. The page
itself is just an temp variable to get contiguous memory, as the
sub-buffers must be contiguous.

> 
> Just keep track of the actual original physical allocation, then!
> 
> By all means vmap it too for whoever wants the virtual allocation, but
> remember the *real* allocation, and keep it as a 'struct page'
> together with the order that you already have.

The virtual address needs to be created as that's all the code cares
about. For the case where the physical memory is passed in, it requires
a vmap(). The struct page is only needed when the ring buffer is going
to be mmapped to user space.

> 
> And then you never use vmalloc_to_page() - or even virt_to_page() - at
> all, because you actually know your base allocation, and keep it in
> that form that so much of this code wants in the first place.
> 
> Having a a nice reliable 'struct page *' (together with that size
> order) and keeping it in that form would be *so* much cleaner.
> 
> Instead of randomly translating it to (two different!) kinds of
> virtual kernel addresses and then translating it back when you wanted
> the original proper format.

By moving the physical mapping of the code into the ring buffer code, I
can set a flag for the buffer to mark it as being mapped directly, and
we can save the struct pages for this location (easily found by the
address). Then to get the page, it could be a simple helper function of:

struct page *rb_get_page(struct trace_buffer *buffer, unsigned long addr)
{
	if (buffer->flags & RB_FL_PHYSICAL) {
		addr -= buffer->vmap_start;
		addr += buffer->phys_start;
		return pfn_to_page(addr >> PAGE_SHIFT);
	}
	return virt_to_page(addr);
}

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 19:01, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Let me explain this better. Yes it uses alloc_page() but that's just an
> intermediate function. What is saved is the page_address(page), as that
> is what is used by the code.

I understood.

But NO, that is *not* what is used by the code.

You literally use "struct page *" whenever you want to mmap it.

If the *only* thing that was used was the virtual address, this
wouldn't be a discussion.

As it is, you allocate it in form A, then you turn it into *either*
form B or C, and then when you need form A again you turn it back. And
you do so in a particularly disgusting and random way.

> The virtual address needs to be created as that's all the code cares
> about.

Stop LYING.

If that was "all the code cared about" we'd not have this discussion.

It clearly wants things *back* as 'struct page' again. Stop making
shit up and maki9ng excuses for your bad code.

And yes, it would probably be fine to just keep it as *both* in parallel.

But honestly, if you only save one thing - as you currently do - you
should save the thing that is more fundamental.

That's 'struct page *'. That's the allocation, that's the thing you
need for mmap, and that's what you *should* have used for
de-allocation too (even if I think you currently just end up using the
virtual mapping and depend on the VM code undoing your mapping).

Anyway, I'm not arguing any more. I'm telling you that this code will

 (a) not be pulled this merge window

 (b) not be pulled EVER unless you clean it up

 (c) you need to stop with this hackery

Apparently you knew it was ugly hackery, since you said "I figured
this would be the most controversial", so it wasn't a surprise to you
that I'm complaining.

Why did you send it then? Because I'm serious - this has been a
pattern, and I  AM DONE.

Next time, I won't bother to explain why I'm not pulling, because
apparently even when I explain, you just start making excuses and
making more garbage up.

Next time, you're going in the spam filter.

Because it's annoying that I have to go through your pull requests so
carefully, but what makes me truly fed up is that this KEEPS
HAPPENING, and then when I complain you just start arguing about it
even though you seem to have been aware that the code was shit.

            Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 19:17, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But NO, that is *not* what is used by the code.
>
> You literally use "struct page *" whenever you want to mmap it.
>
> If the *only* thing that was used was the virtual address, this
> wouldn't be a discussion.

Just to clarify: if you don't mmap this into user space, then that's
fine. Then you can keep just the kernel virtual address.

And I already told you that that is one option: just don't mmap.

But as long as you insist on mmaping the buffer into user space, you
follow the rules. And the rules are that you don't play games. You do
this RIGHT, or you don't do it at all.

And you don't lie about things and claim that the only thing that is
used is the kernel virtual address.

Comprende?

                 Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 19:17:34 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 27 Mar 2025 at 19:01, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Let me explain this better. Yes it uses alloc_page() but that's just an
> > intermediate function. What is saved is the page_address(page), as that
> > is what is used by the code.  
> 
> I understood.
> 
> But NO, that is *not* what is used by the code.
> 
> You literally use "struct page *" whenever you want to mmap it.

When we want to map it to user space, which was added in 6.10. Before
that struct page was hardly used.

> 
> If the *only* thing that was used was the virtual address, this
> wouldn't be a discussion.
> 
> As it is, you allocate it in form A, then you turn it into *either*
> form B or C, and then when you need form A again you turn it back. And
> you do so in a particularly disgusting and random way.
> 
> > The virtual address needs to be created as that's all the code cares
> > about.  
> 
> Stop LYING.

I'M NOT LYING, and honestly I'm quite offended that you accused me of
lying!

I make it a point to point out very clearly what I'm doing and have
never tried to sneak something past you. That's why our arguments have
been from the cover letter and not the patches themselves.

> 
> If that was "all the code cared about" we'd not have this discussion.
> 
> It clearly wants things *back* as 'struct page' again. Stop making
> shit up and maki9ng excuses for your bad code.

Before 6.10 (from 2.6.28 through to 6.9), the only uses of struct page
was in the allocation and freeing of the ring buffer. It wasn't used
before that. The virtual address is what the kernel writes to, it's
what was read.

We just started allowing the ring buffer to be memory mapped by user
space in 6.10. But that's not even the main way the buffer is read.
Most of the time, the buffer is read by the "trace" file and that is
where the kernel translates the buffer into human readable text. That
most definitely doesn't need the struct page.

Note, in the splice case, the tracing code does convert it back to a
struct page. But if you do splice on a memory mapped buffer, the ring
buffer code allocates a new page and copies it, as the memory mapped
buffer can not be passed to splice as that requires giving up ownership.

But when you read the "trace" or "trace_pipe" file, or even read the
"trace_pipe_raw", the struct page is never used.

> 
> And yes, it would probably be fine to just keep it as *both* in parallel.
> 
> But honestly, if you only save one thing - as you currently do - you
> should save the thing that is more fundamental.
> 
> That's 'struct page *'. That's the allocation, that's the thing you
> need for mmap, and that's what you *should* have used for
> de-allocation too (even if I think you currently just end up using the
> virtual mapping and depend on the VM code undoing your mapping).
> 
> Anyway, I'm not arguing any more. I'm telling you that this code will
> 
>  (a) not be pulled this merge window
> 
>  (b) not be pulled EVER unless you clean it up
> 
>  (c) you need to stop with this hackery
> 
> Apparently you knew it was ugly hackery, since you said "I figured
> this would be the most controversial", so it wasn't a surprise to you
> that I'm complaining.

Well, it wasn't that I thought it was hackery, it was that it feels like
you are looking for anything to yell at me for at every pull request I
do. And this one I figured you would find something with.

> 
> Why did you send it then? Because I'm serious - this has been a
> pattern, and I  AM DONE.

Because I didn't feel it was as hackery as you do. I'm not even doing
this work alone. I even talk with the memory management folks. They are
not telling me it's hackery.

> 
> Next time, I won't bother to explain why I'm not pulling, because
> apparently even when I explain, you just start making excuses and
> making more garbage up.
> 
> Next time, you're going in the spam filter.
> 
> Because it's annoying that I have to go through your pull requests so
> carefully, but what makes me truly fed up is that this KEEPS
> HAPPENING, and then when I complain you just start arguing about it
> even though you seem to have been aware that the code was shit.

I'm sorry that you feel this way, perhaps I should quit and find
another profession :-p

-- Steve

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 19:19:33 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 27 Mar 2025 at 19:17, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > But NO, that is *not* what is used by the code.
> >
> > You literally use "struct page *" whenever you want to mmap it.
> >
> > If the *only* thing that was used was the virtual address, this
> > wouldn't be a discussion.  
> 
> Just to clarify: if you don't mmap this into user space, then that's
> fine. Then you can keep just the kernel virtual address.
> 
> And I already told you that that is one option: just don't mmap.

When I found that the physical memory mapped code when mapped to user
space was causing crashes as the virt_to_page() code didn't work with
vmap(), that was exactly what I did, and mentioned that it would be an
easy fix, but I guess not in a way you liked.

 https://lore.kernel.org/all/20250215144719.404616dc@batman.local.home/

- Prevent mmap()ing persistent ring buffer

  The persistent ring buffer uses vmap() to map the persistent memory.
  Currently, the mmap() logic only uses virt_to_page() to get the page
  from the ring buffer memory and use that to map to user space. This works
  because a normal ring buffer uses alloc_page() to allocate its memory.
  But because the persistent ring buffer use vmap() it causes a kernel
  crash.  Fixing this to work with vmap() is not hard, but since mmap() on
  persistent memory buffers never worked, just have the mmap() return
  -ENODEV (what was returned before mmap() for persistent memory ring
  buffers, as they never supported mmap. Normal buffers will still allow
  mmap(). Implementing mmap() for persistent memory ring buffers can wait
  till the next merge window.

> 
> But as long as you insist on mmaping the buffer into user space, you
> follow the rules. And the rules are that you don't play games. You do
> this RIGHT, or you don't do it at all.

Are you OK with the idea of moving the mapping code into the ring
buffer code (which I think is cleaner) and then keeping track of that
to get to the pages with the helper function?

struct page *rb_get_page(struct trace_buffer *buffer, unsigned long addr)
{
	if (buffer->flags & RB_FL_PHYSICAL) {
		addr -= buffer->vmap_start;
		addr += buffer->phys_start;
		return pfn_to_page(addr >> PAGE_SHIFT);
	}
	return virt_to_page(addr);
}

It doesn't keep track of the pages themselves, as then I would need to
keep track of both the virtual and physical addresses. But since the
pages are either allocated via page_alloc() and converted by
page_address(), or is mapped on a physically contiguous memory area,
where the physical address can be easily calculated.

Or is this too hacky for you?

> 
> And you don't lie about things and claim that the only thing that is
> used is the kernel virtual address.

I didn't say the "only thing" I said most of the code. The memory
mapping to user space is very new, and used for tooling that is doing
streaming, where it consumes the data without saving it. It is only
using it for calculations, where memory mapping is faster than reading
as is doesn't have the overhead of a copy. But as this is new, it feels
like an exception and not the rule. Hence, why I said most of the code
doesn't care about struct page. Maybe you see this as lying, but to me,
it's simply my point of view.

-- Steve

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

 On Thu, 27 Mar 2025 at 19:44, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> When we want to map it to user space, which was added in 6.10. Before
> that struct page was hardly used.

Right. That *used* to be the situation. But that's no longer the
situation since you want to mmap things.

Again: if you remove mmap(), all this goes away.

That's an option. But instead, you added *more* mmap, and made the
problem acute by also making the code much uglier.

What used to be a small and fairly harmless hack is now ugly beyond
belief and fragile.

> I'M NOT LYING, and honestly I'm quite offended that you accused me of
> lying!

What's the definition of lying again? Saying something that wasn't
true? That's literally what you did. You claimed only the virtual
address was used. That's *CLEARLY* not true.

The fact that it *used* to be true, and that the code was originally
designed in a time when it was true is really not relevant to the
current situation, now is it?

> Before 6.10 (from 2.6.28 through to 6.9), the only uses of struct page
> was in the allocation and freeing of the ring buffer. It wasn't used
> before that. The virtual address is what the kernel writes to, it's
> what was read.

What the hell is your point?

Bringing up something that *used* to be true, but isn't any longer true?

Seriously, what's the point of that?

So face the music. The fact that you mmap the thing means that now it
needs 'struct page'. Stop making excuses, stop making things up, and
just deal with it.

or don;t.

> But when you read the "trace" or "trace_pipe" file, or even read the
> "trace_pipe_raw", the struct page is never used.

Again, WHAT IS YOUR POINT?

Those aren't the issues. The issue is that you added mmap. Stop
bringing up completely irrelevant issues. Stop saying "but but but
other code doesn't need it".

The code directly under discussion clearly does in fact need it and use it.

> Well, it wasn't that I thought it was hackery, it was that it feels like
> you are looking for anything to yell at me for at every pull request I
> do. And this one I figured you would find something with.

So next time, make sure it's clean. You clearly knew that code was questionable.

And yes, you also knew very well that I look at your pull requests
more carefully. I've made that clear before. There's a history to your
pull requests, and now we're dealing with that history.

People who don't cause issues have an easier time, because I assume
that they will *continue* to not cause issues. I've made that very
clear. It means that you need to be more careful than the average
bear.

So when you go "this will cause controversy", you should take a step
back, and think about how you can *avoid* writing things in an ugly
manner.

Instead you just went for it, and here we are.

So for next time, just don't repeat the same ugly hackery five times
in a row, for one thing. If that code had been *clean* code, I would
almost certainly not have reacted to the nasty "translate pointers
back and forth".

But as it is, instead of having some clear abstractions and helper
functions to do things in one place with sane tests for "if this is a
,TRACE_ARRAY_FL_BOOT thing, we have a clearly separate path" you
repeated the disgusting hackery overr and overr and made sure it was
front and center.

Now it's too late. I've seen that horror, and I'm not willing to take it.

You need to keep the 'struct page' around for mmap. Instead of living
in the past and saying "before mmap, we didn't need it", just realize
that the "before mmap" situation is not relevant any more. We're not
there.

Or just say "mmap just isn't worth the pain of cleaning up this code".

That's ok too.

                Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 20:01, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> Are you OK with the idea of moving the mapping code into the ring
> buffer code (which I think is cleaner) and then keeping track of that
> to get to the pages with the helper function?
>
> struct page *rb_get_page(struct trace_buffer *buffer, unsigned long addr)
> {
>         if (buffer->flags & RB_FL_PHYSICAL) {
>                 addr -= buffer->vmap_start;
>                 addr += buffer->phys_start;
>                 return pfn_to_page(addr >> PAGE_SHIFT);
>         }
>         return virt_to_page(addr);
> }

I think that would have been *enormously* better, yes, if only because
it clearly abstracts things out, and doesn't do the whole "oh, let's
ask what the VM thinks this page is" which really drove me up the
wall.

It may still be based on kernel virtual addresses (and that
"virt_to_page()" looks odd - you can't give it an 'unsigned long', so
I assume this was untested pseudo-code), but now that code at least
makes it very clear that it knows what the addresses are supposed to
be.

And it also makes it clear that both cases are actually based on
underlying contiguous physical addresses just using different virtual
mapping strategies. So now the "page++" thing I complained about is
suddenly not crazy code any more.

Plus the RB_FL_PHYSICAL thing is not just clearer semantically, it
happens to be about a million times faster than doing
"vmalloc_to_page()" (ok, that's obvious hyperbole, but
vmalloc_to_page() kind of a pig and looks things up in the page tables
because that's what it is designed for).

So yes. The kernel virtual address scheme can probably be salvaged.
But it needs doing things *right*, not hackery.

            Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 20:05:14 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > I'M NOT LYING, and honestly I'm quite offended that you accused me of
> > lying!  
> 
> What's the definition of lying again? Saying something that wasn't
> true? That's literally what you did. You claimed only the virtual
> address was used. That's *CLEARLY* not true.

This is more of a confusing of the words. I did say "that's all the
code cares about", I meant the code that is used for recording and
reporting the events. I thought it was obvious that I didn't mean the
mmapped code. It wasn't lying, it was a miscommunication.

> 
> > Before 6.10 (from 2.6.28 through to 6.9), the only uses of struct page
> > was in the allocation and freeing of the ring buffer. It wasn't used
> > before that. The virtual address is what the kernel writes to, it's
> > what was read.  
> 
> What the hell is your point?
> 
> Bringing up something that *used* to be true, but isn't any longer true?

My point was that I can't *just* use the struct page. It sounds to me
that you want me to to refactor all of the code to handle struct page
even when the consumer is in the kernel.

> 
> So face the music. The fact that you mmap the thing means that now it
> needs 'struct page'. Stop making excuses, stop making things up, and
> just deal with it.

Again, are you suggesting that the code needs to be converted to using
struct page for all of it? Even for the kernel consumer code?

> 
> or don;t.
> 
> > But when you read the "trace" or "trace_pipe" file, or even read the
> > "trace_pipe_raw", the struct page is never used.  
> 
> Again, WHAT IS YOUR POINT?
> 
> Those aren't the issues. The issue is that you added mmap. Stop
> bringing up completely irrelevant issues. Stop saying "but but but
> other code doesn't need it".
> 
> The code directly under discussion clearly does in fact need it and use it.

I guess, are you asking to save the struct page for every allocation,
and not using virt_to_page() to get it? In other words, do I need to
increase the memory footprint to add another pointer because the code
can be mmapped?

	page = page_alloc();
	bpage->page = page_address(page);
	bpage->struct_page = page;

??

> 
> So when you go "this will cause controversy", you should take a step
> back, and think about how you can *avoid* writing things in an ugly
> manner.

Because this was the code we last had an argument about, which is why I
figured it would be "controversial". You don't need to read more into
it than that.

> 
> Instead you just went for it, and here we are.

Again, it wasn't because I thought it was hackery, but because this is
the code we last argued about. Hence, why I thought it was the most
"controversial".

> 
> So for next time, just don't repeat the same ugly hackery five times
> in a row, for one thing. If that code had been *clean* code, I would
> almost certainly not have reacted to the nasty "translate pointers
> back and forth".
> 
> But as it is, instead of having some clear abstractions and helper
> functions to do things in one place with sane tests for "if this is a
> ,TRACE_ARRAY_FL_BOOT thing, we have a clearly separate path" you
> repeated the disgusting hackery overr and overr and made sure it was
> front and center.
> 
> Now it's too late. I've seen that horror, and I'm not willing to take it.
> 
> You need to keep the 'struct page' around for mmap. Instead of living
> in the past and saying "before mmap, we didn't need it", just realize
> that the "before mmap" situation is not relevant any more. We're not
> there.
> 
> Or just say "mmap just isn't worth the pain of cleaning up this code".

So you are saying that you want this to save the page struct directly
then. My only concern with that is the added memory to save the
structure when it could be easily calculated.

As I work on memory limited devices, I try not to waste memory when
there's another way to get what is needed.

-- Steve

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 20:19:09 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I think that would have been *enormously* better, yes, if only because
> it clearly abstracts things out, and doesn't do the whole "oh, let's
> ask what the VM thinks this page is" which really drove me up the
> wall.

Thanks for verifying. I'll update the code to do this, and get it ready
for 6.16, and hopefully there will be nothing in there that you will
think is "hackery".

-- Steve

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 20:19, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, 27 Mar 2025 at 20:01, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > Are you OK with the idea of moving the mapping code into the ring
> > buffer code (which I think is cleaner) and then keeping track of that
> > to get to the pages with the helper function?
> >
> > struct page *rb_get_page(struct trace_buffer *buffer, unsigned long addr)
> > {
> >         if (buffer->flags & RB_FL_PHYSICAL) {
> >                 addr -= buffer->vmap_start;
> >                 addr += buffer->phys_start;
> >                 return pfn_to_page(addr >> PAGE_SHIFT);
> >         }
> >         return virt_to_page(addr);
> > }
>
> I think that would have been *enormously* better, yes

.. actually, thinking a bit more about it, I think you want another
level of abstraction to deal with the chunking side (ie I think you
want to make the subbuf_ids[] thing explicit and the 'page order' as
well).

So you have that 'meta_buffer' thing for the first page, but you also
have that subbuf_ids[] array with pages chunked by 'order'), and I
think you need another level of abstraction for those.

IOW, you'd have 'rb_get_meta_buffer_page()' for the first page, and
then something like 'rb_get_buffer_page()' for the subbuf_id[] thing.

Or something in that direction. I didn't look *too* closely at what
the code actually needs, I'm just trying to make sure that you don't
pass random kernel addresses around, but something more structured.

Preferably maybe just the page index *within* the ring buffer,
perhaps, and have the helper function do the whole "is this the meta
buffer or one of the subbuf_id things"?

Hmm?

More clear abstraction is what I'm looking for, in other words.

              Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Linus Torvalds]

On Thu, 27 Mar 2025 at 20:27, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> So you are saying that you want this to save the page struct directly
> then. My only concern with that is the added memory to save the
> structure when it could be easily calculated.

I think with the right amount of clean abstraction work, you can
probably salvage this without having to do the double accounting.

Because I did like your rb_get_page() concept at least in that pseudo-code form.

I don't know what the actual tested end result looks like, maybe there
ends up being problems with it..

         Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

Adding Vincent who did the user space memory mapping code, and is
working on having pKVM hyperviser mapping with this as well.

Some background too. The user space memory mapping work came out of a
discussion between Vincent and I at the tracing summit in 2023. Where
pKVM adds a hypervisor context that is not the kernel. It acts as a go
between of the kernel (host) and the guest, were the kernel does not
have access to the guest memory, nor to the hypervisor memory. But just
like user space, memory between the kernel and the hypervisor can be
mapped to each.

As the hypervisor is a black box, Vincent was looking for a way to add
tracing to it where the kernel can't manipulate the pages but can read
the hypervisor trace content. When discussing this, we found that this
had the same requirements as memory mapping the buffer with user space,
and since that was a long time request for tooling that only read the
buffer and never saved it, we decided to do the work with user space
and eventual move it to the pKVM hypervisor.

The code is going to be split up a bit to allow the hypervisor to use
some of it. That was destined for 6.16 as well.

Since Vincent has working code for pKVM, and this may change, not to
mention that he's the original author of this code, it would be good
for him to see if he can also clean up this code in the process ;-)

Vincent,

The code for memory mapping needs to be cleaned up a bit. Namely, the
fact that we have two ways to map the ring buffer (one using the normal
kernel allocation the other with physically mapped memory). Ideally we
should keep track of the pages a bit better. I can fix up the
physically mapped memory part, but I'm not sure how this is going to
affect the pKVM changes you are making.

-- Steve


On Thu, 27 Mar 2025 20:36:00 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, 27 Mar 2025 at 20:19, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Thu, 27 Mar 2025 at 20:01, Steven Rostedt <rostedt@goodmis.org> wrote:  
> > >
> > > Are you OK with the idea of moving the mapping code into the ring
> > > buffer code (which I think is cleaner) and then keeping track of that
> > > to get to the pages with the helper function?
> > >
> > > struct page *rb_get_page(struct trace_buffer *buffer, unsigned long addr)
> > > {
> > >         if (buffer->flags & RB_FL_PHYSICAL) {
> > >                 addr -= buffer->vmap_start;
> > >                 addr += buffer->phys_start;
> > >                 return pfn_to_page(addr >> PAGE_SHIFT);
> > >         }
> > >         return virt_to_page(addr);
> > > }  
> >
> > I think that would have been *enormously* better, yes  
> 
> .. actually, thinking a bit more about it, I think you want another
> level of abstraction to deal with the chunking side (ie I think you
> want to make the subbuf_ids[] thing explicit and the 'page order' as
> well).
> 
> So you have that 'meta_buffer' thing for the first page, but you also
> have that subbuf_ids[] array with pages chunked by 'order'), and I
> think you need another level of abstraction for those.
> 
> IOW, you'd have 'rb_get_meta_buffer_page()' for the first page, and
> then something like 'rb_get_buffer_page()' for the subbuf_id[] thing.
> 
> Or something in that direction. I didn't look *too* closely at what
> the code actually needs, I'm just trying to make sure that you don't
> pass random kernel addresses around, but something more structured.
> 
> Preferably maybe just the page index *within* the ring buffer,
> perhaps, and have the helper function do the whole "is this the meta
> buffer or one of the subbuf_id things"?
> 
> Hmm?
> 
> More clear abstraction is what I'm looking for, in other words.
> 
>               Linus

Re: [GIT PULL] ring-buffer: Updates for v6.15

[Steven Rostedt]

On Thu, 27 Mar 2025 19:17:34 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Anyway, I'm not arguing any more. I'm telling you that this code will
> 
>  (a) not be pulled this merge window
> 
>  (b) not be pulled EVER unless you clean it up
> 
>  (c) you need to stop with this hackery

This entire debate was on the very first commit that allowed the persistent
ring buffer to be memory mapped by user space, which introduced the:

	if (virt_addr_valid((void *)cpu_buffer->subbuf_ids[s]))
		page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]);
	else
		page = vmalloc_to_page((void *)cpu_buffer->subbuf_ids[s]);

"hack" that you hate.

This commit was the first thing added as I wrote it right after fixing the
issue that caused it to crash when user space tried to map it. I basically
fixed it and forgot about it (which is why there were 5 instances of that
code, where I did plan on at least wrapping it, but forgot).

Most of the work was Masami's saving the module addresses in the persistent
ring buffer so that it can be retrieved after a crash. That code is totally
unrelated to this.

I just did a rebase removing this commit and one commit that fixed not
adding a typecast of a long to a pointer in virt_addr_valid() (yes, my
pseudo code had the same bug with the virt_to_page() but I didn't compile
it, I actually wrote that directly in the email).

No other commit depends on this commit. Are you fine with the rest of the
pull request? I can start running it through my tests without these changes.

-- Steve