Messages in this thread | | | From | Namhyung Kim <> | Date | Sun, 12 May 2024 18:41:36 -0700 | Subject | Re: [PATCH 1/1] perf lock info: Display both map and thread by default |
| |
On Thu, May 9, 2024 at 3:01 AM Nick Forrington <nick.forrington@armcom> wrote: > > Hello, > > On 08/05/2024 22:18, Namhyung Kim wrote: > > Hello, > > > > On Wed, May 8, 2024 at 6:46 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) > >> > >> > >> CONTENTION OPTIONS > >> diff --git a/tools/perf/builtin-lock.c b/tools/perf/builtin-lock.c > >> index 230461280e45..d0b8645487ad 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); > > it seems you need pr_info("\n"). Where does lock_output come from? > > > It's a static FILE* that defaults to stderr.
Oh, right. I somehow confused this with perf sched. ;-p
> > > It's used by dump_threads and dump_map (via fprintf), so I think it > makes sense to use it for the output spacing between them? > > > > > > >> 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 think we can remove this block now. > > > The original error looked like a function input sanity check (rather > than for handling user input), so I kept it (even if it should no longer > be possible for a user to trigger it). > > > Although I can remove it (or move it to the start of the function) if > you prefer.
Yep, I think we can just remove it.
Thanks, Namhyung
| |