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

Steven Rostedt
2024-01-09T16:41:03Z

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