Skip to contentSkip to conversation

perf vendor events: Add stepping in CPUID string for x86

PATCH In review [email protected] 4 participants 20 messages Nov 14, 2018 – Nov 22, 2018 2 messages not in the archive perf events

lore
Share & tools
lore
#0 lore raw

PATCH 1/2 perf vendor events: Add stepping in CPUID string for x86

From: Kan Liang <[email protected]>

Perf tools cannot find the proper event list for Cascadelake server. Because Cascadelake server and Skylake server have the same CPU model number, which are used by perf tools to find the event list.

The stepping for Skylake server is up to 4. The stepping for Cascadelake server starts from 5. The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().
The stepping information for Skylake server is updated in mapfile.csv. A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model".
- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.
- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

The script, which using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

---
 tools/perf/arch/x86/util/header.c          | 67 +++++++++++++++++++++-
 tools/perf/pmu-events/arch/x86/mapfile.csv |  2 +-
 tools/perf/util/pmu.c                      |  2 +-
 3 files changed, 68 insertions(+), 3 deletions(-)
tools/perf/arch/x86/util/header.c +66 -1
index fb0d71afee8b..b428a4b00bf7 100644
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <regex.h>
 
 #include "../../util/header.h"
 
@@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 {
 	char *buf = malloc(128);
 
-	if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
+	if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
 		free(buf);
 		return NULL;
 	}
 	return buf;
 }
+
+/* Full CPUID format for x86 is vendor-family-model-stepping */
+static bool is_full_cpuid(const char *cpuid)
+{
+	const char *tmp = cpuid;
+	int count = 0;
+
+	while ((tmp = strchr(tmp, '-')) != NULL) {
+		count++;
+		tmp++;
+	}
+
+	if (count == 3)
+		return true;
+
+	return false;
+}
+
+int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
+{
+	regex_t re;
+	regmatch_t pmatch[1];
+	int match;
+	bool full_mapcpuid = is_full_cpuid(mapcpuid);
+	bool full_cpuid = is_full_cpuid(cpuid);
+
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", cpuid);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, cpuid, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(cpuid, '-') - cpuid;
+		else
+			cpuid_len = strlen(cpuid);
+
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;
+	}
+
+	return 1;
+}
tools/perf/pmu-events/arch/x86/mapfile.csv +1 -1
index 7e3cce3bcf3b..183a42c99251 100644
@@ -31,4 +31,4 @@ GenuineIntel-6-2A,v15,sandybridge,core
 GenuineIntel-6-2C,v2,westmereep-dp,core
 GenuineIntel-6-25,v2,westmereep-sp,core
 GenuineIntel-6-2F,v2,westmereex,core
-GenuineIntel-6-55,v1,skylakex,core
+GenuineIntel-6-55-[01234],v1,skylakex,core
tools/perf/util/pmu.c +1 -1
index afd68524ffa9..0996260178f7 100644
@@ -670,7 +670,7 @@ char * __weak get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
  * cpuid string generated on this platform.
  * Otherwise return non-zero.
  */
-int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
+int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
 {
 	regex_t re;
 	regmatch_t pmatch[1];
Signature · 1 line
2.17.1

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

Re: [PATCH 2/2] perf vendor events: Add JSON metrics for Cascadelake server

Quoted 3 lines · On Wed, Nov 14, 2018 at 01:24:16PM -0800, [email protected] wrote:

From: Kan Liang <[email protected]>

Add JSON metrics (based on event list v1) for Cascadelake server

hum mail server probably crippled the email so it won't apply:

patching file pmu-events/arch/x86/cascadelakex/cache.json
patching file pmu-events/arch/x86/cascadelakex/clx-metrics.json
patching file pmu-events/arch/x86/cascadelakex/floating-point.json
patching file pmu-events/arch/x86/cascadelakex/frontend.json
patching file pmu-events/arch/x86/cascadelakex/memory.json
patching file pmu-events/arch/x86/cascadelakex/other.json
patching file pmu-events/arch/x86/cascadelakex/pipeline.json
patch: **** malformed patch at line 29879: ow status bit and resets the counter to less than MAX. The reset value to the counter is not clocked immediately so the overflow status bit will flip 'high (1)' and generate another PMI (if enabled) after which the reset value gets clocked into the counter. Therefore, software will get the interrupt, read the overflow status bit '1 for bit 34 while the counter value is less than MAX. Software should ignore this case.",

I've similar error before and it was because of the 998 chars size limit for line.. I guess you could either post it as attachment or change the transfer encoding

jirka

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw

Re: [PATCH 2/2] perf vendor events: Add JSON metrics for Cascadelake server

Quoted 19 lines · Kan
Quoted 16 lines · Jiri Olsa
Quoted 3 lines · On Wed, Nov 14, 2018 at 01:24:16PM -0800, [email protected] wrote:

From: Kan Liang <[email protected]>

Add JSON metrics (based on event list v1) for Cascadelake server

hum mail server probably crippled the email so it won't apply:

patching file pmu-events/arch/x86/cascadelakex/cache.json
patching file pmu-events/arch/x86/cascadelakex/clx-metrics.json
patching file pmu-events/arch/x86/cascadelakex/floating-point.json
patching file pmu-events/arch/x86/cascadelakex/frontend.json
patching file pmu-events/arch/x86/cascadelakex/memory.json
patching file pmu-events/arch/x86/cascadelakex/other.json
patching file pmu-events/arch/x86/cascadelakex/pipeline.json
patch: **** malformed patch at line 29879: ow status bit and resets the counter to less than MAX. The reset value to the counter is not clocked immediately so the overflow status bit will flip 'high (1)' and generate another PMI (if enabled) after which the reset value gets clocked into the counter. Therefore, software will get the interrupt, read the overflow status bit '1 for bit 34 while the counter value is less than MAX. Software should ignore this case.",

Quoted 6 lines
Quoted 4 lines

I've similar error before and it was because of the 998 chars size limit for line.. I guess you could either post it as attachment or change the transfer encoding

The patch is attached. Can it be applied?

yep, that one works

thanks,
jirka

#5 lore raw

Re: [PATCH 2/2] perf vendor events: Add JSON metrics for Cascadelake server

Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:

Quoted 20 lines · Kan
Quoted 19 lines · Jiri Olsa
Quoted 16 lines · On Wed, Nov 14, 2018 at 01:24:16PM -0800, [email protected] wrote:
Quoted 3 lines

From: Kan Liang <[email protected]>

Add JSON metrics (based on event list v1) for Cascadelake server

hum mail server probably crippled the email so it won't apply:

patching file pmu-events/arch/x86/cascadelakex/cache.json
patching file pmu-events/arch/x86/cascadelakex/clx-metrics.json
patching file pmu-events/arch/x86/cascadelakex/floating-point.json
patching file pmu-events/arch/x86/cascadelakex/frontend.json
patching file pmu-events/arch/x86/cascadelakex/memory.json
patching file pmu-events/arch/x86/cascadelakex/other.json
patching file pmu-events/arch/x86/cascadelakex/pipeline.json
patch: **** malformed patch at line 29879: ow status bit and resets the counter to less than MAX. The reset value to the counter is not clocked immediately so the overflow status bit will flip 'high (1)' and generate another PMI (if enabled) after which the reset value gets clocked into the counter. Therefore, software will get the interrupt, read the overflow status bit '1 for bit 34 while the counter value is less than MAX. Software should ignore this case.",

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

I've similar error before and it was because of the 998 chars size limit for line.. I guess you could either post it as attachment or change the transfer encoding

The patch is attached. Can it be applied?

yep, that one works

Can I have your reviewed-by for this one as well?

- Arnaldo

#6 lore raw

Re: [PATCH 2/2] perf vendor events: Add JSON metrics for Cascadelake server

Em Wed, Nov 21, 2018 at 09:51:30AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 26 lines

Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:

Quoted 25 lines · Kan
Quoted 24 lines · Jiri Olsa
Quoted 20 lines · On Wed, Nov 14, 2018 at 01:24:16PM -0800, [email protected] wrote:
Quoted 3 lines

From: Kan Liang <[email protected]>

Add JSON metrics (based on event list v1) for Cascadelake server

hum mail server probably crippled the email so it won't apply:

patching file pmu-events/arch/x86/cascadelakex/cache.json
patching file pmu-events/arch/x86/cascadelakex/clx-metrics.json
patching file pmu-events/arch/x86/cascadelakex/floating-point.json
patching file pmu-events/arch/x86/cascadelakex/frontend.json
patching file pmu-events/arch/x86/cascadelakex/memory.json
patching file pmu-events/arch/x86/cascadelakex/other.json
patching file pmu-events/arch/x86/cascadelakex/pipeline.json
patch: **** malformed patch at line 29879: ow status bit and resets the counter to less than MAX. The reset value to the counter is not clocked immediately so the overflow status bit will flip 'high (1)' and generate another PMI (if enabled) after which the reset value gets clocked into the counter. Therefore, software will get the interrupt, read the overflow status bit '1 for bit 34 while the counter value is less than MAX. Software should ignore this case.",

I've similar error before and it was because of the 998 chars size limit for line.. I guess you could either post it as attachment or change the transfer encoding

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

The patch is attached. Can it be applied?

yep, that one works

Can I have your reviewed-by for this one as well?

I turned the "yep, that one works" into an Acked-by, please let me know if that is enough.

- Arnaldo

#7 lore raw

Re: [PATCH 2/2] perf vendor events: Add JSON metrics for Cascadelake server

Quoted 27 lines · Arnaldo Carvalho de Melo

Em Wed, Nov 21, 2018 at 09:51:30AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 26 lines

Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:

Quoted 25 lines · Kan
Quoted 24 lines · Jiri Olsa
Quoted 20 lines · On Wed, Nov 14, 2018 at 01:24:16PM -0800, [email protected] wrote:
Quoted 3 lines

From: Kan Liang <[email protected]>

Add JSON metrics (based on event list v1) for Cascadelake server

hum mail server probably crippled the email so it won't apply:

patching file pmu-events/arch/x86/cascadelakex/cache.json
patching file pmu-events/arch/x86/cascadelakex/clx-metrics.json
patching file pmu-events/arch/x86/cascadelakex/floating-point.json
patching file pmu-events/arch/x86/cascadelakex/frontend.json
patching file pmu-events/arch/x86/cascadelakex/memory.json
patching file pmu-events/arch/x86/cascadelakex/other.json
patching file pmu-events/arch/x86/cascadelakex/pipeline.json
patch: **** malformed patch at line 29879: ow status bit and resets the counter to less than MAX. The reset value to the counter is not clocked immediately so the overflow status bit will flip 'high (1)' and generate another PMI (if enabled) after which the reset value gets clocked into the counter. Therefore, software will get the interrupt, read the overflow status bit '1 for bit 34 while the counter value is less than MAX. Software should ignore this case.",

I've similar error before and it was because of the 998 chars size limit for line.. I guess you could either post it as attachment or change the transfer encoding

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

The patch is attached. Can it be applied?

yep, that one works

Can I have your reviewed-by for this one as well?

I turned the "yep, that one works" into an Acked-by, please let me know if that is enough.

yes, if it applies it's ok for me.. but can't judge the events inside ;-)

jirka

#8 lore raw

Re: [PATCH 2/2] perf vendor events: Add JSON metrics for Cascadelake server

Em Wed, Nov 21, 2018 at 02:02:02PM +0100, Jiri Olsa escreveu:

Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 4 lines

Em Wed, Nov 21, 2018 at 09:51:30AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 3 lines

Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:

Quoted 2 lines · Kan
Quoted 1 line

The patch is attached. Can it be applied?

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line

yep, that one works

Can I have your reviewed-by for this one as well?

I turned the "yep, that one works" into an Acked-by, please let me know if that is enough.

yes, if it applies it's ok for me.. but can't judge the events inside ;-)

Ok, Acked-by kept then. Thanks!

- Arnaldo

#9 lore raw
In reply to [email protected]

On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:

SNIP

Quoted 38 lines
tools/perf/arch/x86/util/header.c +17 -1
index fb0d71afee8b..b428a4b00bf7 100644
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <regex.h>
 
 #include "../../util/header.h"
 
@@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 {
 	char *buf = malloc(128);
 
-	if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
+	if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
 		free(buf);
 		return NULL;
 	}
 	return buf;
 }
+
+/* Full CPUID format for x86 is vendor-family-model-stepping */
+static bool is_full_cpuid(const char *cpuid)
+{
+	const char *tmp = cpuid;
+	int count = 0;
+
+	while ((tmp = strchr(tmp, '-')) != NULL) {
+		count++;
+		tmp++;
+	}
+
+	if (count == 3)
+		return true;
+

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 104

Quoted 10 lines
tools/perf/arch/x86/util/header.c +10 -0
+	return false;
+}
+
+int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
+{
+	regex_t re;
+	regmatch_t pmatch[1];
+	int match;
+	bool full_mapcpuid = is_full_cpuid(mapcpuid);
+	bool full_cpuid = is_full_cpuid(cpuid);

cpuid will be always full from now right? why do we need to check it?

also please move this to arch/x86/util/pmu.c
so it matches the weak function object

Quoted 25 lines
tools/perf/arch/x86/util/header.c +25 -0
+
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", cpuid);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, cpuid, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 139

Quoted 10 lines
tools/perf/arch/x86/util/header.c +10 -0
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(cpuid, '-') - cpuid;
+		else
+			cpuid_len = strlen(cpuid);
+
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;

why is this necessary?

thanks,
jirka

author
#10 lore raw
Quoted 46 lines · Jiri Olsa

On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:

SNIP

Quoted 42 lines
tools/perf/arch/x86/util/header.c +21 -1
index fb0d71afee8b..b428a4b00bf7 100644
@@ -4,6 +4,7 @@
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
+#include <regex.h>
 
  #include "../../util/header.h"
 
@@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
  {
  	char *buf = malloc(128);
 
-	if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
+	if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
  		free(buf);
  		return NULL;
  	}
  	return buf;
  }
+
+/* Full CPUID format for x86 is vendor-family-model-stepping */
+static bool is_full_cpuid(const char *cpuid)
+{
+	const char *tmp = cpuid;
+	int count = 0;
+
+	while ((tmp = strchr(tmp, '-')) != NULL) {
+		count++;
+		tmp++;
+	}
+
+	if (count == 3)
+		return true;
+
+	return false;
+}
+
+int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 104

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+{
+	regex_t re;
+	regmatch_t pmatch[1];
+	int match;
+	bool full_mapcpuid = is_full_cpuid(mapcpuid);
+	bool full_cpuid = is_full_cpuid(cpuid);

cpuid will be always full from now right? why do we need to check it?

User may set cpuid by environment string "PERF_CPUID", which may not be full format.

Quoted 2 lines

also please move this to arch/x86/util/pmu.c
so it matches the weak function object

Sure.

Quoted 30 lines
Quoted 30 lines
tools/perf/arch/x86/util/header.c +29 -0
+
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", cpuid);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, cpuid, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(cpuid, '-') - cpuid;
+		else
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 139

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+			cpuid_len = strlen(cpuid);
+
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;

why is this necessary?

It's from previous common code. As my understanding, it just double check the matched strings. There is no harmful. So I keep it.

Thanks,
Kan

author
#11 lore raw
Quoted 56 lines · Kan
Quoted 43 lines · Jiri Olsa

On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:

SNIP

Quoted 39 lines
tools/perf/arch/x86/util/header.c +21 -1
index fb0d71afee8b..b428a4b00bf7 100644
@@ -4,6 +4,7 @@
   #include <stdio.h>
   #include <stdlib.h>
   #include <string.h>
+#include <regex.h>
   #include "../../util/header.h"
@@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
   {
       char *buf = malloc(128);
-    if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
+    if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
           free(buf);
           return NULL;
       }
       return buf;
   }
+
+/* Full CPUID format for x86 is vendor-family-model-stepping */
+static bool is_full_cpuid(const char *cpuid)
+{
+    const char *tmp = cpuid;
+    int count = 0;
+
+    while ((tmp = strchr(tmp, '-')) != NULL) {
+        count++;
+        tmp++;
+    }
+
+    if (count == 3)
+        return true;
+
+    return false;
+}
+
+int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 104

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+{
+    regex_t re;
+    regmatch_t pmatch[1];
+    int match;
+    bool full_mapcpuid = is_full_cpuid(mapcpuid);
+    bool full_cpuid = is_full_cpuid(cpuid);

cpuid will be always full from now right? why do we need to check it?

Quoted 7 lines

User may set cpuid by environment string "PERF_CPUID", which may not be full format.

Quoted 2 lines

also please move this to arch/x86/util/pmu.c
so it matches the weak function object

Sure.

It looks like both pmu.c and header.c have cpuid related functions.
For example,
get_cpuid_str() has weak function in pmu.c. It's declaration in header.h. It's x86 specific function in x86/util/header.c get_cpuid() has weak function in header.c. It's declaration in header.h. It's x86 specific function in x86/util/header.c

It looks like most of the cpuid related functions are in header.c/h. I think it may be better to move all the cpuid related functions to header.c/h.

If it's OK for you, I will send a clean up patch later to move the weak functions strcmp_cpuid_str() and get_cpuid_str() to header.c

What do you think?

Thanks,
Kan

#12 lore raw

On Thu, Nov 15, 2018 at 10:18:27AM -0500, Liang, Kan wrote:

SNIP

Quoted 27 lines
Quoted 11 lines
Quoted 10 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 104

Quoted 7 lines
tools/perf/arch/x86/util/header.c +7 -0
+int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
+{
+    regex_t re;
+    regmatch_t pmatch[1];
+    int match;
+    bool full_mapcpuid = is_full_cpuid(mapcpuid);
+    bool full_cpuid = is_full_cpuid(cpuid);

cpuid will be always full from now right? why do we need to check it?

Quoted 8 lines

User may set cpuid by environment string "PERF_CPUID", which may not be full format.

Quoted 2 lines

also please move this to arch/x86/util/pmu.c
so it matches the weak function object

Sure.

It looks like both pmu.c and header.c have cpuid related functions. For example,
get_cpuid_str() has weak function in pmu.c. It's declaration in header.h. It's x86 specific function in x86/util/header.c
get_cpuid() has weak function in header.c. It's declaration in header.h. It's x86 specific function in x86/util/header.c

Quoted 8 lines

It looks like most of the cpuid related functions are in header.c/h. I think it may be better to move all the cpuid related functions to header.c/h.

If it's OK for you, I will send a clean up patch later to move the weak functions strcmp_cpuid_str() and get_cpuid_str() to header.c

What do you think?

sounds good, thanks

jirka

#13 lore raw
In reply to Liang, Kan
Quoted 56 lines · Kan
Quoted 43 lines · Jiri Olsa

On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:

SNIP

Quoted 39 lines
tools/perf/arch/x86/util/header.c +21 -1
index fb0d71afee8b..b428a4b00bf7 100644
@@ -4,6 +4,7 @@
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
+#include <regex.h>
  #include "../../util/header.h"
@@ -70,9 +71,73 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
  {
  	char *buf = malloc(128);
-	if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
+	if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
  		free(buf);
  		return NULL;
  	}
  	return buf;
  }
+
+/* Full CPUID format for x86 is vendor-family-model-stepping */
+static bool is_full_cpuid(const char *cpuid)
+{
+	const char *tmp = cpuid;
+	int count = 0;
+
+	while ((tmp = strchr(tmp, '-')) != NULL) {
+		count++;
+		tmp++;
+	}
+
+	if (count == 3)
+		return true;
+
+	return false;
+}
+
+int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 104

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+{
+	regex_t re;
+	regmatch_t pmatch[1];
+	int match;
+	bool full_mapcpuid = is_full_cpuid(mapcpuid);
+	bool full_cpuid = is_full_cpuid(cpuid);

cpuid will be always full from now right? why do we need to check it?

Quoted 2 lines

User may set cpuid by environment string "PERF_CPUID", which may not be full format.

ok, forgot about this one

Quoted 46 lines
Quoted 3 lines

also please move this to arch/x86/util/pmu.c
so it matches the weak function object

Sure.

Quoted 30 lines
Quoted 30 lines
tools/perf/arch/x86/util/header.c +29 -0
+
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", cpuid);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, cpuid, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(cpuid, '-') - cpuid;
+		else
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 139

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+			cpuid_len = strlen(cpuid);
+
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;

why is this necessary?

Quoted 2 lines

It's from previous common code. As my understanding, it just double check the matched strings. There is no harmful. So I keep it.

right.. did you consider using the wildcard in the map file so it'd cover the stepping, having entries like:

GenuineIntel-6-1F-*,v2,nehalemep,core

I haven't thought this one through, but seems we could bypass those '-stepping' checks.. but probably other changes would be necessary for the wildcard

thanks,
jirka

author
#14 lore raw
Quoted 42 lines
Quoted 39 lines
Quoted 29 lines
Quoted 29 lines
tools/perf/arch/x86/util/header.c +28 -0
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", cpuid);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, cpuid, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(cpuid, '-') - cpuid;
+		else
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 139

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+			cpuid_len = strlen(cpuid);
+
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;

why is this necessary?

Quoted 2 lines

It's from previous common code. As my understanding, it just double check the matched strings. There is no harmful. So I keep it.

Quoted 9 lines

right.. did you consider using the wildcard in the map file so it'd cover the stepping, having entries like:

   GenuineIntel-6-1F-*,v2,nehalemep,core

I haven't thought this one through, but seems we could bypass those '-stepping' checks.. but probably other changes would be necessary for the wildcard

If using wildcard, the full cpuid format is required for all platforms.
The script which using environment string "PERF_CPUID" will definitely be broken on all platforms. This patch (non-wildcard) only breaks the script on Skylake server and Cascadelake server.

I think that's the only drawback of using wildcard.

Thanks,
Kan

#15 lore raw
Quoted 52 lines · Kan
Quoted 42 lines
Quoted 39 lines
Quoted 29 lines
Quoted 29 lines
tools/perf/arch/x86/util/header.c +28 -0
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", cpuid);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, cpuid, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(cpuid, '-') - cpuid;
+		else
Quoted 9 lines

tools/perf/arch/x86/util/header.c get_cpuid_str(struct perf_pmu *pmu __maybe_unused) · line 139

Quoted 6 lines
tools/perf/arch/x86/util/header.c +6 -0
+			cpuid_len = strlen(cpuid);
+
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;

why is this necessary?

Quoted 2 lines

It's from previous common code. As my understanding, it just double check the matched strings. There is no harmful. So I keep it.

Quoted 9 lines

right.. did you consider using the wildcard in the map file so it'd cover the stepping, having entries like:

   GenuineIntel-6-1F-*,v2,nehalemep,core

I haven't thought this one through, but seems we could bypass those '-stepping' checks.. but probably other changes would be necessary for the wildcard

Quoted 7 lines

If using wildcard, the full cpuid format is required for all platforms. The script which using environment string "PERF_CPUID" will definitely be broken on all platforms.
This patch (non-wildcard) only breaks the script on Skylake server and Cascadelake server.

I think that's the only drawback of using wildcard.

ok, let's keep your way

jirk

Reviewed-by on v1 1/2
#16 lore raw
In reply to [email protected]
Quoted 21 lines · On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:

From: Kan Liang <[email protected]>

Perf tools cannot find the proper event list for Cascadelake server. Because Cascadelake server and Skylake server have the same CPU model number, which are used by perf tools to find the event list.

The stepping for Skylake server is up to 4. The stepping for Cascadelake server starts from 5. The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().
The stepping information for Skylake server is updated in mapfile.csv. A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model".
- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.
- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

Quoted 4 lines

The script, which using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

thanks,
jirka

author
#17 lore raw
Quoted 22 lines · Jiri Olsa
Quoted 21 lines · On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:

From: Kan Liang <[email protected]>

Perf tools cannot find the proper event list for Cascadelake server. Because Cascadelake server and Skylake server have the same CPU model number, which are used by perf tools to find the event list.

The stepping for Skylake server is up to 4. The stepping for Cascadelake server starts from 5. The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().
The stepping information for Skylake server is updated in mapfile.csv. A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model".
- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.
- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

Quoted 7 lines
Quoted 4 lines

The script, which using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

Thanks Jirka,

Hi Arnaldo,

Are you OK with the patch?
If yes, I will go ahead to cleanup the *_cpuid_str() by moving them to header.c as promised. https://lkml.org/lkml/2018/11/15/929
The new patch will be on top of this patch.

Thanks,
Kan

#18 lore raw

Em Thu, Nov 15, 2018 at 04:01:46PM -0500, Liang, Kan escreveu:

Quoted 33 lines · Jiri Olsa
Quoted 22 lines · On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:
Quoted 21 lines

From: Kan Liang <[email protected]>

Perf tools cannot find the proper event list for Cascadelake server. Because Cascadelake server and Skylake server have the same CPU model number, which are used by perf tools to find the event list.

The stepping for Skylake server is up to 4. The stepping for Cascadelake server starts from 5. The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().
The stepping information for Skylake server is updated in mapfile.csv. A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model".
- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.
- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

Quoted 7 lines
Quoted 4 lines

The script, which using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

Quoted 8 lines

Thanks Jirka,

Hi Arnaldo,

Are you OK with the patch?
If yes, I will go ahead to cleanup the *_cpuid_str() by moving them to header.c as promised. https://lkml.org/lkml/2018/11/15/929
The new patch will be on top of this patch.

I'm travelling, will look at it soon, can't now, battery almost deead :-\

- Arnaldo

author
#19 lore raw
Quoted 38 lines · Arnaldo Carvalho de Melo

Em Thu, Nov 15, 2018 at 04:01:46PM -0500, Liang, Kan escreveu:

Quoted 37 lines · Jiri Olsa
Quoted 22 lines · On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:
Quoted 21 lines

From: Kan Liang <[email protected]>

Perf tools cannot find the proper event list for Cascadelake server. Because Cascadelake server and Skylake server have the same CPU model number, which are used by perf tools to find the event list.

The stepping for Skylake server is up to 4. The stepping for Cascadelake server starts from 5. The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().
The stepping information for Skylake server is updated in mapfile.csv. A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model".
- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.
- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

Quoted 7 lines
Quoted 4 lines

The script, which using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

Thanks Jirka,

Hi Arnaldo,

Quoted 7 lines
Quoted 4 lines

Are you OK with the patch?
If yes, I will go ahead to cleanup the *_cpuid_str() by moving them to header.c as promised. https://lkml.org/lkml/2018/11/15/929
The new patch will be on top of this patch.

I'm travelling, will look at it soon, can't now, battery almost deead :-\

Hi Arnaldo,

Have you got a chance to look at the code?

Thanks,
Kan

#20 lore raw
In reply to Liang, Kan

Em Thu, Nov 15, 2018 at 04:01:46PM -0500, Liang, Kan escreveu:

Quoted 32 lines · Jiri Olsa
Quoted 22 lines · On Wed, Nov 14, 2018 at 01:24:15PM -0800, [email protected] wrote:
Quoted 21 lines

From: Kan Liang <[email protected]>

Perf tools cannot find the proper event list for Cascadelake server. Because Cascadelake server and Skylake server have the same CPU model number, which are used by perf tools to find the event list.

The stepping for Skylake server is up to 4. The stepping for Cascadelake server starts from 5. The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().
The stepping information for Skylake server is updated in mapfile.csv. A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model".
- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.
- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

Quoted 6 lines
Quoted 4 lines

The script, which using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

Quoted 8 lines

Thanks Jirka,

Hi Arnaldo,

Are you OK with the patch?
If yes, I will go ahead to cleanup the *_cpuid_str() by moving them to header.c as promised. https://lkml.org/lkml/2018/11/15/929
The new patch will be on top of this patch.

Looks ok after reading the discussion between you and Jiri, will now look at the patch that Jiri reviewed.

- Arnaldo

#21 lore raw
In reply to [email protected]
perf vendor events: Add stepping in CPUID string for x86
Commit-ID:  3b54411a4485c9f1d50f003810ecfceca82a09ae
Gitweb:     https://git.kernel.org/tip/3b54411a4485c9f1d50f003810ecfceca82a09ae
Author:     Kan Liang <[email protected]>
AuthorDate: Wed, 14 Nov 2018 13:24:15 -0800
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 21 Nov 2018 22:39:57 -0300

perf vendor events: Add stepping in CPUID string for x86

The perf tools cannot find the proper event list for the Cascadelake server. Because the Cascadelake server and the Skylake server have the same CPU model number, which are used by the perf tools to find the event list.

The stepping for Skylake server is up to 4.

The stepping for Cascadelake server starts from 5.

The stepping can be used to distinguish between them.

The stepping is added in get_cpuid_str().

The stepping information for Skylake server is updated in mapfile.csv.

A x86 specific strcmp_cpuid_cmp() function is added to handle two CPUID formats in mapfile.csv, "vendor-family-model-stepping" and "vendor-family-model":

- If a cpuid-regular-expression from the mapfile.csv using the new stepping format, a cpuid-string generated on the machine must include stepping. Otherwise, it is a mismatch.

- If the cpuid-regular-expression using the old non-stepping format, the stepping in the cpuid-string will be ignored.

The script, using environment string "PERF_CPUID" without stepping on Skylake server, will be broken. If so, users must fix their scripts.

Committer notes:

Fixed this build error on centos:6 and debian:7:

arch/x86/util/header.c: In function 'is_full_cpuid':
arch/x86/util/header.c:82:39: error: declaration of 'cpuid' shadows a global declaration [-Werror=shadow] arch/x86/util/header.c:12:1: error: shadowed declaration is here [-Werror=shadow] arch/x86/util/header.c: In function 'strcmp_cpuid_str':
arch/x86/util/header.c:98:56: error: declaration of 'cpuid' shadows a global declaration [-Werror=shadow] arch/x86/util/header.c:12:1: error: shadowed declaration is here [-Werror=shadow]
cc1: all warnings being treated as errors

---
 tools/perf/arch/x86/util/header.c          | 66 +++++++++++++++++++++++++++++-
 tools/perf/pmu-events/arch/x86/mapfile.csv |  2 +-
 tools/perf/util/pmu.c                      |  2 +-
 3 files changed, 67 insertions(+), 3 deletions(-)
tools/perf/arch/x86/util/header.c +65 -1
index fb0d71afee8b..af9a9f2600be 100644
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <regex.h>
 
 #include "../../util/header.h"
 
@@ -70,9 +71,72 @@ get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
 {
 	char *buf = malloc(128);
 
-	if (buf && __get_cpuid(buf, 128, "%s-%u-%X$") < 0) {
+	if (buf && __get_cpuid(buf, 128, "%s-%u-%X-%X$") < 0) {
 		free(buf);
 		return NULL;
 	}
 	return buf;
 }
+
+/* Full CPUID format for x86 is vendor-family-model-stepping */
+static bool is_full_cpuid(const char *id)
+{
+	const char *tmp = id;
+	int count = 0;
+
+	while ((tmp = strchr(tmp, '-')) != NULL) {
+		count++;
+		tmp++;
+	}
+
+	if (count == 3)
+		return true;
+
+	return false;
+}
+
+int strcmp_cpuid_str(const char *mapcpuid, const char *id)
+{
+	regex_t re;
+	regmatch_t pmatch[1];
+	int match;
+	bool full_mapcpuid = is_full_cpuid(mapcpuid);
+	bool full_cpuid = is_full_cpuid(id);
+
+	/*
+	 * Full CPUID format is required to identify a platform.
+	 * Error out if the cpuid string is incomplete.
+	 */
+	if (full_mapcpuid && !full_cpuid) {
+		pr_info("Invalid CPUID %s. Full CPUID is required, "
+			"vendor-family-model-stepping\n", id);
+		return 1;
+	}
+
+	if (regcomp(&re, mapcpuid, REG_EXTENDED) != 0) {
+		/* Warn unable to generate match particular string. */
+		pr_info("Invalid regular expression %s\n", mapcpuid);
+		return 1;
+	}
+
+	match = !regexec(&re, id, 1, pmatch, 0);
+	regfree(&re);
+	if (match) {
+		size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
+		size_t cpuid_len;
+
+		/* If the full CPUID format isn't required,
+		 * ignoring the stepping.
+		 */
+		if (!full_mapcpuid && full_cpuid)
+			cpuid_len = strrchr(id, '-') - id;
+		else
+			cpuid_len = strlen(id);
+
+		/* Verify the entire string matched. */
+		if (match_len == cpuid_len)
+			return 0;
+	}
+
+	return 1;
+}
tools/perf/pmu-events/arch/x86/mapfile.csv +1 -1
index 7e3cce3bcf3b..183a42c99251 100644
@@ -31,4 +31,4 @@ GenuineIntel-6-2A,v15,sandybridge,core
 GenuineIntel-6-2C,v2,westmereep-dp,core
 GenuineIntel-6-25,v2,westmereep-sp,core
 GenuineIntel-6-2F,v2,westmereex,core
-GenuineIntel-6-55,v1,skylakex,core
+GenuineIntel-6-55-[01234],v1,skylakex,core
tools/perf/util/pmu.c +1 -1
index 7348eea0248f..c660625d7d4b 100644
@@ -670,7 +670,7 @@ char * __weak get_cpuid_str(struct perf_pmu *pmu __maybe_unused)
  * cpuid string generated on this platform.
  * Otherwise return non-zero.
  */
-int strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
+int __weak strcmp_cpuid_str(const char *mapcpuid, const char *cpuid)
 {
 	regex_t re;
 	regmatch_t pmatch[1];