Re: [tip: perf/core] perf/x86: Annotate struct bts_buffer with __counted_by()

[Ingo Molnar <mingo@kernel.org>]

* Thorsten Blum <thorsten.blum@linux.dev> wrote:

> On 5. Mar 2025, at 10:18, Ingo Molnar wrote:
> > Actually, on a second thought:
> > 
> >> - buf = kzalloc_node(offsetof(struct bts_buffer, buf[nbuf]), GFP_KERNEL, node);
> >> + buf = kzalloc_node(struct_size(buf, buf, nbuf), GFP_KERNEL, node);
> > 
> > Firstly, in what world is 'buf, buf' more readable? One is a member of 
> > a structure, the other is the name of the structure - and they match, 
> > which shows that this function's naming conventions are a mess.
> > 
> > Which should be fixed first ...
> 
> Yes, I noticed this too, but since buf->buf[] is used all over the place
> (also in other functions), I didn't rename it in this patch.
> 
> We could just keep offsetof(struct bts_buffer, buf[nbuf]), or use
> struct_size_t(struct bts_buffer, buf, nbuf) and still benefit from
> additional compile-time checks, or rename the local variable to struct
> bts_buffer *bts and use struct_size(bts, buf, nbuf), for example. Any
> preferences or other ideas?

To clean up this code before changing it, so that the changes become 
obvious to review.

Please also split out the annotation for instrumentation, it's separate 
from any struct_size() changes, right?

> > I'm also not sure the code is correct ...
> 
> Which part of it?

The size calculation. On a second reading I *think* it's correct, but 
it's unnecessarily confusing due to the buf<->buf aliasing.

So in a cleaned up version of the code:

  - If we name 'struct bts_buffer' objects 'bb'
  - and bb:buf[] is the var-array
  - and we rename 'nbuf' to 'nr_buf' (the number of bb:buf[] elements)

then the code right now does:

        bb = kzalloc_node(offsetof(struct bts_buffer, bb[nr_buf]), GFP_KERNEL, node);

... which looks correct.

Thanks,

	Ingo