linux-kernel - Re: [PATCH v8 01/10] perf record --off-cpu: Add --off-cpu-thresh option

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]

Message-ID: <CAH0uvoj5EY3mkgtduKV7VY8iTWCrhokCtuZimy=N7=3Ah9gGgw@mail.gmail.com>
Date: Mon, 18 Nov 2024 15:03:41 -0800
From: Howard Chu <howardchu95@...il.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: acme@...nel.org, peterz@...radead.org, irogers@...gle.com, 
	mingo@...hat.com, jolsa@...nel.org, adrian.hunter@...el.com, 
	kan.liang@...ux.intel.com, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, James Clark <james.clark@...aro.org>, 
	Mark Rutland <mark.rutland@....com>, Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH v8 01/10] perf record --off-cpu: Add --off-cpu-thresh option

Hello Namhyung,

On Mon, Nov 18, 2024 at 1:09 PM Namhyung Kim <namhyung@...nel.org> wrote:
>
> Hello Howard,
>
> On Tue, Nov 12, 2024 at 04:28:09PM -0800, Howard Chu wrote:
> > Specify the threshold for dumping offcpu samples with --off-cpu-thresh,
> > the unit is us (microsecond). Default value is 500000us (500ms, 0.5s).
>
> I think it's better to add an option after you implemented the feature.
> Before that you may hardcode the threshold if necessary.
>
> Also it'd be a good practice to add an example for the option in the
> commit message.

Sure.

>
> >
> > Suggested-by: Ian Rogers <irogers@...gle.com>
> > Reviewed-by: Ian Rogers <irogers@...gle.com>
> > Signed-off-by: Howard Chu <howardchu95@...il.com>
> > Cc: Adrian Hunter <adrian.hunter@...el.com>
> > Cc: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
> > Cc: Ingo Molnar <mingo@...hat.com>
> > Cc: James Clark <james.clark@...aro.org>
> > Cc: Jiri Olsa <jolsa@...nel.org>
> > Cc: Kan Liang <kan.liang@...ux.intel.com>
> > Cc: Mark Rutland <mark.rutland@....com>
> > Cc: Namhyung Kim <namhyung@...nel.org>
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Link: https://lore.kernel.org/r/20241108204137.2444151-2-howardchu95@gmail.com
> > Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
> > ---
> >  tools/perf/builtin-record.c | 26 ++++++++++++++++++++++++++
> >  tools/perf/util/off_cpu.h   |  1 +
> >  tools/perf/util/record.h    |  1 +
>
> Please update the documentation if you add a new option.

Ok sure.

Thanks,
Howard
>
> Thanks,
> Namhyung
>
>
> >  3 files changed, 28 insertions(+)
> >
> > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> > index f83252472921..c069000efe5c 100644
> > --- a/tools/perf/builtin-record.c
> > +++ b/tools/perf/builtin-record.c
> > @@ -3149,6 +3149,28 @@ static int record__parse_mmap_pages(const struct option *opt,
> >       return ret;
> >  }
> >
> > +static int record__parse_off_cpu_thresh(const struct option *opt,
> > +                                     const char *str,
> > +                                     int unset __maybe_unused)
> > +{
> > +     struct record_opts *opts = opt->value;
> > +     char *endptr;
> > +     u64 off_cpu_thresh_us;
> > +
> > +     if (!str)
> > +             return -EINVAL;
> > +
> > +     off_cpu_thresh_us = strtoull(str, &endptr, 10);
> > +
> > +     /* threshold isn't string "0", yet strtoull() returns 0, parsing failed */
> > +     if (*endptr || (off_cpu_thresh_us == 0 && strcmp(str, "0")))
> > +             return -EINVAL;
> > +     else
> > +             opts->off_cpu_thresh_us = off_cpu_thresh_us;
> > +
> > +     return 0;
> > +}
> > +
> >  void __weak arch__add_leaf_frame_record_opts(struct record_opts *opts __maybe_unused)
> >  {
> >  }
> > @@ -3342,6 +3364,7 @@ static struct record record = {
> >               .ctl_fd              = -1,
> >               .ctl_fd_ack          = -1,
> >               .synth               = PERF_SYNTH_ALL,
> > +             .off_cpu_thresh_us   = OFFCPU_THRESH,
> >       },
> >  };
> >
> > @@ -3564,6 +3587,9 @@ static struct option __record_options[] = {
> >       OPT_BOOLEAN(0, "off-cpu", &record.off_cpu, "Enable off-cpu analysis"),
> >       OPT_STRING(0, "setup-filter", &record.filter_action, "pin|unpin",
> >                  "BPF filter action"),
> > +     OPT_CALLBACK(0, "off-cpu-thresh", &record.opts, "us",
> > +                  "Dump off-cpu samples if off-cpu time reaches this threshold. The unit is microsecond (default: 500000)",
> > +                  record__parse_off_cpu_thresh),
> >       OPT_END()
> >  };
> >
> > diff --git a/tools/perf/util/off_cpu.h b/tools/perf/util/off_cpu.h
> > index 2dd67c60f211..c6edc0f7c40d 100644
> > --- a/tools/perf/util/off_cpu.h
> > +++ b/tools/perf/util/off_cpu.h
> > @@ -16,6 +16,7 @@ struct record_opts;
> >                             PERF_SAMPLE_PERIOD | PERF_SAMPLE_CALLCHAIN | \
> >                             PERF_SAMPLE_CGROUP)
> >
> > +#define OFFCPU_THRESH 500000ull
> >
> >  #ifdef HAVE_BPF_SKEL
> >  int off_cpu_prepare(struct evlist *evlist, struct target *target,
> > diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h
> > index a6566134e09e..2ca74add26c0 100644
> > --- a/tools/perf/util/record.h
> > +++ b/tools/perf/util/record.h
> > @@ -79,6 +79,7 @@ struct record_opts {
> >       int           synth;
> >       int           threads_spec;
> >       const char    *threads_user_spec;
> > +     u64           off_cpu_thresh_us;
> >  };
> >
> >  extern const char * const *record_usage;
> > --
> > 2.43.0
> >