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

[Steven Rostedt]

On Thu, 27 Mar 2025 17:59:54 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Thu, 27 Mar 2025 at 14:09, Steven Rostedt <rostedt@xxxxxxxxxxx> 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