Message-ID: <20240719121940.5ce9a90a@rorschach.local.home>
Date: Fri, 19 Jul 2024 12:19:40 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, LKML
 <linux-kernel@...r.kernel.org>, Masami Hiramatsu <mhiramat@...nel.org>, Dan
 Carpenter <dan.carpenter@...aro.org>, Thorsten Blum
 <thorsten.blum@...lux.com>
Subject: Re: [GIT PULL] ring-buffer: Updates for 6.11

On Fri, 19 Jul 2024 10:59:37 -0400
Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> > So I went to update this, and realized there's no place that actually
> > mentions anything about this being used across reboots (besides in the
> > change logs). The only documentation is in kernel-parameters.txt:
> > 
> >                          If memory has been reserved (see memmap for x86), the instance
> >                          can use that memory:
> >                          
> >                                  memmap=12M$0x284500000 trace_instance=boot_map@...84500000:12M
> > 
> >                          The above will create a "boot_map" instance that uses the physical
> >                          memory at 0x284500000 that is 12Megs. The per CPU buffers of that
> >                          instance will be split up accordingly.
> > 
> > I do plan on adding more documentation on the use cases of this, but I
> > was planning on doing that in the next merge window when the
> > reserve_mem kernel parameter can be used. Right now, we only document
> > what it does, and not what it is used for.
> > 
> > Do you still have an issue with that part missing? No where does it
> > mention that this is used across boots. There is a file created in the
> > boot mapped instance directory that hints to the usage
> > (last_boot_info), but still there's no assumptions made.
> > 
> > In other words, why document a restriction on something that hasn't
> > been documented?  
> 
> AFAIU the intended use of this feature is to convey trace buffer
> data from one kernel to the next across a reboot, which makes it
> a whole different/new kind of ABI.

That may be my intention, but there's nothing here to imply that. In
fact, after I read the document, it looks to me as a way to simply
designate a location of address space for the ring buffer. This could
be because of some special hardware. Nothing here says "exists on
reboot". Thinking that this implies that the ring buffer will last
across boots is not going to make it a new API. In fact, in my test
cases, it fails (due to KASLR) approximately once every 5 boots. So
it's not something you can even rely on.

> 
> Having no documentation will not stop anyone from using this new
> feature and make assumptions about ABI guarantees. I am concerned
> that this ABI ends up being defined by accident/misuses rather than
> by design if it is merged without documentation.

This is not an ABI. Remember, Linus's mandate is to "not break user
space". There's no use space involved here.

> 
> Very often when I find myself documenting a feature, I look back at
> the user-facing result and modify the ABI where it does not make
> sense. Merging this ABI without documentation prevents that.

Again, this isn't a "user facing feature", where I consider "user"
being user space.

> 
> So if you ask my honest opinion there, I would say that merging this
> ABI without documentation feels rushed.

It's not an ABI (Application Binary Interface). What's the application?
What's the binary interface? It's a kernel command line telling the
kernel to place the ring buffer at some specific address space. Nothing
more.

-- Steve

Powered by blists - more mailing lists