* [PATCH] libperf: Fix read_size calculations
@ 2022-02-23 13:13 Tzvetomir Stoyanov (VMware)
2022-02-23 13:59 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 4+ messages in thread
From: Tzvetomir Stoyanov (VMware) @ 2022-02-23 13:13 UTC (permalink / raw)
To: acme, olsajiri, irogers; +Cc: linux-perf-users
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.
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;
size += sizeof(u64);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] libperf: Fix read_size calculations
2022-02-23 13:13 [PATCH] libperf: Fix read_size calculations Tzvetomir Stoyanov (VMware)
@ 2022-02-23 13:59 ` Arnaldo Carvalho de Melo
2022-02-23 18:20 ` Jiri Olsa
0 siblings, 1 reply; 4+ messages in thread
From: Arnaldo Carvalho de Melo @ 2022-02-23 13:59 UTC (permalink / raw)
To: Tzvetomir Stoyanov (VMware)
Cc: olsajiri, irogers, linux-perf-users, Jiri Olsa
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;
> size += sizeof(u64);
> }
>
> --
> 2.34.1
--
- Arnaldo
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libperf: Fix read_size calculations
2022-02-23 13:59 ` Arnaldo Carvalho de Melo
@ 2022-02-23 18:20 ` Jiri Olsa
2022-02-24 9:10 ` Tzvetomir Stoyanov
0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2022-02-23 18:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Tzvetomir Stoyanov (VMware), irogers, linux-perf-users, Jiri Olsa
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] libperf: Fix read_size calculations
2022-02-23 18:20 ` Jiri Olsa
@ 2022-02-24 9:10 ` Tzvetomir Stoyanov
0 siblings, 0 replies; 4+ messages in thread
From: Tzvetomir Stoyanov @ 2022-02-24 9:10 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, Ian Rogers, linux-perf-users, Jiri Olsa
On Wed, Feb 23, 2022 at 8:20 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> 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?
You are right, I didn't call the perf_evlist__set_leader() API, that's
why the leader wasn't counted. When I call that API and revert this
patch, everything looks OK.
Thanks Jiri!
>
> thanks,
> jirka
>
> > > size += sizeof(u64);
> > > }
> > >
> > > --
> > > 2.34.1
> >
> > --
> >
> > - Arnaldo
--
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-24 9:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-23 13:13 [PATCH] libperf: Fix read_size calculations Tzvetomir Stoyanov (VMware)
2022-02-23 13:59 ` Arnaldo Carvalho de Melo
2022-02-23 18:20 ` Jiri Olsa
2022-02-24 9:10 ` Tzvetomir Stoyanov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).