Re: [PATCH] libperf: Fix read_size calculations

[Jiri Olsa <olsajiri@gmail.com>]

On Wed, Feb 23, 2022 at 10:59:03AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 23, 2022 at 03:13:49PM +0200, Tzvetomir Stoyanov (VMware) escreveu:
> > Reading the counters from perf descriptor fails because of size
> > mismatch when PERF_FORMAT_GROUP is set. The group count must include the
> > parent and the count of siblings.
> 
> Yeah, it should match the calculation done in the kernel, in
> __perf_event_read_size(), that has...
> 
> static void __perf_event_read_size(struct perf_event *event, int nr_siblings)
> {
>         int entry = sizeof(u64); /* value */
>         int size = 0;
>         int nr = 1;
> 
>         if (event->attr.read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)
>                 size += sizeof(u64);
> 
>         if (event->attr.read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)
>                 size += sizeof(u64);
> 
>         if (event->attr.read_format & PERF_FORMAT_ID)
>                 entry += sizeof(u64);
> 
>         if (event->attr.read_format & PERF_FORMAT_GROUP) {
>                 nr += nr_siblings;
>                 size += sizeof(u64);
>         }
> 
>         size += entry * nr;
>         event->read_size = size;
> }
> 
> So this bug has been with us since forever (well, 2017), so we need:
> 
> Fixes: 5c30af92f2b1e9d8 ("libperf: Adopt perf_evsel__read() function from tools/perf")
> 
> To fix it in libperf and probably:
> 
> Fixes: de63403bfd14ae8d ("perf tools: Add perf_evsel__read_size function")
> 
> To get this to stable kernels versions predating the move of this
> function to libperf.
> 
> Jiri?
> 
> - Arnaldo
>  
> > Signed-off-by: Tzvetomir Stoyanov (VMware) <tz.stoyanov@gmail.com>
> > ---
> >  tools/lib/perf/evsel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> > index 210ea7c06ce8..4597a53c0152 100644
> > --- a/tools/lib/perf/evsel.c
> > +++ b/tools/lib/perf/evsel.c
> > @@ -299,7 +299,7 @@ int perf_evsel__read_size(struct perf_evsel *evsel)
> >  		entry += sizeof(u64);
> >  
> >  	if (read_format & PERF_FORMAT_GROUP) {
> > -		nr = evsel->nr_members;
> > +		nr += evsel->nr_members;

hum, I'll double check but AFAICS from tests/parse-events.c,
nr_members already includes both parent (leader) and members,
which is different from nr_siblings in kernel

can you please provide more details?

thanks,
jirka

> >  		size += sizeof(u64);
> >  	}
> >  
> > -- 
> > 2.34.1
> 
> -- 
> 
> - Arnaldo