Re: [PATCH 2/4] tools lib perf: Handle read format in perf_evsel__read()

From: Jiri Olsa
Date: Tue Aug 16 2022 - 09:20:16 EST


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@xxxxxxxxxx>
> ---
> 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