Message-ID: <76333f65-c2c0-47bc-94f7-8f18e60def30@efficios.com>
Date: Fri, 19 Jul 2024 14:05:39 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Steven Rostedt <rostedt@...dmis.org>
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 2024-07-19 12:19, Steven Rostedt wrote:
> 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.

Then the commit message is misrepresenting the usefulness of the
feature. What is the use-case for specifying the location of the
ring buffer address space in physical memory beyond re-accessing
it after reboot ? What is that special hardware you are referring
to ?

> 
>>
>> 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.

I should have used filesystem terminology here rather than ABI. This
ring buffer memory area is similar to a filesystem on-disk format,
because it needs to be agreed upon by two distinct kernel instances.

I'm pretty sure there are inherent expectations about not breaking
file systems.

IMHO it makes it even more important to clearly document the guarantees
given about it or lack thereof.

> 
>>
>> 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.

You are right, it's not about userspace. It is about the in-memory
equivalent of an on-disk format, and it should be documented.

> 
>>
>> 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?

No argument there, it's not about userspace applications. It's the
in-memory equivalent of an "on-disk format".

> It's a kernel command line telling the
> kernel to place the ring buffer at some specific address space. Nothing
> more.

The whole justification for adding this feature *is* keeping a ring
buffer around across kernel reboots. If it's just a way to let the
kernel specify where to place the ring buffer in memory, then what
is the use-case justifying its integration ?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Powered by blists - more mailing lists