* [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).