Received-SPF: pass (google.com: domain of linux-kernel+bounces-21118-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 9 Jan 2024 11:42:00 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Vincent Donnefort <vdonnefort@google.com>
Cc: Masami Hiramatsu <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 v10 1/2] ring-buffer: Introducing ring-buffer mapping
 functions
Message-ID: <20240109114200.01367342@gandalf.local.home>
In-Reply-To: <ZZ1ir0edlY3OzjyC@google.com>
References: <20240105094729.2363579-1-vdonnefort@google.com>
	<20240105094729.2363579-2-vdonnefort@google.com>
	<20240109234230.e99da87104d58fee59ad75c6@kernel.org>
	<ZZ1ir0edlY3OzjyC@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Tue, 9 Jan 2024 15:13:51 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:

> > > @@ -388,6 +389,7 @@ struct rb_irq_work {
> > >  	bool				waiters_pending;
> > >  	bool				full_waiters_pending;
> > >  	bool				wakeup_full;
> > > +	bool				is_cpu_buffer;  
> > 
> > I think 'is_cpu_buffer' is a bit unclear (or generic),
> > what about 'meta_page_update'?  
> 
> Hum not sure about that change. This was really to identify if parent of
> rb_irq_work is a cpu_buffer or a trace_buffer. It can be a cpu_buffer regardless
> of the need to update the meta-page.

Yeah, this was added because the irq_work is called with the rb_work for
both the cpu_buffer and the global struct tracing_buffer object. The
meta_page is only available to the cpu_buffer and does not exist on the
struct trace_buffer, so this is checked before doing a "container_of()" on
the wrong structure. Both the cpu_buffer and the global buffer call the
same function with the rb_work structure.

So "is_cpu_buffer" is the right terminology as it's unrelated to the meta page:

struct trace_buffer {
	[..]
	struct rb_irq_work	irq_work;
	[..]
};

	struct trace_buffer *buffer;
	buffer->irq_work.is_cpu_buffer = false;

struct ring_buffer_per_cpu {
	[..]
	struct rb_irq_work	irq_work;
	[..]
}

	struct ring_buffer_per_cpu *cpu_buffer;
	cpu_buffer->irq_work.is_cpu_buffer = true;


[..]
	init_irq_work(&buffer->irq_work.work, rb_wake_up_waiters);
[..]
	init_irq_work(&cpu_buffer->irq_work.work, rb_wake_up_waiters);

// both the buffer and cpu_buffer call rb_wake_up_waiters()

static void rb_wake_up_waiters(struct irq_work *work)
{
	struct rb_irq_work *rbwork = container_of(work, struct rb_irq_work, work);

// This container_of() gets rbwork which is the rb_irq_work structure that
// both buffer and cpu_buffer have.

	if (rbwork->is_cpu_buffer) {
		struct ring_buffer_per_cpu *cpu_buffer;

		cpu_buffer = container_of(rbwork, struct ring_buffer_per_cpu,
					  irq_work);

// The above crashes if done by the buffer and not the cpu_buffer.
// The "is_cpu_buffer" is there to differentiate the two rb_work entities.
// It is a way to say this is safe to do the above "container_of()".

		/*
		 * If the waiter is a cpu_buffer, this might be due to a
		 * userspace mapping. Let's update the meta-page.
		 */
		rb_update_meta_page(cpu_buffer);
	}


-- Steve