Messages in this thread | | | Date | Wed, 8 May 2024 10:15:24 +0100 | From | Nick Forrington <> | Subject | Re: [PATCH v2 1/1] perf lock info: Display both map and thread by default |
| |
On 02/05/2024 19:32, Namhyung Kim wrote: > Hello, > > On Thu, May 2, 2024 at 2:15 AM Nick Forrington<nick.forrington@arm.com> 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<nick.forrington@arm.com> >> --- >> tools/perf/Documentation/perf-lock.txt | 4 ++-- >> tools/perf/builtin-lock.c | 22 +++++++++++++++++----- >> 2 files changed, 19 insertions(+), 7 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) > But your change allows both thread and map info to be printed, right?
Yes
>> CONTENTION OPTIONS >> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c >> index 230461280e45..86c68c39aac0 100644 >> --- a/tools/perf/builtin-lock.c >> +++ b/tools/perf/builtin-lock.c >> @@ -1483,11 +1483,16 @@ static int dump_info(void) >> >> if (info_threads) >> dump_threads(); >> - else if (info_map) >> + >> + if (info_map) { >> + if (info_threads) >> + fputc('\n', lock_output); >> dump_map(); >> - else { >> + } >> + >> + if (!info_threads && !info_map) { >> rc = -1; >> - pr_err("Unknown type of information\n"); >> + pr_err("No lock info specified\n"); >> } > I guess you made this condition impossible.
Yes. (I kept/updated the error handling for potential future errors)
>> return rc; >> @@ -2578,9 +2583,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 only the thread list in perf.data"), >> OPT_BOOLEAN('m', "map", &info_map, >> - "map of lock instances (address:name table)"), >> + "dump only the map of lock instances (address:name table)"), > Ditto, I think we can drop the 'only' part.
Ok, I'll submit a new version with this removed
> Thanks, > Namhyung > > >> OPT_PARENT(lock_options) >> }; >> >> @@ -2694,6 +2699,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 >> >>
| |