Re: [PATCH 04/10] perf stat: Add --exclude-guest option

[Namhyung Kim <namhyung@kernel.org>]

Hello,

On Mon, Sep 23, 2024 at 09:47:17AM +0100, James Clark wrote:
> 
> 
> On 06/09/2024 3:33 pm, Liang, Kan wrote:
> > 
> > 
> > On 2024-09-05 4:24 p.m., Namhyung Kim wrote:
> > > This option is to support the old behavior of setting exclude_guest by
> > > default.  Now it doesn't set the bit so users want the old behavior can
> > > use this option.
> > > 
> > >    $ perf stat true
> > > 
> > >     Performance counter stats for 'true':
> > > 
> > >                  0.86 msec task-clock:u                     #    0.443 CPUs utilized
> > >                     0      context-switches:u               #    0.000 /sec
> > >                     0      cpu-migrations:u                 #    0.000 /sec
> > >                    49      page-faults:u                    #   56.889 K/sec
> > >                   ...
> > > 
> > >    $ perf stat --exclude-guest true
> > > 
> > >     Performance counter stats for 'true':
> > > 
> > >                  0.79 msec task-clock:Hu                    #    0.490 CPUs utilized
> > >                     0      context-switches:Hu              #    0.000 /sec
> > >                     0      cpu-migrations:Hu                #    0.000 /sec
> > >                    49      page-faults:Hu                   #   62.078 K/sec
> > >                   ...
> > > 
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > > ---
> > >   tools/perf/Documentation/perf-stat.txt | 7 +++++++
> > >   tools/perf/builtin-stat.c              | 2 ++
> > >   2 files changed, 9 insertions(+)
> > > 
> > > diff --git a/tools/perf/Documentation/perf-stat.txt b/tools/perf/Documentation/perf-stat.txt
> > > index 2bc06367248691dd..d28d8370a856598f 100644
> > > --- a/tools/perf/Documentation/perf-stat.txt
> > > +++ b/tools/perf/Documentation/perf-stat.txt
> > > @@ -382,6 +382,13 @@ color the metric's computed value.
> > >   Don't print output, warnings or messages. This is useful with perf stat
> > >   record below to only write data to the perf.data file.
> > > +--exclude-guest::
> > > +Don't count event in the guest mode.  It was the old behavior but the
> > > +default is changed to count guest events also.  Use this option if you
> > > +want the old behavior (host only).  Note that this option needs to be
> > > +before other events in case you added -e/--event option in the command
> > > +line.
> > 
> > I'm not sure if we really need this option. I think it may bring more
> > trouble than what we get.
> > 
> > The name of the "--exclude-guest" sounds like a replacement of the event
> > modifier "H". But in fact, it's not. It should only affect the default.
> > It doesn't set the "H" for any events.

Well I think it's tricky but it'd set "H" modifier events after the
option.  But I have to agree that it can bring more troubles.

> > 
> > Except for the perf kvm user, I don't think there are many users which
> > care the exclude_guest. The behavior of the perf kvm is not changed. So
> > the option seems not that important. If we really want an option to
> > restore the old behavior, it's better to choose a better name and update
> > the description.

Personally I don't want to this option but just worried if there's a
case where exclude_guest is preferred.

> 
> Do we not want to keep exclude_guest for record, but remove it for stat?

What I really want is to synchronize record and stat behavior in terms
of the default exclusion mode.  If everyone is fine, I'd like to remove
exclude_guest from the default and set it only if needed (through the
fallback) like we do for exclude_kernel.

> 
> Because in record the addresses of guest samples don't make sense without
> extra work, but for stat you might want to see an overview of the whole
> system.

I think it depends on the use case and (power) users should know about
their environment and requirement.  The concern is what's the reasonable
default, but I think it should be the same both in perf record and stat
at least.

> 
> For Coresight tracing and SPE we would want to keep exclude_guest, otherwise
> you generate a load of extra trace that you can't make use of. Say you were
> doing PGO on your host you wouldn't be recompiling anything the guests were
> running.

For the specific use case, I think we can guide users to add "H"
modifier (I guess it's not the default event for perf record).
Maybe we can consider per-PMU default attributes.

> 
> If we do change the defaults isn't ':H' already enough to go back to the old
> behavior? I'm wondering why we need an argument when all the other exclude
> rules are done with the letter modifiers?

I'm not sure I follow this.  But maybe we don't need this option at all.
We can add ":H" for every event but I'm too lazy to add them to all the
default events in perf data. :)

Thanks,
Namhyung
 
> > > +
> > >   STAT RECORD
> > >   -----------
> > >   Stores stat data into perf data file.
> > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > > index d8315dae930184ba..4d47675af5cc3094 100644
> > > --- a/tools/perf/builtin-stat.c
> > > +++ b/tools/perf/builtin-stat.c
> > > @@ -2491,6 +2491,8 @@ int cmd_stat(int argc, const char **argv)
> > >   		OPT_BOOLEAN_FLAG(0, "all-user", &stat_config.all_user,
> > >   				"Configure all used events to run in user space.",
> > >   				PARSE_OPT_EXCLUSIVE),
> > > +		OPT_BOOLEAN(0, "exclude-guest", &exclude_HG_default,
> > > +			"Don't count events in the guest mode"),
> > >   		OPT_BOOLEAN(0, "percore-show-thread", &stat_config.percore_show_thread,
> > >   			"Use with 'percore' event qualifier to show the event "
> > >   			"counts of one hardware thread by sum up total hardware "
>