[PATCH v1 0/5] perf: Support perf-mem/perf-c2c for AlderLake

[PATCH v1 0/5] perf: Support perf-mem/perf-c2c for AlderLake

[Jin Yao]

AlderLake uses a hybrid architecture utilizing Golden Cove cores
(core CPU) and Gracemont cores (atom CPU). This patchset supports
perf-mem and perf-c2c for AlderLake. 

Jin Yao (5):
  perf util: Check mem-loads auxiliary event
  perf tools: Support pmu name in perf_mem_events__name
  perf tools: Check if mem_events is supported for hybrid
  perf mem: Support record for hybrid platform
  perf c2c: Support record for hybrid platform

 tools/perf/arch/arm64/util/mem-events.c   |   2 +-
 tools/perf/arch/powerpc/util/mem-events.c |   2 +-
 tools/perf/arch/x86/util/mem-events.c     |  37 ++++---
 tools/perf/builtin-c2c.c                  |  36 +++----
 tools/perf/builtin-mem.c                  |  39 ++++----
 tools/perf/util/mem-events.c              | 112 ++++++++++++++++++++--
 tools/perf/util/mem-events.h              |   4 +-
 7 files changed, 172 insertions(+), 60 deletions(-)

-- 
2.17.1

[PATCH v1 1/5] perf util: Check mem-loads auxiliary event

[Jin Yao]

For some platforms, an auxiliary event has to be enabled
simultaneously with the load latency event.

For Alderlake, the auxiliary event is created in "cpu_core" pmu.

So first we need to check the existing of "cpu_core" pmu
and then check if this pmu has auxiliary event.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/arch/x86/util/mem-events.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/perf/arch/x86/util/mem-events.c b/tools/perf/arch/x86/util/mem-events.c
index 588110fd8904..e79232e3f2a0 100644
--- a/tools/perf/arch/x86/util/mem-events.c
+++ b/tools/perf/arch/x86/util/mem-events.c
@@ -11,8 +11,13 @@ static bool mem_loads_name__init;
 
 bool is_mem_loads_aux_event(struct evsel *leader)
 {
-	if (!pmu_have_event("cpu", "mem-loads-aux"))
-		return false;
+	if (perf_pmu__find("cpu")) {
+		if (!pmu_have_event("cpu", "mem-loads-aux"))
+			return false;
+	} else if (perf_pmu__find("cpu_core")) {
+		if (!pmu_have_event("cpu_core", "mem-loads-aux"))
+			return false;
+	}
 
 	return leader->core.attr.config == MEM_LOADS_AUX;
 }
-- 
2.17.1

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

[Jin Yao]

The perf_mem_events__name assumes the pmu is "cpu" but
it's not true for hybrid platform. For example, for Alderlake,
one pmu is "cpu_core".

We introduce a new parameter 'pmu_name' in perf_mem_events__name
to let the caller specify a pmu name.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/arch/arm64/util/mem-events.c   |  2 +-
 tools/perf/arch/powerpc/util/mem-events.c |  2 +-
 tools/perf/arch/x86/util/mem-events.c     | 28 ++++++++++++++---------
 tools/perf/builtin-c2c.c                  |  4 ++--
 tools/perf/builtin-mem.c                  |  4 ++--
 tools/perf/util/mem-events.c              |  4 ++--
 tools/perf/util/mem-events.h              |  2 +-
 7 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/tools/perf/arch/arm64/util/mem-events.c b/tools/perf/arch/arm64/util/mem-events.c
index 2a2497372671..be41721b9aa1 100644
--- a/tools/perf/arch/arm64/util/mem-events.c
+++ b/tools/perf/arch/arm64/util/mem-events.c
@@ -20,7 +20,7 @@ struct perf_mem_event *perf_mem_events__ptr(int i)
 	return &perf_mem_events[i];
 }
 
-char *perf_mem_events__name(int i)
+char *perf_mem_events__name(int i, char *pmu_name __maybe_unused)
 {
 	struct perf_mem_event *e = perf_mem_events__ptr(i);
 
diff --git a/tools/perf/arch/powerpc/util/mem-events.c b/tools/perf/arch/powerpc/util/mem-events.c
index 07fb5e049488..4120fafe0be4 100644
--- a/tools/perf/arch/powerpc/util/mem-events.c
+++ b/tools/perf/arch/powerpc/util/mem-events.c
@@ -3,7 +3,7 @@
 #include "mem-events.h"
 
 /* PowerPC does not support 'ldlat' parameter. */
-char *perf_mem_events__name(int i)
+char *perf_mem_events__name(int i, char *pmu_name __maybe_unused)
 {
 	if (i == PERF_MEM_EVENTS__LOAD)
 		return (char *) "cpu/mem-loads/";
diff --git a/tools/perf/arch/x86/util/mem-events.c b/tools/perf/arch/x86/util/mem-events.c
index e79232e3f2a0..e50f9e3a695b 100644
--- 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;
+	}
+
 	return (char *)e->name;
 }
diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index e3b9d63077ef..a4fd375acdd1 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2971,13 +2971,13 @@ static int perf_c2c__record(int argc, const char **argv)
 
 		if (!e->supported) {
 			pr_err("failed: event '%s' not supported\n",
-			       perf_mem_events__name(j));
+			       perf_mem_events__name(j, NULL));
 			free(rec_argv);
 			return -1;
 		}
 
 		rec_argv[i++] = "-e";
-		rec_argv[i++] = perf_mem_events__name(j);
+		rec_argv[i++] = perf_mem_events__name(j, NULL);
 	}
 
 	if (all_user)
diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
index cdd2b9f643f6..03795bf49d51 100644
--- a/tools/perf/builtin-mem.c
+++ b/tools/perf/builtin-mem.c
@@ -135,13 +135,13 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 
 		if (!e->supported) {
 			pr_err("failed: event '%s' not supported\n",
-			       perf_mem_events__name(j));
+			       perf_mem_events__name(j, NULL));
 			free(rec_argv);
 			return -1;
 		}
 
 		rec_argv[i++] = "-e";
-		rec_argv[i++] = perf_mem_events__name(j);
+		rec_argv[i++] = perf_mem_events__name(j, NULL);
 	}
 
 	if (all_user)
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index f93a852ad838..c736eaded06c 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -37,7 +37,7 @@ struct perf_mem_event * __weak perf_mem_events__ptr(int i)
 	return &perf_mem_events[i];
 }
 
-char * __weak perf_mem_events__name(int i)
+char * __weak perf_mem_events__name(int i, char *pmu_name  __maybe_unused)
 {
 	struct perf_mem_event *e = perf_mem_events__ptr(i);
 
@@ -141,7 +141,7 @@ void perf_mem_events__list(void)
 		fprintf(stderr, "%-13s%-*s%s\n",
 			e->tag ?: "",
 			verbose > 0 ? 25 : 0,
-			verbose > 0 ? perf_mem_events__name(j) : "",
+			verbose > 0 ? perf_mem_events__name(j, NULL) : "",
 			e->supported ? ": available" : "");
 	}
 }
diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
index cacdebd65b8a..a3fa19093fd2 100644
--- a/tools/perf/util/mem-events.h
+++ b/tools/perf/util/mem-events.h
@@ -38,7 +38,7 @@ extern unsigned int perf_mem_events__loads_ldlat;
 int perf_mem_events__parse(const char *str);
 int perf_mem_events__init(void);
 
-char *perf_mem_events__name(int i);
+char *perf_mem_events__name(int i, char *pmu_name);
 struct perf_mem_event *perf_mem_events__ptr(int i);
 bool is_mem_loads_aux_event(struct evsel *leader);
 
-- 
2.17.1

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

[Jiri Olsa]

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

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

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

[Jin Yao]

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@linux.intel.com>
---
 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),
 };
 #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;
+}
+
 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;
+		}
 
-		if (!stat(path, &st))
-			e->supported = found = true;
+		if (e->supported)
+			found = true;
 	}
 
 	return found ? 0 : -ENOENT;
-- 
2.17.1

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

[Jiri Olsa]

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@linux.intel.com>
> ---
>  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

>  };
>  #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?

> +}
> +
>  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);

jirka

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

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@linux.intel.com>
>> ---
>>   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
>>
> 

[PATCH v1 4/5] perf mem: Support record for hybrid platform

[Jin Yao]

Support 'perf mem record' for hybrid platform. On hybrid platform,
such as Alderlake, when executing 'perf mem record', it actually calls:

record -e {cpu_core/mem-loads-aux/,cpu_core/mem-loads,ldlat=30/}:P
       -e cpu_atom/mem-loads,ldlat=30/P
       -e cpu_core/mem-stores/P
       -e cpu_atom/mem-stores/P

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-mem.c     | 39 ++++++++++++----------
 tools/perf/util/mem-events.c | 65 ++++++++++++++++++++++++++++++++++++
 tools/perf/util/mem-events.h |  2 ++
 3 files changed, 89 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
index 03795bf49d51..a50abcb45f0f 100644
--- a/tools/perf/builtin-mem.c
+++ b/tools/perf/builtin-mem.c
@@ -62,8 +62,9 @@ static const char * const *record_mem_usage = __usage;
 
 static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 {
-	int rec_argc, i = 0, j;
+	int rec_argc, i = 0, j,  tmp_nr = 0;
 	const char **rec_argv;
+	char **rec_tmp;
 	int ret;
 	bool all_user = false, all_kernel = false;
 	struct perf_mem_event *e;
@@ -87,11 +88,20 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 	argc = parse_options(argc, argv, options, record_mem_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	rec_argc = argc + 9; /* max number of arguments */
+	rec_argc = argc + 64; /* max number of arguments */
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -1;
 
+	/*
+	 * Save the allocated event name strings.
+	 */
+	rec_tmp = calloc(rec_argc + 1, sizeof(char *));
+	if (!rec_tmp) {
+		free(rec_argv);
+		return -1;
+	}
+
 	rec_argv[i++] = "record";
 
 	e = perf_mem_events__ptr(PERF_MEM_EVENTS__LOAD_STORE);
@@ -128,21 +138,9 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 	if (mem->data_page_size)
 		rec_argv[i++] = "--data-page-size";
 
-	for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
-		e = perf_mem_events__ptr(j);
-		if (!e->record)
-			continue;
-
-		if (!e->supported) {
-			pr_err("failed: event '%s' not supported\n",
-			       perf_mem_events__name(j, NULL));
-			free(rec_argv);
-			return -1;
-		}
-
-		rec_argv[i++] = "-e";
-		rec_argv[i++] = perf_mem_events__name(j, NULL);
-	}
+	ret = perf_mem_events__record_args(rec_argv, &i, rec_tmp, &tmp_nr);
+	if (ret)
+		goto out;
 
 	if (all_user)
 		rec_argv[i++] = "--all-user";
@@ -164,6 +162,13 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
 	}
 
 	ret = cmd_record(i, rec_argv);
+out:
+	for (i = 0; i < tmp_nr; i++) {
+		if (rec_tmp[i])
+			free(rec_tmp[i]);
+	}
+
+	free(rec_tmp);
 	free(rec_argv);
 	return ret;
 }
diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
index e8f6e745eaf0..909ee91b75f0 100644
--- a/tools/perf/util/mem-events.c
+++ b/tools/perf/util/mem-events.c
@@ -173,6 +173,71 @@ void perf_mem_events__list(void)
 	}
 }
 
+static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
+						    int idx)
+{
+	const char *mnt = sysfs__mount();
+	char sysfs_name[100];
+	struct perf_pmu *pmu;
+
+	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)) {
+			pr_err("failed: event '%s' not supported\n",
+			       perf_mem_events__name(idx, pmu->name));
+		}
+	}
+}
+
+int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
+				 char **rec_tmp, int *tmp_nr)
+{
+	int i = *argv_nr, k = 0;
+	struct perf_mem_event *e;
+	struct perf_pmu *pmu;
+	char *s;
+
+	for (int j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
+		e = perf_mem_events__ptr(j);
+		if (!e->record)
+			continue;
+
+		if (!perf_pmu__has_hybrid()) {
+			if (!e->supported) {
+				pr_err("failed: event '%s' not supported\n",
+				       perf_mem_events__name(j, NULL));
+				return -1;
+			}
+
+			rec_argv[i++] = "-e";
+			rec_argv[i++] = perf_mem_events__name(j, NULL);
+		} else {
+			if (!e->supported) {
+				perf_mem_events__print_unsupport_hybrid(e, j);
+				return -1;
+			}
+
+			perf_pmu__for_each_hybrid_pmu(pmu) {
+				rec_argv[i++] = "-e";
+				s = perf_mem_events__name(j, pmu->name);
+				if (s) {
+					s = strdup(s);
+					if (!s)
+						return -1;
+
+					rec_argv[i++] = s;
+					rec_tmp[k++] = s;
+				}
+			}
+		}
+	}
+
+	*argv_nr = i;
+	*tmp_nr = k;
+	return 0;
+}
+
 static const char * const tlb_access[] = {
 	"N/A",
 	"HIT",
diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
index a3fa19093fd2..916242f8020a 100644
--- a/tools/perf/util/mem-events.h
+++ b/tools/perf/util/mem-events.h
@@ -43,6 +43,8 @@ struct perf_mem_event *perf_mem_events__ptr(int i);
 bool is_mem_loads_aux_event(struct evsel *leader);
 
 void perf_mem_events__list(void);
+int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
+				 char **rec_tmp, int *tmp_nr);
 
 int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
 int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
-- 
2.17.1

Re: [PATCH v1 4/5] perf mem: Support record for hybrid platform

[Jiri Olsa]

On Thu, May 20, 2021 at 03:00:39PM +0800, Jin Yao wrote:
> Support 'perf mem record' for hybrid platform. On hybrid platform,
> such as Alderlake, when executing 'perf mem record', it actually calls:
> 
> record -e {cpu_core/mem-loads-aux/,cpu_core/mem-loads,ldlat=30/}:P
>        -e cpu_atom/mem-loads,ldlat=30/P
>        -e cpu_core/mem-stores/P
>        -e cpu_atom/mem-stores/P
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-mem.c     | 39 ++++++++++++----------
>  tools/perf/util/mem-events.c | 65 ++++++++++++++++++++++++++++++++++++
>  tools/perf/util/mem-events.h |  2 ++
>  3 files changed, 89 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
> index 03795bf49d51..a50abcb45f0f 100644
> --- a/tools/perf/builtin-mem.c
> +++ b/tools/perf/builtin-mem.c
> @@ -62,8 +62,9 @@ static const char * const *record_mem_usage = __usage;
>  
>  static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>  {
> -	int rec_argc, i = 0, j;
> +	int rec_argc, i = 0, j,  tmp_nr = 0;
>  	const char **rec_argv;
> +	char **rec_tmp;
>  	int ret;
>  	bool all_user = false, all_kernel = false;
>  	struct perf_mem_event *e;
> @@ -87,11 +88,20 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>  	argc = parse_options(argc, argv, options, record_mem_usage,
>  			     PARSE_OPT_KEEP_UNKNOWN);
>  
> -	rec_argc = argc + 9; /* max number of arguments */
> +	rec_argc = argc + 64; /* max number of arguments */

please add comment on how you got the number 64


>  	rec_argv = calloc(rec_argc + 1, sizeof(char *));
>  	if (!rec_argv)
>  		return -1;
>  
> +	/*
> +	 * Save the allocated event name strings.
> +	 */
> +	rec_tmp = calloc(rec_argc + 1, sizeof(char *));
> +	if (!rec_tmp) {
> +		free(rec_argv);
> +		return -1;
> +	}

why not do strdup on all of them and always call free instead?
that would get rid of the rec_tmp and tmp_nr

> +
>  	rec_argv[i++] = "record";
>  
>  	e = perf_mem_events__ptr(PERF_MEM_EVENTS__LOAD_STORE);
> @@ -128,21 +138,9 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>  	if (mem->data_page_size)
>  		rec_argv[i++] = "--data-page-size";
>  
> -	for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
> -		e = perf_mem_events__ptr(j);
> -		if (!e->record)
> -			continue;
> -
> -		if (!e->supported) {
> -			pr_err("failed: event '%s' not supported\n",
> -			       perf_mem_events__name(j, NULL));
> -			free(rec_argv);
> -			return -1;
> -		}
> -
> -		rec_argv[i++] = "-e";
> -		rec_argv[i++] = perf_mem_events__name(j, NULL);
> -	}
> +	ret = perf_mem_events__record_args(rec_argv, &i, rec_tmp, &tmp_nr);
> +	if (ret)
> +		goto out;
>  
>  	if (all_user)
>  		rec_argv[i++] = "--all-user";
> @@ -164,6 +162,13 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>  	}
>  
>  	ret = cmd_record(i, rec_argv);
> +out:
> +	for (i = 0; i < tmp_nr; i++) {
> +		if (rec_tmp[i])
> +			free(rec_tmp[i]);

no need to check rec_tmp[i] != NULL, free will do that

> +	}
> +
> +	free(rec_tmp);
>  	free(rec_argv);
>  	return ret;
>  }
> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index e8f6e745eaf0..909ee91b75f0 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -173,6 +173,71 @@ void perf_mem_events__list(void)
>  	}
>  }
>  
> +static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
> +						    int idx)
> +{
> +	const char *mnt = sysfs__mount();
> +	char sysfs_name[100];
> +	struct perf_pmu *pmu;
> +
> +	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)) {
> +			pr_err("failed: event '%s' not supported\n",
> +			       perf_mem_events__name(idx, pmu->name));
> +		}
> +	}
> +}
> +
> +int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
> +				 char **rec_tmp, int *tmp_nr)
> +{
> +	int i = *argv_nr, k = 0;
> +	struct perf_mem_event *e;
> +	struct perf_pmu *pmu;
> +	char *s;
> +
> +	for (int j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
> +		e = perf_mem_events__ptr(j);
> +		if (!e->record)
> +			continue;
> +
> +		if (!perf_pmu__has_hybrid()) {
> +			if (!e->supported) {
> +				pr_err("failed: event '%s' not supported\n",
> +				       perf_mem_events__name(j, NULL));
> +				return -1;
> +			}
> +
> +			rec_argv[i++] = "-e";
> +			rec_argv[i++] = perf_mem_events__name(j, NULL);
> +		} else {
> +			if (!e->supported) {
> +				perf_mem_events__print_unsupport_hybrid(e, j);
> +				return -1;
> +			}
> +
> +			perf_pmu__for_each_hybrid_pmu(pmu) {
> +				rec_argv[i++] = "-e";
> +				s = perf_mem_events__name(j, pmu->name);
> +				if (s) {
> +					s = strdup(s);
> +					if (!s)
> +						return -1;
> +
> +					rec_argv[i++] = s;
> +					rec_tmp[k++] = s;
> +				}
> +			}
> +		}
> +	}
> +
> +	*argv_nr = i;
> +	*tmp_nr = k;
> +	return 0;
> +}
> +
>  static const char * const tlb_access[] = {
>  	"N/A",
>  	"HIT",
> diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
> index a3fa19093fd2..916242f8020a 100644
> --- a/tools/perf/util/mem-events.h
> +++ b/tools/perf/util/mem-events.h
> @@ -43,6 +43,8 @@ struct perf_mem_event *perf_mem_events__ptr(int i);
>  bool is_mem_loads_aux_event(struct evsel *leader);
>  
>  void perf_mem_events__list(void);
> +int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
> +				 char **rec_tmp, int *tmp_nr);
>  
>  int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
>  int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
> -- 
> 2.17.1
> 

Re: [PATCH v1 4/5] perf mem: Support record for hybrid platform

[Jin, Yao]

Hi Jiri,

On 5/25/2021 1:19 AM, Jiri Olsa wrote:
> On Thu, May 20, 2021 at 03:00:39PM +0800, Jin Yao wrote:
>> Support 'perf mem record' for hybrid platform. On hybrid platform,
>> such as Alderlake, when executing 'perf mem record', it actually calls:
>>
>> record -e {cpu_core/mem-loads-aux/,cpu_core/mem-loads,ldlat=30/}:P
>>         -e cpu_atom/mem-loads,ldlat=30/P
>>         -e cpu_core/mem-stores/P
>>         -e cpu_atom/mem-stores/P
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-mem.c     | 39 ++++++++++++----------
>>   tools/perf/util/mem-events.c | 65 ++++++++++++++++++++++++++++++++++++
>>   tools/perf/util/mem-events.h |  2 ++
>>   3 files changed, 89 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/builtin-mem.c b/tools/perf/builtin-mem.c
>> index 03795bf49d51..a50abcb45f0f 100644
>> --- a/tools/perf/builtin-mem.c
>> +++ b/tools/perf/builtin-mem.c
>> @@ -62,8 +62,9 @@ static const char * const *record_mem_usage = __usage;
>>   
>>   static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   {
>> -	int rec_argc, i = 0, j;
>> +	int rec_argc, i = 0, j,  tmp_nr = 0;
>>   	const char **rec_argv;
>> +	char **rec_tmp;
>>   	int ret;
>>   	bool all_user = false, all_kernel = false;
>>   	struct perf_mem_event *e;
>> @@ -87,11 +88,20 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   	argc = parse_options(argc, argv, options, record_mem_usage,
>>   			     PARSE_OPT_KEEP_UNKNOWN);
>>   
>> -	rec_argc = argc + 9; /* max number of arguments */
>> +	rec_argc = argc + 64; /* max number of arguments */
> 
> please add comment on how you got the number 64
> 

original max number of arguments is 9, *2 (=18) for hybrid pmus. 18 should be OK. I use 64 is to 
allocate a big enough value.

> 
>>   	rec_argv = calloc(rec_argc + 1, sizeof(char *));
>>   	if (!rec_argv)
>>   		return -1;
>>   
>> +	/*
>> +	 * Save the allocated event name strings.
>> +	 */
>> +	rec_tmp = calloc(rec_argc + 1, sizeof(char *));
>> +	if (!rec_tmp) {
>> +		free(rec_argv);
>> +		return -1;
>> +	}
> 
> why not do strdup on all of them and always call free instead?
> that would get rid of the rec_tmp and tmp_nr
> 

That is also one method. Let me try it.

>> +
>>   	rec_argv[i++] = "record";
>>   
>>   	e = perf_mem_events__ptr(PERF_MEM_EVENTS__LOAD_STORE);
>> @@ -128,21 +138,9 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   	if (mem->data_page_size)
>>   		rec_argv[i++] = "--data-page-size";
>>   
>> -	for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
>> -		e = perf_mem_events__ptr(j);
>> -		if (!e->record)
>> -			continue;
>> -
>> -		if (!e->supported) {
>> -			pr_err("failed: event '%s' not supported\n",
>> -			       perf_mem_events__name(j, NULL));
>> -			free(rec_argv);
>> -			return -1;
>> -		}
>> -
>> -		rec_argv[i++] = "-e";
>> -		rec_argv[i++] = perf_mem_events__name(j, NULL);
>> -	}
>> +	ret = perf_mem_events__record_args(rec_argv, &i, rec_tmp, &tmp_nr);
>> +	if (ret)
>> +		goto out;
>>   
>>   	if (all_user)
>>   		rec_argv[i++] = "--all-user";
>> @@ -164,6 +162,13 @@ static int __cmd_record(int argc, const char **argv, struct perf_mem *mem)
>>   	}
>>   
>>   	ret = cmd_record(i, rec_argv);
>> +out:
>> +	for (i = 0; i < tmp_nr; i++) {
>> +		if (rec_tmp[i])
>> +			free(rec_tmp[i]);
> 
> no need to check rec_tmp[i] != NULL, free will do that
>

Yes, no need checking for NULL here.

Thanks
Jin Yao

>> +	}
>> +
>> +	free(rec_tmp);
>>   	free(rec_argv);
>>   	return ret;
>>   }
>> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
>> index e8f6e745eaf0..909ee91b75f0 100644
>> --- a/tools/perf/util/mem-events.c
>> +++ b/tools/perf/util/mem-events.c
>> @@ -173,6 +173,71 @@ void perf_mem_events__list(void)
>>   	}
>>   }
>>   
>> +static void perf_mem_events__print_unsupport_hybrid(struct perf_mem_event *e,
>> +						    int idx)
>> +{
>> +	const char *mnt = sysfs__mount();
>> +	char sysfs_name[100];
>> +	struct perf_pmu *pmu;
>> +
>> +	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)) {
>> +			pr_err("failed: event '%s' not supported\n",
>> +			       perf_mem_events__name(idx, pmu->name));
>> +		}
>> +	}
>> +}
>> +
>> +int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
>> +				 char **rec_tmp, int *tmp_nr)
>> +{
>> +	int i = *argv_nr, k = 0;
>> +	struct perf_mem_event *e;
>> +	struct perf_pmu *pmu;
>> +	char *s;
>> +
>> +	for (int j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
>> +		e = perf_mem_events__ptr(j);
>> +		if (!e->record)
>> +			continue;
>> +
>> +		if (!perf_pmu__has_hybrid()) {
>> +			if (!e->supported) {
>> +				pr_err("failed: event '%s' not supported\n",
>> +				       perf_mem_events__name(j, NULL));
>> +				return -1;
>> +			}
>> +
>> +			rec_argv[i++] = "-e";
>> +			rec_argv[i++] = perf_mem_events__name(j, NULL);
>> +		} else {
>> +			if (!e->supported) {
>> +				perf_mem_events__print_unsupport_hybrid(e, j);
>> +				return -1;
>> +			}
>> +
>> +			perf_pmu__for_each_hybrid_pmu(pmu) {
>> +				rec_argv[i++] = "-e";
>> +				s = perf_mem_events__name(j, pmu->name);
>> +				if (s) {
>> +					s = strdup(s);
>> +					if (!s)
>> +						return -1;
>> +
>> +					rec_argv[i++] = s;
>> +					rec_tmp[k++] = s;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	*argv_nr = i;
>> +	*tmp_nr = k;
>> +	return 0;
>> +}
>> +
>>   static const char * const tlb_access[] = {
>>   	"N/A",
>>   	"HIT",
>> diff --git a/tools/perf/util/mem-events.h b/tools/perf/util/mem-events.h
>> index a3fa19093fd2..916242f8020a 100644
>> --- a/tools/perf/util/mem-events.h
>> +++ b/tools/perf/util/mem-events.h
>> @@ -43,6 +43,8 @@ struct perf_mem_event *perf_mem_events__ptr(int i);
>>   bool is_mem_loads_aux_event(struct evsel *leader);
>>   
>>   void perf_mem_events__list(void);
>> +int perf_mem_events__record_args(const char **rec_argv, int *argv_nr,
>> +				 char **rec_tmp, int *tmp_nr);
>>   
>>   int perf_mem__tlb_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
>>   int perf_mem__lvl_scnprintf(char *out, size_t sz, struct mem_info *mem_info);
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH v1 4/5] perf mem: Support record for hybrid platform

[Jin, Yao]

Hi Jiri,

>>>       rec_argv = calloc(rec_argc + 1, sizeof(char *));
>>>       if (!rec_argv)
>>>           return -1;
>>> +    /*
>>> +     * Save the allocated event name strings.
>>> +     */
>>> +    rec_tmp = calloc(rec_argc + 1, sizeof(char *));
>>> +    if (!rec_tmp) {
>>> +        free(rec_argv);
>>> +        return -1;
>>> +    }
>>
>> why not do strdup on all of them and always call free instead?
>> that would get rid of the rec_tmp and tmp_nr
>>
> 
> That is also one method. Let me try it.
> 

If we do strdup on all of them, such as,

	if (e->record)
		rec_argv[i++] = strdup("-W");

	rec_argv[i++] = strdup("-d");

	if (mem->phys_addr)
		rec_argv[i++] = strdup("--phys-data");
	....

That looks too much strdup used here. So I choose to use a rec_tmp[] to record the allocated string 
and free them before exit the function.

Or we record the start index and end index in rec_argv[] for the allocated event string, use strdup 
on them and call free before exit the function.

What do you think?

Thanks
Jin Yao

Re: [PATCH v1 4/5] perf mem: Support record for hybrid platform

[Jin, Yao]

Hi Jiri,

On 5/25/2021 3:39 PM, Jin, Yao wrote:
> Hi Jiri,
> 
>>>>       rec_argv = calloc(rec_argc + 1, sizeof(char *));
>>>>       if (!rec_argv)
>>>>           return -1;
>>>> +    /*
>>>> +     * Save the allocated event name strings.
>>>> +     */
>>>> +    rec_tmp = calloc(rec_argc + 1, sizeof(char *));
>>>> +    if (!rec_tmp) {
>>>> +        free(rec_argv);
>>>> +        return -1;
>>>> +    }
>>>
>>> why not do strdup on all of them and always call free instead?
>>> that would get rid of the rec_tmp and tmp_nr
>>>
>>
>> That is also one method. Let me try it.
>>
> 
> If we do strdup on all of them, such as,
> 
>      if (e->record)
>          rec_argv[i++] = strdup("-W");
> 
>      rec_argv[i++] = strdup("-d");
> 
>      if (mem->phys_addr)
>          rec_argv[i++] = strdup("--phys-data");
>      ....
> 
> That looks too much strdup used here. So I choose to use a rec_tmp[] to record the allocated string 
> and free them before exit the function.
> 
> Or we record the start index and end index in rec_argv[] for the allocated event string, use strdup 
> on them and call free before exit the function.
> 

This method looks also not OK.

The rec_argv[] is changed in cmd_record() for some complex command lines.

For example,

./perf mem record -- ./memtest -R0d -b2000 -d64 -n100

Before cmd_record(), rec_argv[3] = "-e".
After cmd_record(), rec_argv[3] = "-d64"

Even we do strdup on all of rec_argv[], but the entries are probably changed in cmd_record(), so we 
can't free the entries at the end of __cmd_record().

Maybe we have to use the original way which just records the allocated event string to a temporary 
array and free the whole array at the end of __cmd_record().

What do you think?

Thanks
Jin Yao

> What do you think?
> 
> Thanks
> Jin Yao

Re: [PATCH v1 4/5] perf mem: Support record for hybrid platform

[Jiri Olsa]

On Wed, May 26, 2021 at 09:51:34AM +0800, Jin, Yao wrote:
> Hi Jiri,
> 
> On 5/25/2021 3:39 PM, Jin, Yao wrote:
> > Hi Jiri,
> > 
> > > > >       rec_argv = calloc(rec_argc + 1, sizeof(char *));
> > > > >       if (!rec_argv)
> > > > >           return -1;
> > > > > +    /*
> > > > > +     * Save the allocated event name strings.
> > > > > +     */
> > > > > +    rec_tmp = calloc(rec_argc + 1, sizeof(char *));
> > > > > +    if (!rec_tmp) {
> > > > > +        free(rec_argv);
> > > > > +        return -1;
> > > > > +    }
> > > > 
> > > > why not do strdup on all of them and always call free instead?
> > > > that would get rid of the rec_tmp and tmp_nr
> > > > 
> > > 
> > > That is also one method. Let me try it.
> > > 
> > 
> > If we do strdup on all of them, such as,
> > 
> >      if (e->record)
> >          rec_argv[i++] = strdup("-W");
> > 
> >      rec_argv[i++] = strdup("-d");
> > 
> >      if (mem->phys_addr)
> >          rec_argv[i++] = strdup("--phys-data");
> >      ....
> > 
> > That looks too much strdup used here. So I choose to use a rec_tmp[] to
> > record the allocated string and free them before exit the function.
> > 
> > Or we record the start index and end index in rec_argv[] for the
> > allocated event string, use strdup on them and call free before exit the
> > function.
> > 
> 
> This method looks also not OK.
> 
> The rec_argv[] is changed in cmd_record() for some complex command lines.
> 
> For example,
> 
> ./perf mem record -- ./memtest -R0d -b2000 -d64 -n100
> 
> Before cmd_record(), rec_argv[3] = "-e".
> After cmd_record(), rec_argv[3] = "-d64"
> 
> Even we do strdup on all of rec_argv[], but the entries are probably changed
> in cmd_record(), so we can't free the entries at the end of __cmd_record().
> 
> Maybe we have to use the original way which just records the allocated event
> string to a temporary array and free the whole array at the end of
> __cmd_record().
> 
> What do you think?

ok, it was worth to try ;-)

thanks,
jirka

[PATCH v1 5/5] perf c2c: Support record for hybrid platform

[Jin Yao]

Support 'perf c2c record' for hybrid platform. On hybrid platform,
such as Alderlake, when executing 'perf c2c record', it actually calls:

record -W -d --phys-data --sample-cpu
-e {cpu_core/mem-loads-aux/,cpu_core/mem-loads,ldlat=30/}:P
-e cpu_atom/mem-loads,ldlat=30/P
-e cpu_core/mem-stores/P
-e cpu_atom/mem-stores/P

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-c2c.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index a4fd375acdd1..70804ad9017a 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2907,8 +2907,9 @@ static const char * const *record_mem_usage = __usage_record;
 
 static int perf_c2c__record(int argc, const char **argv)
 {
-	int rec_argc, i = 0, j;
+	int rec_argc, i = 0, j, rec_nr = 0;
 	const char **rec_argv;
+	char **rec_tmp;
 	int ret;
 	bool all_user = false, all_kernel = false;
 	bool event_set = false;
@@ -2932,11 +2933,17 @@ static int perf_c2c__record(int argc, const char **argv)
 	argc = parse_options(argc, argv, options, record_mem_usage,
 			     PARSE_OPT_KEEP_UNKNOWN);
 
-	rec_argc = argc + 11; /* max number of arguments */
+	rec_argc = argc + 64; /* max number of arguments */
 	rec_argv = calloc(rec_argc + 1, sizeof(char *));
 	if (!rec_argv)
 		return -1;
 
+	rec_tmp = calloc(rec_argc + 1, sizeof(char *));
+	if (!rec_tmp) {
+		free(rec_argv);
+		return -1;
+	}
+
 	rec_argv[i++] = "record";
 
 	if (!event_set) {
@@ -2964,21 +2971,9 @@ static int perf_c2c__record(int argc, const char **argv)
 	rec_argv[i++] = "--phys-data";
 	rec_argv[i++] = "--sample-cpu";
 
-	for (j = 0; j < PERF_MEM_EVENTS__MAX; j++) {
-		e = perf_mem_events__ptr(j);
-		if (!e->record)
-			continue;
-
-		if (!e->supported) {
-			pr_err("failed: event '%s' not supported\n",
-			       perf_mem_events__name(j, NULL));
-			free(rec_argv);
-			return -1;
-		}
-
-		rec_argv[i++] = "-e";
-		rec_argv[i++] = perf_mem_events__name(j, NULL);
-	}
+	ret = perf_mem_events__record_args(rec_argv, &i, rec_tmp, &rec_nr);
+	if (ret)
+		goto out;
 
 	if (all_user)
 		rec_argv[i++] = "--all-user";
@@ -3002,6 +2997,13 @@ static int perf_c2c__record(int argc, const char **argv)
 	}
 
 	ret = cmd_record(i, rec_argv);
+out:
+	for (i = 0; i < rec_nr; i++) {
+		if (rec_tmp[i])
+			free(rec_tmp[i]);
+	}
+
+	free(rec_tmp);
 	free(rec_argv);
 	return ret;
 }
-- 
2.17.1