Message-ID: <aBvx-J-ISifmw0NS@google.com>
Date: Wed, 7 May 2025 16:51:20 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Dmitry Vyukov <dvyukov@...gle.com>, Ingo Molnar <mingo@...nel.org>,
	Ian Rogers <irogers@...gle.com>,
	Kan Liang <kan.liang@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-perf-users@...r.kernel.org, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [RFC/PATCH] perf report: Support latency profiling in
 system-wide mode

On Wed, May 07, 2025 at 12:47:26PM -0300, Arnaldo Carvalho de Melo wrote:
> On Wed, May 07, 2025 at 11:58:10AM +0200, Dmitry Vyukov wrote:
> > On Tue, 6 May 2025 at 18:58, Arnaldo Carvalho de Melo <acme@...nel.org> wrote:
> > > > So maybe when we notice that cycles was used 'perf report/top' should
> > > > use the term 'wall-clock' for the column name?
> 
> > > > So instead of having a --latency we could have a hint that would tell
> > > > the user that if this knob was set:
> 
> > > >   $ perf config report.wall-clock=true
>  
> > I am not sure it can be as simple as a single global knob.
> 
> > First, record needs to collect additional info and that may be
> > somewhat expensive.
>  
> > Second, report now has several modes:
> >  - it can show latency, but order by the current overhead
> >  - it can also show latency, and order by latency
> > A user wants one or another depending on what they are optimizing
> > (possibly looking at both).
>  
> > I also feel that global config switches are even less discoverable by
> > median users (read -- ~nobody will know about that). If these things
> > are normal flags with a help description, then some users may
> > eventually discover them.
> 
> So, the addition of:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e9cbc854d8b148e3491291fb615e94261970fb54
> 
>   perf config: Add a function to set one variable in .perfconfig
> 
>   To allow for setting a variable from some other tool, like with the
>   "wallclock" patchset needs to allow the user to opt-in to having
>   that key in the sort order for 'perf report'.
> 
>   Cc: Dmitriy Vyukov <dvyukov@...gle.com>
> 
> 
> Was to tell the user about the new possibilities of profiling, once,
> with something like:
> 
> root@...ber:~# perf report
>              ┌─────────────────────────────────────────────────────┐
>              │Do you want to use latency mode?                     │
>              │That will add a 'Latency' column that would mean     │
>              │'wall-clock' time when used with cycles, for instance│
>              │                                                     │
>              │                                                     │
>              │Enter: Yes, ESC: No                                  │
>              └─────────────────────────────────────────────────────┘
>  
> The working should inform if the current perf.data has what is needed
> and thus this would take immediate effect, and if not, inform that as
> well and what is needed to be able to use that, things like you
> described above:

Looks useful!  Maybe we want to add it to interactive TUI only..

> 
> > First, record needs to collect additional info and that may be
> > somewhat expensive.
> 
> If the user says that the feature is desired by pressing Enter, follow
> up questions can be asked as well, like you described:

I'm afraid it can have a long chain of questions when a new user starts
to use the perf tool with any config.  Maybe we need to limit it somehow.

> 
> > Second, report now has several modes:
> 
> >  - it can show latency, but order by the current overhead
> >  - it can also show latency, and order by latency
> 
> > A user wants one or another depending on what they are optimizing
> > (possibly looking at both).
> 
> Once these are answered, the questions goes away, as they are now
> recorded in the user's ~/.perfconfig file and will be used when what is
> needed is present in perf.data file.

Sounds good.

> 
> The user should also be informed that to change that knob, 'perf config'
> is your friend.
> 
> Additionally some hotkey could be made available to change that
> behaviour, on the fly, with the option of turning the new mode the new
> default, again writing to ~/.perfconfig.
> 
> Sometimes this "on the fly" needs reprocessing of the whole perf.data
> file, sometimes it may be a matter of just showing extra available
> fields, like the 'V' key now, that goes on bumping the verbosity in the
> 'perf report' and 'perf top' browsers (they share the hists browser), or
> 'j' that toogles showing the lines from jump sources to jump targets in
> the annotate browser, 'J' to show how many jumps target a particular
> jump target, also in the annotate browser, etc.
> 
> This could help experimenting with the various modes of doing profiling,
> interactively, to see the problem from different angles, without having
> to restart the whole process by exiting the 'perf report' to add new
> command line options, then restart, etc, reusing computation sometimes
> when switching views, etc.
> 
> To get default behaviour its just a matter of renaming ~/.perfconfig to
> ~/.perfconfig.some_suitabe_name_for_later_use.
> 
> This way interesting new features like this don't get buried behind
> either command line options nor man pages, giving the user at least an
> opportunity to be informed about it.
> 
> That is what the patch below barely starts sketching, not doing all I
> just described.

Sounds like a good idea.

Thanks,
Namhyung

Powered by blists - more mailing lists