Re: [PATCH v1 3/5] perf tools: Check if mem_events is supported for hybrid

[Jin, Yao]

Hi Jiri,

On 5/25/2021 1:19 AM, Jiri Olsa wrote:
> On Thu, May 20, 2021 at 03:00:38PM +0800, Jin Yao wrote:
> > Check if the mem_events ('mem-loads' and 'mem-stores') exist
> > in the sysfs path.
> >
> > For Alderlake, the hybrid cpu pmu are "cpu_core" and "cpu_atom".
> > Check the existing of following paths:
> > /sys/devices/cpu_atom/events/mem-loads
> > /sys/devices/cpu_atom/events/mem-stores
> > /sys/devices/cpu_core/events/mem-loads
> > /sys/devices/cpu_core/events/mem-stores
> >
> > If the patch exists, the mem_event is supported.
> >
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/util/mem-events.c | 43 +++++++++++++++++++++++++++++-------
> >   1 file changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> > index c736eaded06c..e8f6e745eaf0 100644
> > --- a/tools/perf/util/mem-events.c
> > +++ b/tools/perf/util/mem-events.c
> > @@ -12,14 +12,16 @@
> >   #include "mem-events.h"
> >   #include "debug.h"
> >   #include "symbol.h"
> > +#include "pmu.h"
> > +#include "pmu-hybrid.h"
> >   
> >   unsigned int perf_mem_events__loads_ldlat = 30;
> >   
> >   #define E(t, n, s) { .tag = t, .name = n, .sysfs_name = s }
> >   
> >   static struct perf_mem_event perf_mem_events[PERF_MEM_EVENTS__MAX] = {
> > -	E("ldlat-loads",	"cpu/mem-loads,ldlat=%u/P",	"cpu/events/mem-loads"),
> > -	E("ldlat-stores",	"cpu/mem-stores/P",		"cpu/events/mem-stores"),
> > +	E("ldlat-loads",	"%s/mem-loads,ldlat=%u/P",	"%s/events/mem-loads"),
> > +	E("ldlat-stores",	"%s/mem-stores/P",		"%s/events/mem-stores"),
> >   	E(NULL,			NULL,				NULL),
>
> so this was generic place, now it's x86 specific, I wonder we should
> move it under arch/x86 to avoid confusion

Yes, I will move x86 specific perf_mem_events[] to arch/x86/util/mem-events.c.

> >   };
> >   #undef E
> > @@ -100,6 +102,18 @@ int perf_mem_events__parse(const char *str)
> >   	return -1;
> >   }
> >   
> > +static bool perf_mem_events__supported(const char *mnt, char *sysfs_name)
> > +{
> > +	char path[PATH_MAX];
> > +	struct stat st;
> > +
> > +	scnprintf(path, PATH_MAX, "%s/devices/%s", mnt, sysfs_name);
> > +	if (!stat(path, &st))
> > +		return true;
> > +
> > +	return false;
>
> could be just 'return !stat(path, &st);' right?

Right :)

> > +}
> > +
> >   int perf_mem_events__init(void)
> >   {
> >   	const char *mnt = sysfs__mount();
> > @@ -110,9 +124,10 @@ int perf_mem_events__init(void)
> >   		return -ENOENT;
> >   
> >   	for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
> > -		char path[PATH_MAX];
> >   		struct perf_mem_event *e = perf_mem_events__ptr(j);
> > -		struct stat st;
> > +		struct perf_pmu *pmu;
> > +		char sysfs_name[100];
> > +		int unsupported = 0;
> >   
> >   		/*
> >   		 * If the event entry isn't valid, skip initialization
> > @@ -121,11 +136,23 @@ int perf_mem_events__init(void)
> >   		if (!e->tag)
> >   			continue;
> >   
> > -		scnprintf(path, PATH_MAX, "%s/devices/%s",
> > -			  mnt, e->sysfs_name);
> > +		if (!perf_pmu__has_hybrid()) {
> > +			scnprintf(sysfs_name, sizeof(sysfs_name),
> > +				  e->sysfs_name, "cpu");
> > +			e->supported = perf_mem_events__supported(mnt, sysfs_name);
> > +		} else {
> > +			perf_pmu__for_each_hybrid_pmu(pmu) {
> > +				scnprintf(sysfs_name, sizeof(sysfs_name),
> > +					  e->sysfs_name, pmu->name);
> > +				if (!perf_mem_events__supported(mnt, sysfs_name))
> > +					unsupported++;
> > +			}
> > +
> > +			e->supported = (unsupported == 0) ? true : false;
>
> could you just do in the above loop:
> 			e->supported |= perf_mem_events__supported(mnt, sysfs_name);

That's OK, thanks!

Thanks
Jin Yao

> jirka
>
> > +		}
> >   
> > -		if (!stat(path, &st))
> > -			e->supported = found = true;
> > +		if (e->supported)
> > +			found = true;
> >   	}
> >   
> >   	return found ? 0 : -ENOENT;
> > --
> > 2.17.1