On Tue, Aug 20, 2024 at 08:45:03AM -0700, Namhyung Kim wrote:
> +++ b/tools/perf/builtin-report.c
> @@ -795,8 +795,13 @@ static int count_lost_samples_event(const struct perf_tool *tool,
>
> evsel = evlist__id2evsel(rep->session->evlist, sample->id);
> if (evsel) {
> - hists__inc_nr_lost_samples(evsel__hists(evsel),
> - event->lost_samples.lost);
> + struct hists *hists = evsel__hists(evsel);
> + u32 count = event->lost_samples.lost;
> +
> + if (event->header.misc & PERF_RECORD_MISC_LOST_SAMPLES_BPF)
> + hists__inc_nr_dropped_samples(hists, count);
So this is inconsistent, we call it sometimes "lost", sometines
"dropped", I think we should make it consistent and call it "dropped",
because its not like it was "lost" because we didn't have the required
resources, memory, ring buffer being full, etc, i.e. the semantic
associated with PERF_RECORD_LOST.
I.e. LOST is non intentional, not expected, DROPPED is the result of the
user _asking_ for something to be trown away, to be filtered, its
expected behaviour, there is value in differentiating one from the
other.
- Arnaldo
> + else
> + hists__inc_nr_lost_samples(hists, count);
> }
> return 0;
> }
> diff --git a/tools/perf/ui/stdio/hist.c b/tools/perf/ui/stdio/hist.c
> index 9372e8904d22..74b2c619c56c 100644
> --- a/tools/perf/ui/stdio/hist.c
> +++ b/tools/perf/ui/stdio/hist.c
> @@ -913,11 +913,11 @@ size_t events_stats__fprintf(struct events_stats *stats, FILE *fp)
> continue;
>
> if (i && total) {
> - ret += fprintf(fp, "%16s events: %10d (%4.1f%%)\n",
> + ret += fprintf(fp, "%20s events: %10d (%4.1f%%)\n",
> name, stats->nr_events[i],
> 100.0 * stats->nr_events[i] / total);
> } else {
> - ret += fprintf(fp, "%16s events: %10d\n",
> + ret += fprintf(fp, "%20s events: %10d\n",
> name, stats->nr_events[i]);
> }
> }
> diff --git a/tools/perf/util/events_stats.h b/tools/perf/util/events_stats.h
> index f43e5b1a366a..eabd7913c309 100644
> --- a/tools/perf/util/events_stats.h
> +++ b/tools/perf/util/events_stats.h
> @@ -18,7 +18,18 @@
> * PERF_RECORD_LOST_SAMPLES event. The number of lost-samples events is stored
> * in .nr_events[PERF_RECORD_LOST_SAMPLES] while total_lost_samples tells
> * exactly how many samples the kernel in fact dropped, i.e. it is the sum of
> - * all struct perf_record_lost_samples.lost fields reported.
> + * all struct perf_record_lost_samples.lost fields reported without setting the
> + * misc field in the header.
> + *
> + * The BPF program can discard samples according to the filter expressions given
> + * by the user. This number is kept in a BPF map and dumped at the end of perf
> + * record in a PERF_RECORD_LOST_SAMPLES event. To differentiate it from other
> + * lost samples, perf tools sets PERF_RECORD_MISC_LOST_SAMPLES_BPF flag in the
> + * header.misc field. The number of dropped-samples events is stored in
> + * .nr_events[PERF_RECORD_LOST_SAMPLES] while total_dropped_samples tells
> + * exactly how many samples the BPF program in fact dropped, i.e. it is the sum
> + * of all struct perf_record_lost_samples.lost fields reported with the misc
> + * field set in the header.
> *
> * The total_period is needed because by default auto-freq is used, so
> * multiplying nr_events[PERF_EVENT_SAMPLE] by a frequency isn't possible to get
> @@ -28,6 +39,7 @@
> struct events_stats {
> u64 total_lost;
> u64 total_lost_samples;
> + u64 total_dropped_samples;
> u64 total_aux_lost;
> u64 total_aux_partial;
> u64 total_aux_collision;
> @@ -48,6 +60,7 @@ struct hists_stats {
> u32 nr_samples;
> u32 nr_non_filtered_samples;
> u32 nr_lost_samples;
> + u32 nr_dropped_samples;
> };
>
> void events_stats__inc(struct events_stats *stats, u32 type);
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 0f9ce2ee2c31..dadce2889e52 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -2385,6 +2385,11 @@ void hists__inc_nr_lost_samples(struct hists *hists, u32 lost)
> hists->stats.nr_lost_samples += lost;
> }
>
> +void hists__inc_nr_dropped_samples(struct hists *hists, u32 lost)
> +{
> + hists->stats.nr_dropped_samples += lost;
> +}
> +
> static struct hist_entry *hists__add_dummy_entry(struct hists *hists,
> struct hist_entry *pair)
> {
> @@ -2729,18 +2734,24 @@ size_t evlist__fprintf_nr_events(struct evlist *evlist, FILE *fp)
>
> evlist__for_each_entry(evlist, pos) {
> struct hists *hists = evsel__hists(pos);
> + u64 total_samples = hists->stats.nr_samples;
> +
> + total_samples += hists->stats.nr_lost_samples;
> + total_samples += hists->stats.nr_dropped_samples;
>
> - if (symbol_conf.skip_empty && !hists->stats.nr_samples &&
> - !hists->stats.nr_lost_samples)
> + if (symbol_conf.skip_empty && total_samples == 0)
> continue;
>
> ret += fprintf(fp, "%s stats:\n", evsel__name(pos));
> if (hists->stats.nr_samples)
> - ret += fprintf(fp, "%16s events: %10d\n",
> + ret += fprintf(fp, "%20s events: %10d\n",
> "SAMPLE", hists->stats.nr_samples);
> if (hists->stats.nr_lost_samples)
> - ret += fprintf(fp, "%16s events: %10d\n",
> + ret += fprintf(fp, "%20s events: %10d\n",
> "LOST_SAMPLES", hists->stats.nr_lost_samples);
> + if (hists->stats.nr_dropped_samples)
> + ret += fprintf(fp, "%20s events: %10d\n",
> + "LOST_SAMPLES (BPF)", hists->stats.nr_dropped_samples);
> }
>
> return ret;
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 30c13fc8cbe4..4ea247e54fb6 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -371,6 +371,7 @@ void hists__inc_stats(struct hists *hists, struct hist_entry *h);
> void hists__inc_nr_events(struct hists *hists);
> void hists__inc_nr_samples(struct hists *hists, bool filtered);
> void hists__inc_nr_lost_samples(struct hists *hists, u32 lost);
> +void hists__inc_nr_dropped_samples(struct hists *hists, u32 lost);
>
> size_t hists__fprintf(struct hists *hists, bool show_header, int max_rows,
> int max_cols, float min_pcnt, FILE *fp,
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index c08774d06d14..00a49d0744fc 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -642,8 +642,9 @@ int machine__process_lost_event(struct machine *machine __maybe_unused,
> int machine__process_lost_samples_event(struct machine *machine __maybe_unused,
> union perf_event *event, struct perf_sample *sample)
> {
> - dump_printf(": id:%" PRIu64 ": lost samples :%" PRI_lu64 "\n",
> - sample->id, event->lost_samples.lost);
> + dump_printf(": id:%" PRIu64 ": lost samples :%" PRI_lu64 "%s\n",
> + sample->id, event->lost_samples.lost,
> + event->header.misc & PERF_RECORD_MISC_LOST_SAMPLES_BPF ? " (BPF)" : "");
> return 0;
> }
>
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index d2bd563119bc..774eb3382000 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -1290,8 +1290,9 @@ static int machines__deliver_event(struct machines *machines,
> evlist->stats.total_lost += event->lost.lost;
> return tool->lost(tool, event, sample, machine);
> case PERF_RECORD_LOST_SAMPLES:
> - if (tool->lost_samples == perf_event__process_lost_samples &&
> - !(event->header.misc & PERF_RECORD_MISC_LOST_SAMPLES_BPF))
> + if (event->header.misc & PERF_RECORD_MISC_LOST_SAMPLES_BPF)
> + evlist->stats.total_dropped_samples += event->lost_samples.lost;
> + else if (tool->lost_samples == perf_event__process_lost_samples)
> evlist->stats.total_lost_samples += event->lost_samples.lost;
> return tool->lost_samples(tool, event, sample, machine);
> case PERF_RECORD_READ:
> --
> 2.46.0.184.g6999bdac58-goog
