On Thu, 6 Feb 2025 14:10:35 +0900
Masami Hiramatsu (Google) <mhira...@kernel.org> wrote:
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 7146b780176f..0446d053dbd6 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -49,7 +49,12 @@ static void update_pages_handler(struct work_struct
> > *work);
> >
>
> Can you add a comment to explain the layout of these meta-data and subbufs?
Sure.
>
>
> > struct ring_buffer_meta {
> > int magic;
> > - int struct_size;
> > + int struct_sizes;
> > + unsigned long total_size;
> > + unsigned long buffers_offset;
> > +};
> > +
> > @@ -1664,14 +1674,73 @@ static void *rb_range_buffer(struct
> > ring_buffer_per_cpu *cpu_buffer, int idx)
> > return (void *)ptr;
> > }
> >
> > +/*
> > + * See if the existing memory contains a valid meta section.
> > + * if so, use that, otherwise initialize it.
>
> Note that this must be called only for the persistent ring buffer.
Or just make it return false if bmeta is NULL.
The range_addr_start is only set for persistent ring buffers.
Also, this is to initialize a persistent ring buffer meta data, which makes
calling it for anything else rather strange.
>
> > + */
> > +static bool rb_meta_init(struct trace_buffer *buffer)
> > +{
> > + unsigned long ptr = buffer->range_addr_start;
> > + struct ring_buffer_meta *bmeta;
> > + unsigned long total_size;
> > + int struct_sizes;
> > +
> > + bmeta = (struct ring_buffer_meta *)ptr;
> > + buffer->meta = bmeta;
> > +
> > + total_size = buffer->range_addr_end - buffer->range_addr_start;
> > +
> > + struct_sizes = sizeof(struct ring_buffer_cpu_meta);
> > + struct_sizes |= sizeof(*bmeta) << 16;
> > +
> > + /* The first buffer will start one page after the meta page */
> > + ptr += sizeof(*bmeta);
> > + ptr = ALIGN(ptr, PAGE_SIZE);
> > +
> > + if (bmeta->magic != RING_BUFFER_META_MAGIC) {
> > + pr_info("Ring buffer boot meta mismatch of magic\n");
> > + goto init;
> > + }
> > +
> > + if (bmeta->struct_sizes != struct_sizes) {
> > + pr_info("Ring buffer boot meta mismatch of struct size\n");
> > + goto init;
> > + }
> > +
> > + if (bmeta->total_size != total_size) {
> > + pr_info("Ring buffer boot meta mismatch of total size\n");
> > + goto init;
> > + }
> > +
> > + if (bmeta->buffers_offset > bmeta->total_size) {
> > + pr_info("Ring buffer boot meta mismatch of offset outside of
> > total size\n");
> > + goto init;
> > + }
> > +
> > + if (bmeta->buffers_offset != (void *)ptr - (void *)bmeta) {
> > + pr_info("Ring buffer boot meta mismatch of first buffer
> > offset\n");
> > + goto init;
> > + }
> > +
> > + return true;
> > +
> > @@ -2327,10 +2386,16 @@ static struct trace_buffer *alloc_buffer(unsigned
> > long size, unsigned flags,
> >
> > /* If start/end are specified, then that overrides size */
> > if (start && end) {
> > + unsigned long buffers_start;
> > unsigned long ptr;
> > int n;
> >
> > size = end - start;
> > +
> > + /* Subtract the buffer meta data */
> > + size -= PAGE_SIZE;
> > + buffers_start = start + PAGE_SIZE;
> > +
>
> So the buffer meta data is PAGE_SIZE aligned?
It's documented in the reserve_mem to be, but not as a requirement.
I should modify this to not depend on that.
Thanks,
-- Steve
>
> > size = size / nr_cpu_ids;
> >
> > /*
> > @@ -2340,7 +2405,7 @@ static struct trace_buffer *alloc_buffer(unsigned
> > long size, unsigned flags,
> > * needed, plus account for the integer array index that
> > * will be appended to the meta data.
> > */
> > - nr_pages = (size - sizeof(struct ring_buffer_meta)) /
> > + nr_pages = (size - sizeof(struct ring_buffer_cpu_meta)) /
> > (subbuf_size + sizeof(int));
> > /* Need at least two pages plus the reader page */
> > if (nr_pages < 3)
> > @@ -2348,8 +2413,8 @@ static struct trace_buffer *alloc_buffer(unsigned
> > long size, unsigned flags,
> >
> > again:
> > /* Make sure that the size fits aligned */
> > - for (n = 0, ptr = start; n < nr_cpu_ids; n++) {
> > - ptr += sizeof(struct ring_buffer_meta) +
> > + for (n = 0, ptr = buffers_start; n < nr_cpu_ids; n++) {
> > + ptr += sizeof(struct ring_buffer_cpu_meta) +
> > sizeof(int) * nr_pages;
> > ptr = ALIGN(ptr, subbuf_size);
> > ptr += subbuf_size * nr_pages;
> > @@ -3075,7 +3140,7 @@ static void rb_inc_iter(struct ring_buffer_iter *iter)
> > }
> >
> > /* Return the index into the sub-buffers for a given sub-buffer */
> > -static int rb_meta_subbuf_idx(struct ring_buffer_meta *meta, void *subbuf)
> > +static int rb_meta_subbuf_idx(struct ring_buffer_cpu_meta *meta, void
> > *subbuf)
> > {
> > void *subbuf_array;
> >
> > @@ -3087,7 +3152,7 @@ static int rb_meta_subbuf_idx(struct ring_buffer_meta
> > *meta, void *subbuf)
> > static void rb_update_meta_head(struct ring_buffer_per_cpu *cpu_buffer,
> > struct buffer_page *next_page)
> > {
> > - struct ring_buffer_meta *meta = cpu_buffer->ring_meta;
> > + struct ring_buffer_cpu_meta *meta = cpu_buffer->ring_meta;
> > unsigned long old_head = (unsigned long)next_page->page;
> > unsigned long new_head;
> >
> > @@ -3104,7 +3169,7 @@ static void rb_update_meta_head(struct
> > ring_buffer_per_cpu *cpu_buffer,
> > static void rb_update_meta_reader(struct ring_buffer_per_cpu *cpu_buffer,
> > struct buffer_page *reader)
> > {
> > - struct ring_buffer_meta *meta = cpu_buffer->ring_meta;
> > + struct ring_buffer_cpu_meta *meta = cpu_buffer->ring_meta;
> > void *old_reader = cpu_buffer->reader_page->page;
> > void *new_reader = reader->page;
> > int id;
> > @@ -3733,7 +3798,7 @@ rb_set_commit_to_write(struct ring_buffer_per_cpu
> > *cpu_buffer)
> > rb_page_write(cpu_buffer->commit_page));
> > rb_inc_page(&cpu_buffer->commit_page);
> > if (cpu_buffer->ring_meta) {
> > - struct ring_buffer_meta *meta = cpu_buffer->ring_meta;
> > + struct ring_buffer_cpu_meta *meta =
> > cpu_buffer->ring_meta;
> > meta->commit_buffer = (unsigned
> > long)cpu_buffer->commit_page->page;
> > }
> > /* add barrier to keep gcc from optimizing too much */
> > @@ -5986,7 +6051,7 @@ rb_reset_cpu(struct ring_buffer_per_cpu *cpu_buffer)
> > if (cpu_buffer->mapped) {
> > rb_update_meta_page(cpu_buffer);
> > if (cpu_buffer->ring_meta) {
> > - struct ring_buffer_meta *meta = cpu_buffer->ring_meta;
> > + struct ring_buffer_cpu_meta *meta =
> > cpu_buffer->ring_meta;
> > meta->commit_buffer = meta->head_buffer;
> > }
> > }
> > @@ -6020,7 +6085,7 @@ static void reset_disabled_cpu_buffer(struct
> > ring_buffer_per_cpu *cpu_buffer)
> > void ring_buffer_reset_cpu(struct trace_buffer *buffer, int cpu)
> > {
> > struct ring_buffer_per_cpu *cpu_buffer = buffer->buffers[cpu];
> > - struct ring_buffer_meta *meta;
> > + struct ring_buffer_cpu_meta *meta;
> >
> > if (!cpumask_test_cpu(cpu, buffer->cpumask))
> > return;
> > @@ -6058,7 +6123,7 @@ EXPORT_SYMBOL_GPL(ring_buffer_reset_cpu);
> > void ring_buffer_reset_online_cpus(struct trace_buffer *buffer)
> > {
> > struct ring_buffer_per_cpu *cpu_buffer;
> > - struct ring_buffer_meta *meta;
> > + struct ring_buffer_cpu_meta *meta;
> > int cpu;
> >
> > /* prevent another thread from changing buffer sizes */
>
> Thank you,
>
> > --
> > 2.45.2
> >
> >
>
>
