On Sun, May 05, 2024 at 05:24:55PM +0200, Christophe JAILLET wrote:
> Le 05/05/2024 à 16:15, Erick Archer a écrit :
> > diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> > index 4013408ce012..080537eff69f 100644
> > --- a/kernel/events/ring_buffer.c
> > +++ b/kernel/events/ring_buffer.c
> > @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > watermark, int cpu, int flags)
> > unsigned long size;
>
> Hi,
>
> Should size be size_t?
I'm sorry, but I don't have enough knowledge to answer this question.
The "size" variable is used as a return value by struct_size and as
a parameter to the order_base_2() and kzalloc_node() functions.
The size type for the kzalloc_node function is "size_t" but for the
order_base_2() macro it is necessary an unsigned type (since this
is expanded to "__ilog2_u32(u32 n)" or "__ilog2_u64(u64 n)").
So, I don't know if it is correct to change the type to size_t.
Maybe someone can help with this.
>
> > int i, node;
> > - size = sizeof(struct perf_buffer);
> > - size += nr_pages * sizeof(void *);
> > -
> > + size = struct_size(rb, data_pages, nr_pages);
> > if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
> > goto fail;
> > @@ -833,6 +831,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > watermark, int cpu, int flags)
> > if (!rb)
> > goto fail;
> > + rb->nr_pages = nr_pages;
> > rb->user_page = perf_mmap_alloc_page(cpu);
> > if (!rb->user_page)
> > goto fail_user_page;
> > @@ -843,8 +842,6 @@ struct perf_buffer *rb_alloc(int nr_pages, long
> > watermark, int cpu, int flags)
> > goto fail_data_pages;
> > }
> > - rb->nr_pages = nr_pages;
> > -
> > ring_buffer_init(rb, watermark, flags);
> > return rb;
> > @@ -916,18 +913,15 @@ void rb_free(struct perf_buffer *rb)
> > struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int
> > flags)
> > {
> > struct perf_buffer *rb;
> > - unsigned long size;
> > void *all_buf;
> > int node;
> > - size = sizeof(struct perf_buffer);
> > - size += sizeof(void *);
> > -
> > node = (cpu == -1) ? cpu : cpu_to_node(cpu);
> > - rb = kzalloc_node(size, GFP_KERNEL, node);
> > + rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
> > if (!rb)
> > goto fail;
> > + rb->nr_pages = nr_pages;
>
> I don't think this is correct.
I think you are right. My bad :(
> There is already a logic in place about it a few lines below:
>
> all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
> if (!all_buf)
> goto fail_all_buf;
>
> rb->user_page = all_buf;
> rb->data_pages[0] = all_buf + PAGE_SIZE;
> if (nr_pages) { <--- here
> rb->nr_pages = 1; <---
> rb->page_order = ilog2(nr_pages);
> }
>
> I think that what is needed is to move this block just 2 lines above,
> (before rb->data_pages[0] = ...)
>
>
> I'm also wondering what should be done if nr_pages = 0.
Perhaps this is enough since we only allocate memory for one
member of the array.
@@ -916,18 +913,15 @@ void rb_free(struct perf_buffer *rb)
struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
{
struct perf_buffer *rb;
- unsigned long size;
void *all_buf;
int node;
- size = sizeof(struct perf_buffer);
- size += sizeof(void *);
-
node = (cpu == -1) ? cpu : cpu_to_node(cpu);
- rb = kzalloc_node(size, GFP_KERNEL, node);
+ rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
if (!rb)
goto fail;
+ rb->nr_pages = 1;
INIT_WORK(&rb->work, rb_free_work);
all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
I think that we don't need to deal with the "nr_pages = 0" case
since the flex array will always have a length of one.
Kees, can you help us with this?
Regards,
Erick
> CJ
>
> > INIT_WORK(&rb->work, rb_free_work);
> > all_buf = vmalloc_user((nr_pages + 1) * PAGE_SIZE);
>
