| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACT4Y+Z6KoYxY0AiC1eK=Ch9CQFuk7rWud_4GAMyjGdv+yemtQ@mail.gmail.com>
Date: Wed, 29 Jan 2025 08:12:51 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: irogers@...gle.com, linux-perf-users@...r.kernel.org,
linux-kernel@...r.kernel.org, Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: [PATCH v3 6/7] perf report: Add --latency flag
On Wed, 29 Jan 2025 at 06:03, Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Mon, Jan 27, 2025 at 10:58:53AM +0100, Dmitry Vyukov wrote:
> > Add record/report --latency flag that allows to capture and show
> > latency-centric profiles rather than the default CPU-consumption-centric
> > profiles. For latency profiles record captures context switch events,
> > and report shows Latency as the first column.
>
> It'd be nice if you could add a small example output in the commit
> message.
>
> >
> > Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
> > Cc: Namhyung Kim <namhyung@...nel.org>
> > Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> > Cc: Ian Rogers <irogers@...gle.com>
> > Cc: linux-perf-users@...r.kernel.org
> > Cc: linux-kernel@...r.kernel.org
> > ---
> [SNIP]
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index bc4c3acfe7552..2b6023de7a53a 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -2622,6 +2622,7 @@ struct hpp_dimension {
> > const char *name;
> > struct perf_hpp_fmt *fmt;
> > int taken;
> > + int was_taken;
> > };
> >
> > #define DIM(d, n) { .name = n, .fmt = &perf_hpp__format[d], }
> > @@ -3513,6 +3514,7 @@ static int __hpp_dimension__add(struct hpp_dimension *hd,
> > return -1;
> >
> > hd->taken = 1;
> > + hd->was_taken = 1;
> > perf_hpp_list__register_sort_field(list, fmt);
> > return 0;
> > }
> > @@ -3547,10 +3549,15 @@ static int __hpp_dimension__add_output(struct perf_hpp_list *list,
> > return 0;
> > }
> >
> > -int hpp_dimension__add_output(unsigned col)
> > +int hpp_dimension__add_output(unsigned col, bool implicit)
> > {
> > + struct hpp_dimension *hd;
> > +
> > BUG_ON(col >= PERF_HPP__MAX_INDEX);
> > - return __hpp_dimension__add_output(&perf_hpp_list, &hpp_sort_dimensions[col]);
> > + hd = &hpp_sort_dimensions[col];
> > + if (implicit && !hd->was_taken)
> > + return 0;
>
> I don't understand why you need 'was_taken' field. Can it use the
> 'taken' field?
I've started getting failed asserts in
perf_hpp__cancel_cumulate/latency when removing columns still linked
into sort order list.
I've figured out that columns we implicitly add in setup_overhead and
perf_hpp__init must match exactly. If we add one in setup_overhead,
but not in perf_hpp__init, then it starts failing.
Taken does not work b/c there is reset_dimensions call between these
functions, so they actually add the same columns twice to field/sort
lists (and that prevents failures in
perf_hpp__cancel_cumulate/latency).
Initially I've just tried to match logic in setup_overhead and in
perf_hpp__init. But it turned out quite messy, duplicate logic, and
e.g. in setup_overhead we look at sort_order, but in perf_hpp__init we
already can't look at it b/c we already altered it in setup_overhead.
That logic would also be quite fragile. Adding was_taken looks like
the simplest, most reliable, and less fragile with respect to future
changes approach.
> > + return __hpp_dimension__add_output(&perf_hpp_list, hd);
> > }
> >
> > int sort_dimension__add(struct perf_hpp_list *list, const char *tok,
> > @@ -3809,10 +3816,24 @@ static char *setup_overhead(char *keys)
> > if (sort__mode == SORT_MODE__DIFF)
> > return keys;
> >
> > - keys = prefix_if_not_in("overhead", keys);
> > -
> > - if (symbol_conf.cumulate_callchain)
> > - keys = prefix_if_not_in("overhead_children", keys);
> > + if (symbol_conf.prefer_latency) {
> > + keys = prefix_if_not_in("overhead", keys);
> > + keys = prefix_if_not_in("latency", keys);
> > + if (symbol_conf.cumulate_callchain) {
> > + keys = prefix_if_not_in("overhead_children", keys);
> > + keys = prefix_if_not_in("latency_children", keys);
> > + }
> > + } else if (!keys || (!strstr(keys, "overhead") &&
> > + !strstr(keys, "latency"))) {
> > + if (symbol_conf.enable_latency)
> > + keys = prefix_if_not_in("latency", keys);
> > + keys = prefix_if_not_in("overhead", keys);
> > + if (symbol_conf.cumulate_callchain) {
> > + if (symbol_conf.enable_latency)
> > + keys = prefix_if_not_in("latency_children", keys);
> > + keys = prefix_if_not_in("overhead_children", keys);
> > + }
> > + }
>
> Do you really need this complexity? I think it's better to fix the order
> of columns in both case.
This is sort order which affects ordering of row, and order of rows is
basically the most important thing for a profiler. If we fix the
order, it will be showing completely different things.
"latency" and "overhead" are equivalent with respect to their
fundamentality for a profile. So we shouldn't be adding any of them,
if any of them are already explicitly specified by the user.
For example, the command from tips.txt:
perf report --hierarchy --sort latency,parallelism,comm,symbol
if we prepend "overhead", it all falls apart.
Or for 2 default modes (normals and latency) we want "overhead" or
"latency" to come first. Prepending "latency" for the current CPU mode
would lead to completely different ordering.
> Thanks,
> Namhyung
>
> >
> > return keys;
> > }
> > diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> > index 11fb15f914093..180d36a2bea35 100644
> > --- a/tools/perf/util/sort.h
> > +++ b/tools/perf/util/sort.h
> > @@ -141,7 +141,7 @@ int report_parse_ignore_callees_opt(const struct option *opt, const char *arg, i
> >
> > bool is_strict_order(const char *order);
> >
> > -int hpp_dimension__add_output(unsigned col);
> > +int hpp_dimension__add_output(unsigned col, bool implicit);
> > void reset_dimensions(void);
> > int sort_dimension__add(struct perf_hpp_list *list, const char *tok,
> > struct evlist *evlist,
> > diff --git a/tools/perf/util/symbol_conf.h b/tools/perf/util/symbol_conf.h
> > index c5b2e56127e22..cd9aa82c7d5ad 100644
> > --- a/tools/perf/util/symbol_conf.h
> > +++ b/tools/perf/util/symbol_conf.h
> > @@ -49,7 +49,9 @@ struct symbol_conf {
> > keep_exited_threads,
> > annotate_data_member,
> > annotate_data_sample,
> > - skip_empty;
> > + skip_empty,
> > + enable_latency,
> > + prefer_latency;
> > const char *vmlinux_name,
> > *kallsyms_name,
> > *source_prefix,
> > --
> > 2.48.1.262.g85cc9f2d1e-goog
> >
Powered by blists - more mailing lists