Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 16 Aug 2022 14:54:40 -0700 | Subject | Re: [PATCH 2/4] tools lib perf: Handle read format in perf_evsel__read() |
| |
Hi Jiri,
On Tue, Aug 16, 2022 at 6:19 AM Jiri Olsa <olsajiri@gmail.com> wrote: > > 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 ;-)
Sure, actually it has a comment above the function signature. Maybe we can add an API to read whole group counters but it could be a follow up.
> > > + > > + 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++];
Sure.
> > > and perhaps we should cancel that perf_counts_values's union and keep > only 'val/ena/run...' fields?
I'd like to have them and use it for reading the lost count soon.
Thanks, Namhyung
| |