Re: [PATCH] perf script: Fix ip display when type != attr->type

[Arnaldo Carvalho de Melo]

Em Mon, Sep 13, 2021 at 11:13:56AM -0400, Liang, Kan escreveu:
> 
> 
> On 9/11/2021 9:30 AM, Adrian Hunter wrote:
> > set_print_ip_opts() was not being called when type != attr->type
> > because there is not a one-to-one relationship between output types
> > and attr->type. That resulted in ip not printing.
> > 
> > The attr_type() function is removed, and the match of attr->type to
> > output type is corrected.
> > 
> > Example on ADL using taskset to select an atom cpu:
> > 
> >   # perf record -e cpu_atom/cpu-cycles/ taskset 0x1000 uname
> >   Linux
> >   [ perf record: Woken up 1 times to write data ]
> >   [ perf record: Captured and wrote 0.003 MB perf.data (7 samples) ]
> > 
> >   Before:
> > 
> >    # perf script | head
> >           taskset   428 [-01] 10394.179041:          1 cpu_atom/cpu-cycles/:
> >           taskset   428 [-01] 10394.179043:          1 cpu_atom/cpu-cycles/:
> >           taskset   428 [-01] 10394.179044:         11 cpu_atom/cpu-cycles/:
> >           taskset   428 [-01] 10394.179045:        407 cpu_atom/cpu-cycles/:
> >           taskset   428 [-01] 10394.179046:      16789 cpu_atom/cpu-cycles/:
> >           taskset   428 [-01] 10394.179052:     676300 cpu_atom/cpu-cycles/:
> >             uname   428 [-01] 10394.179278:    4079859 cpu_atom/cpu-cycles/:
> > 
> >   After:
> > 
> >    # perf script | head
> >           taskset   428 10394.179041:          1 cpu_atom/cpu-cycles/:  ffffffff95a0bb97 __intel_pmu_enable_all.constprop.48+0x47 ([kernel.kallsyms])
> >           taskset   428 10394.179043:          1 cpu_atom/cpu-cycles/:  ffffffff95a0bb97 __intel_pmu_enable_all.constprop.48+0x47 ([kernel.kallsyms])
> >           taskset   428 10394.179044:         11 cpu_atom/cpu-cycles/:  ffffffff95a0bb97 __intel_pmu_enable_all.constprop.48+0x47 ([kernel.kallsyms])
> >           taskset   428 10394.179045:        407 cpu_atom/cpu-cycles/:  ffffffff95a0bb97 __intel_pmu_enable_all.constprop.48+0x47 ([kernel.kallsyms])
> >           taskset   428 10394.179046:      16789 cpu_atom/cpu-cycles/:  ffffffff95a0bb97 __intel_pmu_enable_all.constprop.48+0x47 ([kernel.kallsyms])
> >           taskset   428 10394.179052:     676300 cpu_atom/cpu-cycles/:      7f829ef73800 cfree+0x0 (/lib/libc-2.32.so)
> >             uname   428 10394.179278:    4079859 cpu_atom/cpu-cycles/:  ffffffff95bae912 vma_interval_tree_remove+0x1f2 ([kernel.kallsyms])
> > 
> > Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> > ---
> >   tools/perf/builtin-script.c | 24 +++++++++++++-----------
> >   1 file changed, 13 insertions(+), 11 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> > index 0e824f7d8b19..6211d0b84b7a 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -368,16 +368,6 @@ static inline int output_type(unsigned int type)
> >   	return OUTPUT_TYPE_OTHER;
> >   }
> > -static inline unsigned int attr_type(unsigned int type)
> > -{
> > -	switch (type) {
> > -	case OUTPUT_TYPE_SYNTH:
> > -		return PERF_TYPE_SYNTH;
> > -	default:
> > -		return type;
> > -	}
> > -}
> > -
> >   static bool output_set_by_user(void)
> >   {
> >   	int j;
> > @@ -556,6 +546,18 @@ static void set_print_ip_opts(struct perf_event_attr *attr)
> >   		output[type].print_ip_opts |= EVSEL__PRINT_SRCLINE;
> >   }
> > +static struct evsel *find_first_output_type(struct evlist *evlist,
> > +					    unsigned int type)
> > +{
> > +	struct evsel *evsel;
> > +
> > +	evlist__for_each_entry(evlist, evsel) {
> > +		if (output_type(evsel->core.attr.type) == (int)type)
> > +			return evsel;
> > +	}
> > +	return NULL;
> > +}
> > +
> >   /*
> >    * verify all user requested events exist and the samples
> >    * have the expected data
> > @@ -567,7 +569,7 @@ static int perf_session__check_output_opt(struct perf_session *session)
> >   	struct evsel *evsel;
> >   	for (j = 0; j < OUTPUT_TYPE_MAX; ++j) {
> > -		evsel = perf_session__find_first_evtype(session, attr_type(j));
> 
> I think the only consumer of perf_session__find_first_evtype() will only be
> in session.c. Seems we can further clean up the function and make it static.
> Other than that, the patch looks good to me.
> 
>   Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>

Thanks, applied.

- Arnaldo