Messages in this thread | | | From | Jiri Olsa <> | Date | Tue, 16 Aug 2022 15:19:20 +0200 | Subject | Re: [PATCH 2/4] tools lib perf: Handle read format in perf_evsel__read() |
| |
On Mon, Aug 15, 2022 at 12:01:04PM -0700, Namhyung Kim wrote: > The perf_counts_values should be increased to read the new lost data. > Also adjust values after read according the read format. > > This supports PERF_FORMAT_GROUP which has a different data format but > it's only available for leader events. Currently it doesn't have an API > to read sibling (member) events in the group. But users may read the > sibling event directly. > > Also reading from mmap would be disabled when the read format has ID or > LOST bit as it's not exposed via mmap. > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/lib/perf/evsel.c | 72 +++++++++++++++++++++++++++++ > tools/lib/perf/include/perf/event.h | 3 +- > tools/lib/perf/include/perf/evsel.h | 4 +- > 3 files changed, 77 insertions(+), 2 deletions(-) > > diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c > index 952f3520d5c2..fc23670231cb 100644 > --- a/tools/lib/perf/evsel.c > +++ b/tools/lib/perf/evsel.c > @@ -305,6 +305,9 @@ int perf_evsel__read_size(struct perf_evsel *evsel) > if (read_format & PERF_FORMAT_ID) > entry += sizeof(u64); > > + if (read_format & PERF_FORMAT_LOST) > + entry += sizeof(u64); > + > if (read_format & PERF_FORMAT_GROUP) { > nr = evsel->nr_members; > size += sizeof(u64); > @@ -314,24 +317,93 @@ int perf_evsel__read_size(struct perf_evsel *evsel) > return size; > } > > +/* This only reads values for the leader */ > +static int perf_evsel__read_group(struct perf_evsel *evsel, int cpu_map_idx, > + int thread, struct perf_counts_values *count) > +{ > + size_t size = perf_evsel__read_size(evsel); > + int *fd = FD(evsel, cpu_map_idx, thread); > + u64 read_format = evsel->attr.read_format; > + u64 *data; > + int idx = 1; > + > + if (fd == NULL || *fd < 0) > + return -EINVAL; > + > + data = calloc(1, size); > + if (data == NULL) > + return -ENOMEM; > + > + if (readn(*fd, data, size) <= 0) { > + free(data); > + return -errno; > + }
could you please put in here some comment that this is intentionaly reading only the leader or better yet rename the function? I was lost before I got to read the changelog ;-)
> + > + if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) > + count->ena = data[idx++]; > + if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) > + count->run = data[idx++]; > + > + /* value is always available */ > + count->val = data[idx++]; > + if (read_format & PERF_FORMAT_ID) > + count->id = data[idx++]; > + if (read_format & PERF_FORMAT_LOST) > + count->lost = data[idx++]; > + > + free(data); > + return 0; > +} > + > +/* > + * The perf read format is very flexible. It needs to set the proper > + * values according to the read format. > + */ > +static void perf_evsel__adjust_values(struct perf_evsel *evsel, > + struct perf_counts_values *count) > +{ > + u64 read_format = evsel->attr.read_format; > + > + if (!(read_format & PERF_FORMAT_TOTAL_TIME_ENABLED)) { > + memmove(&count->values[2], &count->values[1], 24); > + count->ena = 0; > + } > + > + if (!(read_format & PERF_FORMAT_TOTAL_TIME_RUNNING)) { > + memmove(&count->values[3], &count->values[2], 16); > + count->run = 0; > + } > + > + if (!(read_format & PERF_FORMAT_ID)) { > + memmove(&count->values[4], &count->values[3], 8); > + count->id = 0; > + } > +}
could we do this the same way we read group counters.. like make read into local buffer and initialize perf_counts_values values based on format, something like:
readn(fd, data ...
if (read_format & PERF_FORMAT_TOTAL_TIME_ENABLED) count->ena = data[idx++]; if (read_format & PERF_FORMAT_TOTAL_TIME_RUNNING) count->run = data[idx++];
/* value is always available */ count->val = data[idx++]; if (read_format & PERF_FORMAT_ID) count->id = data[idx++]; if (read_format & PERF_FORMAT_LOST) count->lost = data[idx++];
and perhaps we should cancel that perf_counts_values's union and keep only 'val/ena/run...' fields?
jirka
| |