Improvement to command line handling for perf lock info.
---
v1: https://lore.kernel.org/all/[email protected]/
v2:
* Drop previous "perf lock report: Restore aggregation by caller by
default" patch. Not required for newly collected data with LOCKDEP
tracepoints.
* Display map and thread info (rather than an error) when neither or
both of --map and --thread are specified.
v3:
* Update --help output
v4:
* Remove redundant error handling
Nick Forrington (1):
perf lock info: Display both map and thread by default
tools/perf/Documentation/perf-lock.txt | 4 ++--
tools/perf/builtin-lock.c | 27 ++++++++++++++------------
2 files changed, 17 insertions(+), 14 deletions(-)
--
2.44.0
Change "perf lock info" argument handling to:
Display both map and thread info (rather than an error) when neither are
specified.
Display both map and thread info (rather than just thread info) when
both are requested.
Signed-off-by: Nick Forrington <[email protected]>
---
tools/perf/Documentation/perf-lock.txt | 4 ++--
tools/perf/builtin-lock.c | 27 ++++++++++++++------------
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
index f5938d616d75..57a940399de0 100644
--- a/tools/perf/Documentation/perf-lock.txt
+++ b/tools/perf/Documentation/perf-lock.txt
@@ -111,11 +111,11 @@ INFO OPTIONS
-t::
--threads::
- dump thread list in perf.data
+ dump only the thread list in perf.data
-m::
--map::
- dump map of lock instances (address:name table)
+ dump only the map of lock instances (address:name table)
CONTENTION OPTIONS
diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
index 230461280e45..7eb115aeb927 100644
--- a/tools/perf/builtin-lock.c
+++ b/tools/perf/builtin-lock.c
@@ -1477,20 +1477,16 @@ static void dump_map(void)
fprintf(lock_output, " %#llx: %s\n", (unsigned long long)st->addr, st->name);
}
-static int dump_info(void)
+static void dump_info(void)
{
- int rc = 0;
-
if (info_threads)
dump_threads();
- else if (info_map)
+
+ if (info_map) {
+ if (info_threads)
+ fputc('\n', lock_output);
dump_map();
- else {
- rc = -1;
- pr_err("Unknown type of information\n");
}
-
- return rc;
}
static const struct evsel_str_handler lock_tracepoints[] = {
@@ -1992,7 +1988,7 @@ static int __cmd_report(bool display_info)
setup_pager();
if (display_info) /* used for info subcommand */
- err = dump_info();
+ dump_info();
else {
combine_result();
sort_result();
@@ -2578,9 +2574,9 @@ int cmd_lock(int argc, const char **argv)
const struct option info_options[] = {
OPT_BOOLEAN('t', "threads", &info_threads,
- "dump thread list in perf.data"),
+ "dump the thread list in perf.data"),
OPT_BOOLEAN('m', "map", &info_map,
- "map of lock instances (address:name table)"),
+ "dump the map of lock instances (address:name table)"),
OPT_PARENT(lock_options)
};
@@ -2694,6 +2690,13 @@ int cmd_lock(int argc, const char **argv)
if (argc)
usage_with_options(info_usage, info_options);
}
+
+ /* If neither threads nor map requested, display both */
+ if (!info_threads && !info_map) {
+ info_threads = true;
+ info_map = true;
+ }
+
/* recycling report_lock_ops */
trace_handler = &report_lock_ops;
rc = __cmd_report(true);
--
2.44.0
Is this ok to be applied now?
Thanks,
Nick
On 13/05/2024 10:14, Nick Forrington wrote:
> Change "perf lock info" argument handling to:
>
> Display both map and thread info (rather than an error) when neither are
> specified.
>
> Display both map and thread info (rather than just thread info) when
> both are requested.
>
> Signed-off-by: Nick Forrington <[email protected]>
> ---
> tools/perf/Documentation/perf-lock.txt | 4 ++--
> tools/perf/builtin-lock.c | 27 ++++++++++++++------------
> 2 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> index f5938d616d75..57a940399de0 100644
> --- a/tools/perf/Documentation/perf-lock.txt
> +++ b/tools/perf/Documentation/perf-lock.txt
> @@ -111,11 +111,11 @@ INFO OPTIONS
>
> -t::
> --threads::
> - dump thread list in perf.data
> + dump only the thread list in perf.data
>
> -m::
> --map::
> - dump map of lock instances (address:name table)
> + dump only the map of lock instances (address:name table)
>
>
> CONTENTION OPTIONS
> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> index 230461280e45..7eb115aeb927 100644
> --- a/tools/perf/builtin-lock.c
> +++ b/tools/perf/builtin-lock.c
> @@ -1477,20 +1477,16 @@ static void dump_map(void)
> fprintf(lock_output, " %#llx: %s\n", (unsigned long long)st->addr, st->name);
> }
>
> -static int dump_info(void)
> +static void dump_info(void)
> {
> - int rc = 0;
> -
> if (info_threads)
> dump_threads();
> - else if (info_map)
> +
> + if (info_map) {
> + if (info_threads)
> + fputc('\n', lock_output);
> dump_map();
> - else {
> - rc = -1;
> - pr_err("Unknown type of information\n");
> }
> -
> - return rc;
> }
>
> static const struct evsel_str_handler lock_tracepoints[] = {
> @@ -1992,7 +1988,7 @@ static int __cmd_report(bool display_info)
>
> setup_pager();
> if (display_info) /* used for info subcommand */
> - err = dump_info();
> + dump_info();
> else {
> combine_result();
> sort_result();
> @@ -2578,9 +2574,9 @@ int cmd_lock(int argc, const char **argv)
>
> const struct option info_options[] = {
> OPT_BOOLEAN('t', "threads", &info_threads,
> - "dump thread list in perf.data"),
> + "dump the thread list in perf.data"),
> OPT_BOOLEAN('m', "map", &info_map,
> - "map of lock instances (address:name table)"),
> + "dump the map of lock instances (address:name table)"),
> OPT_PARENT(lock_options)
> };
>
> @@ -2694,6 +2690,13 @@ int cmd_lock(int argc, const char **argv)
> if (argc)
> usage_with_options(info_usage, info_options);
> }
> +
> + /* If neither threads nor map requested, display both */
> + if (!info_threads && !info_map) {
> + info_threads = true;
> + info_map = true;
> + }
> +
> /* recycling report_lock_ops */
> trace_handler = &report_lock_ops;
> rc = __cmd_report(true);
Hello,
On Mon, Jun 3, 2024 at 2:38 AM Nick Forrington <[email protected]> wrote:
>
> Is this ok to be applied now?
Sorry for the delay. Yep, it's now applied to perf-tools-next.
Thanks,
Namhyung
>
> On 13/05/2024 10:14, Nick Forrington wrote:
> > Change "perf lock info" argument handling to:
> >
> > Display both map and thread info (rather than an error) when neither are
> > specified.
> >
> > Display both map and thread info (rather than just thread info) when
> > both are requested.
> >
> > Signed-off-by: Nick Forrington <[email protected]>
> > ---
> > tools/perf/Documentation/perf-lock.txt | 4 ++--
> > tools/perf/builtin-lock.c | 27 ++++++++++++++------------
> > 2 files changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-lock.txt b/tools/perf/Documentation/perf-lock.txt
> > index f5938d616d75..57a940399de0 100644
> > --- a/tools/perf/Documentation/perf-lock.txt
> > +++ b/tools/perf/Documentation/perf-lock.txt
> > @@ -111,11 +111,11 @@ INFO OPTIONS
> >
> > -t::
> > --threads::
> > - dump thread list in perf.data
> > + dump only the thread list in perf.data
> >
> > -m::
> > --map::
> > - dump map of lock instances (address:name table)
> > + dump only the map of lock instances (address:name table)
> >
> >
> > CONTENTION OPTIONS
> > diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c
> > index 230461280e45..7eb115aeb927 100644
> > --- a/tools/perf/builtin-lock.c
> > +++ b/tools/perf/builtin-lock.c
> > @@ -1477,20 +1477,16 @@ static void dump_map(void)
> > fprintf(lock_output, " %#llx: %s\n", (unsigned long long)st->addr, st->name);
> > }
> >
> > -static int dump_info(void)
> > +static void dump_info(void)
> > {
> > - int rc = 0;
> > -
> > if (info_threads)
> > dump_threads();
> > - else if (info_map)
> > +
> > + if (info_map) {
> > + if (info_threads)
> > + fputc('\n', lock_output);
> > dump_map();
> > - else {
> > - rc = -1;
> > - pr_err("Unknown type of information\n");
> > }
> > -
> > - return rc;
> > }
> >
> > static const struct evsel_str_handler lock_tracepoints[] = {
> > @@ -1992,7 +1988,7 @@ static int __cmd_report(bool display_info)
> >
> > setup_pager();
> > if (display_info) /* used for info subcommand */
> > - err = dump_info();
> > + dump_info();
> > else {
> > combine_result();
> > sort_result();
> > @@ -2578,9 +2574,9 @@ int cmd_lock(int argc, const char **argv)
> >
> > const struct option info_options[] = {
> > OPT_BOOLEAN('t', "threads", &info_threads,
> > - "dump thread list in perf.data"),
> > + "dump the thread list in perf.data"),
> > OPT_BOOLEAN('m', "map", &info_map,
> > - "map of lock instances (address:name table)"),
> > + "dump the map of lock instances (address:name table)"),
> > OPT_PARENT(lock_options)
> > };
> >
> > @@ -2694,6 +2690,13 @@ int cmd_lock(int argc, const char **argv)
> > if (argc)
> > usage_with_options(info_usage, info_options);
> > }
> > +
> > + /* If neither threads nor map requested, display both */
> > + if (!info_threads && !info_map) {
> > + info_threads = true;
> > + info_map = true;
> > + }
> > +
> > /* recycling report_lock_ops */
> > trace_handler = &report_lock_ops;
> > rc = __cmd_report(true);