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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Fri, 17 May 2024 20:18:49 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	Changbin Du <changbin.du@...wei.com>, John Fastabend <john.fastabend@...il.com>, 
	Andrii Nakryiko <andrii@...nel.org>, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH v1] perf top: Make zeroing histogram on refresh the default

On Fri, May 17, 2024 at 6:25 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Thu, May 16, 2024 at 3:22 PM Ian Rogers <irogers@...gle.com> wrote:
> >
> > Instead of decaying histograms over time change it so that they are
> > zero-ed on each perf top refresh. Previously the option '-z', or
> > pressing 'z' in tui mode, would enable this behavior. Decaying samples
> > is non-intuitive as it isn't how "top" works. Make zeroing on refresh
> > the default and rename the command line options from 'z' to 'Z' and
> > 'zero' to 'decay'.
>
> While it may make more sense, I'm afraid of changing the default
> behavior.  I think we can add a config option for this.

I don't think the config option would clear up the confusion. I think
zero should be the default given it matches "top". The problem is
worse with filtering as samples will decay and disappear faster when
there are more of them. We could keep a -z option that does nothing,
for the sake of command line compatibility. I don't see the -z option
documented on the wiki, or Brendan Gregg's tutorials so my guess is
that people don't know it exists (I didn't) and they aren't confused
as cycles:P without filtering looks similar with zero or with
decaying.

Thanks,
Ian

> Also instead of adding a new option, it should be able to use the
> existing --no-zero option.
>
> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@...gle.com>
> > ---
> >  tools/perf/Documentation/perf-top.txt |  6 +++---
> >  tools/perf/builtin-top.c              | 23 +++++++++++++----------
> >  tools/perf/ui/browsers/hists.c        |  7 ++++---
> >  tools/perf/util/top.h                 |  2 +-
> >  4 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-top.txt b/tools/perf/Documentation/perf-top.txt
> > index 667e5102075e..f1524cc0d409 100644
> > --- a/tools/perf/Documentation/perf-top.txt
> > +++ b/tools/perf/Documentation/perf-top.txt
> > @@ -124,9 +124,9 @@ Default is to monitor all CPUS.
> >  --verbose::
> >         Be more verbose (show counter open errors, etc).
> >
> > --z::
> > ---zero::
> > -       Zero history across display updates.
> > +-Z::
> > +--decay::
> > +       Decay rather than zero history across display updates.
> >
> >  -s::
> >  --sort::
> > diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> > index e8cbbf10d361..8f06635cb7cd 100644
> > --- a/tools/perf/builtin-top.c
> > +++ b/tools/perf/builtin-top.c
> > @@ -266,10 +266,10 @@ static void perf_top__show_details(struct perf_top *top)
> >         more = symbol__annotate_printf(&he->ms, top->sym_evsel);
> >
> >         if (top->evlist->enabled) {
> > -               if (top->zero)
> > -                       symbol__annotate_zero_histogram(symbol, top->sym_evsel->core.idx);
> > -               else
> > +               if (top->decay_samples)
> >                         symbol__annotate_decay_histogram(symbol, top->sym_evsel->core.idx);
> > +               else
> > +                       symbol__annotate_zero_histogram(symbol, top->sym_evsel->core.idx);
> >         }
> >         if (more != 0)
> >                 printf("%d lines not displayed, maybe increase display entries [e]\n", more);
> > @@ -292,11 +292,11 @@ static void perf_top__resort_hists(struct perf_top *t)
> >                 hists__unlink(hists);
> >
> >                 if (evlist->enabled) {
> > -                       if (t->zero) {
> > -                               hists__delete_entries(hists);
> > -                       } else {
> > +                       if (t->decay_samples) {
> >                                 hists__decay_entries(hists, t->hide_user_symbols,
> >                                                      t->hide_kernel_symbols);
> > +                       } else {
> > +                               hists__delete_entries(hists);
> >                         }
> >                 }
> >
> > @@ -460,7 +460,9 @@ static void perf_top__print_mapped_keys(struct perf_top *top)
> >         fprintf(stdout,
> >                 "\t[U]     hide user symbols.               \t(%s)\n",
> >                 top->hide_user_symbols ? "yes" : "no");
> > -       fprintf(stdout, "\t[z]     toggle sample zeroing.             \t(%d)\n", top->zero ? 1 : 0);
> > +       fprintf(stdout,
> > +               "\t[z]     toggle sample zeroing/decaying.  \t(%s)\n",
> > +               top->decay_samples ? "decay" : "zero");
> >         fprintf(stdout, "\t[qQ]    quit.\n");
> >  }
> >
> > @@ -583,7 +585,7 @@ static bool perf_top__handle_keypress(struct perf_top *top, int c)
> >                         top->hide_user_symbols = !top->hide_user_symbols;
> >                         break;
> >                 case 'z':
> > -                       top->zero = !top->zero;
> > +                       top->decay_samples = !top->decay_samples;
> >                         break;
> >                 default:
> >                         break;
> > @@ -648,7 +650,7 @@ static void *display_thread_tui(void *arg)
> >         ret = evlist__tui_browse_hists(top->evlist, help, &hbt, top->min_percent,
> >                                        &top->session->header.env, !top->record_opts.overwrite);
> >         if (ret == K_RELOAD) {
> > -               top->zero = true;
> > +               top->decay_samples = false;
> >                 goto repeat;
> >         } else
> >                 stop_top();
> > @@ -1502,7 +1504,8 @@ int cmd_top(int argc, const char **argv)
> >                     "child tasks do not inherit counters"),
> >         OPT_STRING(0, "sym-annotate", &top.sym_filter, "symbol name",
> >                     "symbol to annotate"),
> > -       OPT_BOOLEAN('z', "zero", &top.zero, "zero history across updates"),
> > +       OPT_BOOLEAN('Z', "decay", &top.decay_samples,
> > +                   "decay rather than zero history across updates"),
> >         OPT_CALLBACK('F', "freq", &top.record_opts, "freq or 'max'",
> >                      "profile at this frequency",
> >                       record__parse_freq),
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index b7219df51236..bcc4720f8198 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -2305,8 +2305,8 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
> >                                      " drop: %" PRIu64 "/%" PRIu64,
> >                                      top->drop, top->drop_total);
> >
> > -               if (top->zero)
> > -                       printed += scnprintf(bf + printed, size - printed, " [z]");
> > +               if (top->decay_samples)
> > +                       printed += scnprintf(bf + printed, size - printed, " [decay]");
> >
> >                 perf_top__reset_sample_counters(top);
> >         }
> > @@ -3209,9 +3209,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> >                         continue;
> >                 case 'z':
> >                         if (!is_report_browser(hbt)) {
> > +                               /* Toggle between zeroing and decaying samples. */
> >                                 struct perf_top *top = hbt->arg;
> >
> > -                               top->zero = !top->zero;
> > +                               top->decay_samples = !top->decay_samples;
> >                         }
> >                         continue;
> >                 case 'L':
> > diff --git a/tools/perf/util/top.h b/tools/perf/util/top.h
> > index 4c5588dbb131..b2c199925b36 100644
> > --- a/tools/perf/util/top.h
> > +++ b/tools/perf/util/top.h
> > @@ -32,7 +32,7 @@ struct perf_top {
> >         u64                guest_us_samples, guest_kernel_samples;
> >         int                print_entries, count_filter, delay_secs;
> >         int                max_stack;
> > -       bool               hide_kernel_symbols, hide_user_symbols, zero;
> > +       bool               hide_kernel_symbols, hide_user_symbols, decay_samples;
> >  #ifdef HAVE_SLANG_SUPPORT
> >         bool               use_tui;
> >  #endif
> > --
> > 2.45.0.rc1.225.g2a3ae87e7f-goog
> >

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ