Received-SPF: pass (google.com: domain of linux-kernel+bounces-35835-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 23 Jan 2024 13:02:25 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Vincent Donnefort <vdonnefort@google.com>
Cc: mhiramat@kernel.org, linux-kernel@vger.kernel.org,
 linux-trace-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
 kernel-team@android.com
Subject: Re: [PATCH v12 2/6] ring-buffer: Introducing ring-buffer mapping
 functions
Message-ID: <20240123130225.23d5da07@gandalf.local.home>
In-Reply-To: <Za_74T-IJWAa6fny@google.com>
References: <20240123110757.3657908-1-vdonnefort@google.com>
	<20240123110757.3657908-3-vdonnefort@google.com>
	<20240123105149.36abf019@gandalf.local.home>
	<Za_74T-IJWAa6fny@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 23 Jan 2024 17:48:17 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:

> > > + * @subbufs_touched:	Number of subbufs that have been filled.
> > > + * @subbufs_lost:	Number of subbufs lost to overrun.
> > > + * @subbufs_read:	Number of subbufs that have been read.  
> > 
> > Do we actually need the above 3 fields?
> > 
> > What's the use case for them? I don't want to expose internals in the API
> > unless they are needed.  
> 
> subbufs_read is gone, I just forgot to remove it here :-\.
> 
> The two other ones are used for tracing with the hypervisor. That's why I
> preemptively added them. 
> 
> I can remove them and add just append this struct later ... or just overload
> this struct with another one, only shared between the kernel and the hypervisor?

Yes, let's have them be different for now. Or we could just make them
"reserved" and say that they are not used for now.

I also think we should add a flags value too, so that when they are no
longer reserved, a flag can be set to say that they are active.

struct trace_buffer_meta {
	__u32		meta_page_size;
	__u32		meta_struct_len;

	__u32		subbuf_size;
	__u32		nr_subbufs;

	struct {
		__u64	lost_events;
		__u32	id;
		__u32	read;
	} reader;

	__u64	flags;

	__u64	entries;
	__u64	overrun;
	__u64	read;

	__u64	Reserved1;
	__u64	Reserved2;
};


And that way the hypervisor would still have those available, and updated,
but they are not filled for user space. The hypervisor could even set the
flag field saying it's using them.

That is, the first flag is:

enum {
	TRACE_BUFFER_META_FL_SUBUF_INFO		= (1 << 0),
};

And if that's set, those fields are available. But we just don't set them
now ;-)

-- Steve