Re: [PATCH v1 2/5] perf tools: Support pmu name in perf_mem_events__name

[Jin, Yao]

Hi Jiri,

On 5/25/2021 1:20 AM, Jiri Olsa wrote:
> On Thu, May 20, 2021 at 03:00:37PM +0800, Jin Yao wrote:
>
> SNIP
>
> > --- a/tools/perf/arch/x86/util/mem-events.c
> > +++ b/tools/perf/arch/x86/util/mem-events.c
> > @@ -4,10 +4,10 @@
> >   #include "mem-events.h"
> >   
> >   static char mem_loads_name[100];
> > -static bool mem_loads_name__init;
> > +static char mem_stores_name[100];
> >   
> >   #define MEM_LOADS_AUX		0x8203
> > -#define MEM_LOADS_AUX_NAME	"{cpu/mem-loads-aux/,cpu/mem-loads,ldlat=%u/pp}:S"
> > +#define MEM_LOADS_AUX_NAME     "{%s/mem-loads-aux/,%s/mem-loads,ldlat=%u/}:P"
> >   
> >   bool is_mem_loads_aux_event(struct evsel *leader)
> >   {
> > @@ -22,28 +22,34 @@ bool is_mem_loads_aux_event(struct evsel *leader)
> >   	return leader->core.attr.config == MEM_LOADS_AUX;
> >   }
> >   
> > -char *perf_mem_events__name(int i)
> > +char *perf_mem_events__name(int i, char *pmu_name)
> >   {
> >   	struct perf_mem_event *e = perf_mem_events__ptr(i);
> >   
> >   	if (!e)
> >   		return NULL;
> >   
> > -	if (i == PERF_MEM_EVENTS__LOAD) {
> > -		if (mem_loads_name__init)
> > -			return mem_loads_name;
> > -
> > -		mem_loads_name__init = true;
> > +	if (!pmu_name)
> > +		pmu_name = (char *)"cpu";
> >   
> > -		if (pmu_have_event("cpu", "mem-loads-aux")) {
> > +	if (i == PERF_MEM_EVENTS__LOAD) {
> > +		if (pmu_have_event(pmu_name, "mem-loads-aux")) {
> >   			scnprintf(mem_loads_name, sizeof(mem_loads_name),
> > -				  MEM_LOADS_AUX_NAME, perf_mem_events__loads_ldlat);
> > +				  MEM_LOADS_AUX_NAME, pmu_name, pmu_name,
> > +				  perf_mem_events__loads_ldlat);
> >   		} else {
> >   			scnprintf(mem_loads_name, sizeof(mem_loads_name),
> > -				  e->name, perf_mem_events__loads_ldlat);
> > +				  e->name, pmu_name,
> > +				  perf_mem_events__loads_ldlat);
> >   		}
> >   		return mem_loads_name;
> >   	}
> >   
> > +	if (i == PERF_MEM_EVENTS__STORE) {
> > +		scnprintf(mem_stores_name, sizeof(mem_stores_name),
> > +			  e->name, pmu_name);
> > +		return mem_stores_name;
> > +	}
>
> so the patch also adds mem_stores_name and removes mem_loads_name__init,
> that should be explained or more likely in separated patches
>
> jirka

I will not remove mem_loads_name__init in order to keep the original behavior for non-hybrid platform.

I can move the mem_store to a separate patch.

Thanks
Jin Yao