Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 18 Aug 2022 09:59:26 -0700 | Subject | Re: [PATCH 4/4] perf tools: Support reading PERF_FORMAT_LOST |
| |
Hi Jiri,
On Thu, Aug 18, 2022 at 5:04 AM Jiri Olsa <olsajiri@gmail.com> 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
| |