Re: [PATCH v10 1/2] ring-buffer: Introducing ring-buffer mapping functions

[Steven Rostedt <rostedt@goodmis.org>]

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