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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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
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 <[email protected]>
>> ---
>> 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
>>
>
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 <[email protected]>
>> ---
>> 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
>>
>
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
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
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