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

[Linus Torvalds]

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.

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