Re: [PATCH v4] perf script: Fix output type for dynamically allocated core PMU's

[Falcon, Thomas]

On Mon, 2025-03-03 at 13:16 -0800, Namhyung Kim wrote:
> Hello,
> 
> On Wed, Feb 26, 2025 at 11:00:43AM -0600, Thomas Falcon wrote:
> > This patch was originally posted here:
> > 
> > https://lore.kernel.org/all/20241213215421.661139-1-thomas.falcon@xxxxxxxxx/
> > 
> > I have rebased on top of Arnaldo's patch here:
> > 
> > https://lore.kernel.org/all/Z2XCi3PgstSrV0SE@x1/
> > 
> > The original commit message:
> > "
> > perf script output may show different fields on different core
> > PMU's
> > that exist on heterogeneous platforms. For example,
> > 
> > perf record -e "{cpu_core/mem-loads-aux/,cpu_core/event=0xcd,\
> > umask=0x01,ldlat=3,name=MEM_UOPS_RETIRED.LOAD_LATENCY/}:upp"\
> > -c10000 -W -d -a -- sleep 1
> > 
> > perf script:
> > 
> > chromium-browse   46572 [002] 544966.882384:     
> > 10000 	cpu_core/MEM_UOPS_RETIRED.LOAD_LATENCY/: 7ffdf1391b0c     10268100142\
> >  |OP LOAD|LVL L1 hit|SNP None|TLB L1 or L2 hit|LCK No|BLK    N/A   
> > 5   7    0   7fad7c47425d [unknown] (/usr/lib64/libglib-
> > 2.0.so.0.8000.3)
> > 
> > perf record -e cpu_atom/event=0xd0,umask=0x05,ldlat=3,\
> > name=MEM_UOPS_RETIRED.LOAD_LATENCY/upp -c10000 -W -d -a -- sleep 1
> > 
> > perf script:
> > 
> > gnome-control-c  534224 [023] 544951.816227:      10000
> > cpu_atom/MEM_UOPS_RETIRED.LOAD_LATENCY/:   7f0aaaa0aae0  [unknown]
> > (/usr/lib64/libglib-2.0.so.0.8000.3)
> > 
> > Some fields, such as data_src, are not included by default.
> > 
> > The cause is that while one PMU may be assigned a type such as
> > PERF_TYPE_RAW, other core PMU's are dynamically allocated at boot
> > time.
> > If this value does not match an existing PERF_TYPE_X value,
> > output_type(perf_event_attr.type) will return OUTPUT_TYPE_OTHER.
> > 
> > Instead search for a core PMU with a matching perf_event_attr type
> > and, if one is found, return PERF_TYPE_RAW to match output of other
> > core PMU's.
> > "
> > 
> > Suggested-by: Kan Liang <kan.liang@xxxxxxxxx>
> > Signed-off-by: Thomas Falcon <thomas.falcon@xxxxxxxxx>
> > ---
> > v2: restrict pmu lookup to platforms with more than one core pmu
> > v3: only scan core pmu list
> > v4: rebase on top of Arnaldo's patch
> > ---
> >  tools/perf/builtin-script.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-
> > script.c
> > index d797cec4f054..abc860d01420 100644
> > --- a/tools/perf/builtin-script.c
> > +++ b/tools/perf/builtin-script.c
> > @@ -398,10 +398,28 @@ static inline int output_type(unsigned int
> > type)
> >  	return OUTPUT_TYPE_OTHER;
> >  }
> >  
> > +static bool output_type_check_core_pmus(unsigned int type)
> 
> How about renaming to is_core_pmu_type() ?
> 
> > +{
> > +	struct perf_pmu *pmu = NULL;
> > +
> > +	if (perf_pmus__num_core_pmus() > 1) {
> > +		while ((pmu = perf_pmus__scan_core(pmu)) != NULL)
> > {
> > +			if (pmu->type == type)
> > +				return true;
> > +		}
> > +	}
> > +	return false;
> 
> To be more generic, it could be
> 
> 	return type == PERF_TYPE_RAW;
> 
> > +}
> > +
> >  static inline int evsel__output_type(struct evsel *evsel)
> >  {
> > -	if (evsel->script_output_type == OUTPUT_TYPE_UNSET)
> > -		evsel->script_output_type = output_type(evsel-
> > >core.attr.type);
> > +	if (evsel->script_output_type == OUTPUT_TYPE_UNSET) {
> > +		if (output_type(evsel->core.attr.type) ==
> > OUTPUT_TYPE_OTHER &&
> > +		    output_type_check_core_pmus(evsel-
> > >core.attr.type))
> > +			evsel->script_output_type = PERF_TYPE_RAW;
> > +		else
> > +			evsel->script_output_type =
> > output_type(evsel->core.attr.type);
> > +	}
> 
> It seems better to put this logic inside output_type().
> 
> Thanks,
> Namhyung
> 

Thanks, will send a new version soon.

Tom

> >  
> >  	return evsel->script_output_type;
> >  }
> > -- 
> > 2.48.1
> > 
>