Re: [PATCH 4/4] perf tools: Support reading PERF_FORMAT_LOST

[Namhyung Kim]

Hi Jiri,

On Thu, Aug 18, 2022 at 5:04 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Tue, Aug 16, 2022 at 03:17:47PM -0700, Namhyung Kim wrote:
>
> SNIP
>
> > diff --git a/tools/perf/util/event.h b/tools/perf/util/event.h
> > index a7b0931d5137..7753368d70d6 100644
> > --- a/tools/perf/util/event.h
> > +++ b/tools/perf/util/event.h
> > @@ -65,7 +65,8 @@ struct stack_dump {
> >
> >  struct sample_read_value {
> >       u64 value;
> > -     u64 id;
> > +     u64 id;   /* only if PERF_FORMAT_ID */
> > +     u64 lost; /* only if PERF_FORMAT_LOST */
> >  };
>
> I was wondering why not to split this patch into smaller piece,
> but once you change this struct you break all the places

Right.. I'd like to do so but couldn't.. :)

>
> SNIP
>
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1541,7 +1541,7 @@ static int evsel__read_one(struct evsel *evsel, int cpu_map_idx, int thread)
> >  }
> >
> >  static void evsel__set_count(struct evsel *counter, int cpu_map_idx, int thread,
> > -                          u64 val, u64 ena, u64 run)
> > +                          u64 val, u64 ena, u64 run, u64 lost)
> >  {
> >       struct perf_counts_values *count;
> >
> > @@ -1550,6 +1550,7 @@ static void evsel__set_count(struct evsel *counter, int cpu_map_idx, int thread,
> >       count->val    = val;
> >       count->ena    = ena;
> >       count->run    = run;
> > +     count->lost   = lost;
> >
> >       perf_counts__set_loaded(counter->counts, cpu_map_idx, thread, true);
> >  }
> > @@ -1558,7 +1559,7 @@ static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int
> >  {
> >       u64 read_format = leader->core.attr.read_format;
> >       struct sample_read_value *v;
> > -     u64 nr, ena = 0, run = 0, i;
> > +     u64 nr, ena = 0, run = 0, lost = 0, i;
> >
> >       nr = *data++;
> >
> > @@ -1573,16 +1574,25 @@ static int evsel__process_group_data(struct evsel *leader, int cpu_map_idx, int
> >
> >       v = (struct sample_read_value *) data;
> >
> > -     evsel__set_count(leader, cpu_map_idx, thread, v[0].value, ena, run);
> > +     if (read_format & PERF_FORMAT_LOST)
> > +             lost = v->lost;
> > +
> > +     evsel__set_count(leader, cpu_map_idx, thread, v[0].value, ena, run, lost);
> > +
> > +     v = next_sample_read_value(v, read_format);
>
> oneway of making this simpler here and share with other places
> could be adding something like:
>
>   for_each_group_data(v, i, nr, read_format) {
>   }
>
> but not sure how would that turn out, thoughts?

Looks good.  Let me try. :)


>
> >
> >       for (i = 1; i < nr; i++) {
> >               struct evsel *counter;
> >
> > -             counter = evlist__id2evsel(leader->evlist, v[i].id);
> > +             counter = evlist__id2evsel(leader->evlist, v->id);
> >               if (!counter)
> >                       return -EINVAL;
> >
> > -             evsel__set_count(counter, cpu_map_idx, thread, v[i].value, ena, run);
> > +             if (read_format & PERF_FORMAT_LOST)
> > +                     lost = v->lost;
> > +
> > +             evsel__set_count(counter, cpu_map_idx, thread, v->value, ena, run, lost);
> > +             v = next_sample_read_value(v, read_format);
> >       }
> >
> >       return 0;
> > @@ -2475,16 +2485,21 @@ int evsel__parse_sample(struct evsel *evsel, union perf_event *event,
> >
> >                       if (data->read.group.nr > max_group_nr)
> >                               return -EFAULT;
> > -                     sz = data->read.group.nr *
> > -                          sizeof(struct sample_read_value);
> > +
> > +                     sz = data->read.group.nr * sample_read_value_size(read_format);
> >                       OVERFLOW_CHECK(array, sz, max_size);
> > -                     data->read.group.values =
> > -                                     (struct sample_read_value *)array;
> > +                     data->read.group.values = (void *)array;
>
> nit, is this void casting needed?

Well.. the array is a pointer to u64 so the casting is needed.
But it's an unrelated change, can be dropped.

Thanks,
Namhyung


>
> >                       array = (void *)array + sz;
> >               } else {
> >                       OVERFLOW_CHECK_u64(array);
> >                       data->read.one.id = *array;
> >                       array++;
> > +
> > +                     if (read_format & PERF_FORMAT_LOST) {
> > +                             OVERFLOW_CHECK_u64(array);
> > +                             data->read.one.lost = *array;
> > +                             array++;
> > +                     }
> >               }
> >       }
> >
>
> SNIP