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

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

[kan.liang]

From: Kan Liang <kan.liang@linux.intel.com>

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.

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
---
 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(-)

diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
index fb0d71afee8b..b428a4b00bf7 100644
--- a/tools/perf/arch/x86/util/header.c
+++ b/tools/perf/arch/x86/util/header.c
@@ -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;
+}
diff --git a/tools/perf/pmu-events/arch/x86/mapfile.csv b/tools/perf/pmu-events/arch/x86/mapfile.csv
index 7e3cce3bcf3b..183a42c99251 100644
--- a/tools/perf/pmu-events/arch/x86/mapfile.csv
+++ b/tools/perf/pmu-events/arch/x86/mapfile.csv
@@ -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
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index afd68524ffa9..0996260178f7 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -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];
-- 
2.17.1

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

[Jiri Olsa]

On Wed, Nov 14, 2018 at 01:24:16PM -0800, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> 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

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

[Jiri Olsa]

On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:

SNIP

> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index fb0d71afee8b..b428a4b00bf7 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -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);

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

> +
> +	/*
> +	 * 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;

why is this necessary?

thanks,
jirka

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

[Liang, Kan]

On 11/15/2018 8:53 AM, Jiri Olsa wrote:
> On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
> 
> SNIP
> 
>> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
>> index fb0d71afee8b..b428a4b00bf7 100644
>> --- a/tools/perf/arch/x86/util/header.c
>> +++ b/tools/perf/arch/x86/util/header.c
>> @@ -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);
> 
> 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.

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

Sure.

> 
>> +
>> +	/*
>> +	 * 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;
> 
> 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

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

[Jiri Olsa]

On Thu, Nov 15, 2018 at 08:33:27AM -0500, Liang, Kan wrote:
> 
> 
> On 11/15/2018 8:21 AM, Jiri Olsa wrote:
> > On Wed, Nov 14, 2018 at 01:24:16PM -0800, kan.liang@linux.intel.com wrote:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > > 
> > > 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
> > 
> 
> The patch is attached. Can it be applied?

yep, that one works

thanks,
jirka

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

[Liang, Kan]

On 11/15/2018 9:26 AM, Liang, Kan wrote:
> 
> 
> On 11/15/2018 8:53 AM, Jiri Olsa wrote:
>> On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com 
>> wrote:
>>
>> SNIP
>>
>>> diff --git a/tools/perf/arch/x86/util/header.c 
>>> b/tools/perf/arch/x86/util/header.c
>>> index fb0d71afee8b..b428a4b00bf7 100644
>>> --- a/tools/perf/arch/x86/util/header.c
>>> +++ b/tools/perf/arch/x86/util/header.c
>>> @@ -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);
>>
>> 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.
> 
>> 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

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

[Jiri Olsa]

On Thu, Nov 15, 2018 at 09:26:50AM -0500, Liang, Kan wrote:
> 
> 
> On 11/15/2018 8:53 AM, Jiri Olsa wrote:
> > On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
> > 
> > SNIP
> > 
> > > diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> > > index fb0d71afee8b..b428a4b00bf7 100644
> > > --- a/tools/perf/arch/x86/util/header.c
> > > +++ b/tools/perf/arch/x86/util/header.c
> > > @@ -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);
> > 
> > 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.

ok, forgot about this one

> 
> > also please move this to arch/x86/util/pmu.c
> > so it matches the weak function object
> 
> Sure.
> 
> > 
> > > +
> > > +	/*
> > > +	 * 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;
> > 
> > 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.

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

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

[Jiri Olsa]

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

SNIP

> > > > +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?
> > > 
> > 
> > User may set cpuid by environment string "PERF_CPUID", which may not be
> > full format.
> > 
> > > 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?

sounds good, thanks

jirka

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

[Liang, Kan]

>>>> +	/*
>>>> +	 * 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;
>>>
>>> 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.
> 
> 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

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

[Jiri Olsa]

On Thu, Nov 15, 2018 at 10:53:42AM -0500, Liang, Kan wrote:
> 
> > > > > +	/*
> > > > > +	 * 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;
> > > > 
> > > > 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.
> > 
> > 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.

ok, let's keep your way

jirk

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

[Jiri Olsa]

On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> 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.
> 
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>

Reviewed-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

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

[Liang, Kan]

On 11/15/2018 3:44 PM, Jiri Olsa wrote:
> On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> 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.
>>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> 
> Reviewed-by: Jiri Olsa <jolsa@kernel.org>
>

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

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

[Arnaldo Carvalho de Melo]

Em Thu, Nov 15, 2018 at 04:01:46PM -0500, Liang, Kan escreveu:
> 
> 
> On 11/15/2018 3:44 PM, Jiri Olsa wrote:
> > On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> > 
> > Reviewed-by: Jiri Olsa <jolsa@kernel.org>
> > 
> 
> 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

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

[Liang, Kan]

On 11/15/2018 9:14 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Nov 15, 2018 at 04:01:46PM -0500, Liang, Kan escreveu:
>>
>>
>> On 11/15/2018 3:44 PM, Jiri Olsa wrote:
>>> On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
>>>> From: Kan Liang <kan.liang@linux.intel.com>
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>>>
>>> Reviewed-by: Jiri Olsa <jolsa@kernel.org>
>>>
>>
>> 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
> :-\

Hi Arnaldo,

Have you got a chance to look at the code?

Thanks,
Kan

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

[Arnaldo Carvalho de Melo]

Em Thu, Nov 15, 2018 at 04:01:46PM -0500, Liang, Kan escreveu:
> 
> 
> On 11/15/2018 3:44 PM, Jiri Olsa wrote:
> > On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.liang@linux.intel.com wrote:
> > > From: Kan Liang <kan.liang@linux.intel.com>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> > 
> > Reviewed-by: Jiri Olsa <jolsa@kernel.org>
 
> 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

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

[Arnaldo Carvalho de Melo]

Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:
> On Thu, Nov 15, 2018 at 08:33:27AM -0500, Liang, Kan wrote:
> > 
> > 
> > On 11/15/2018 8:21 AM, Jiri Olsa wrote:
> > > On Wed, Nov 14, 2018 at 01:24:16PM -0800, kan.liang@linux.intel.com wrote:
> > > > From: Kan Liang <kan.liang@linux.intel.com>
> > > > 
> > > > 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
> > > 
> > 
> > The patch is attached. Can it be applied?
> 
> yep, that one works

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

- Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Wed, Nov 21, 2018 at 09:51:30AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:
> > On Thu, Nov 15, 2018 at 08:33:27AM -0500, Liang, Kan wrote:
> > > 
> > > 
> > > On 11/15/2018 8:21 AM, Jiri Olsa wrote:
> > > > On Wed, Nov 14, 2018 at 01:24:16PM -0800, kan.liang@linux.intel.com wrote:
> > > > > From: Kan Liang <kan.liang@linux.intel.com>
> > > > > 
> > > > > 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
> > > > 
> > > 
> > > 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

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

[Jiri Olsa]

On Wed, Nov 21, 2018 at 09:54:08AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Nov 21, 2018 at 09:51:30AM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:
> > > On Thu, Nov 15, 2018 at 08:33:27AM -0500, Liang, Kan wrote:
> > > > 
> > > > 
> > > > On 11/15/2018 8:21 AM, Jiri Olsa wrote:
> > > > > On Wed, Nov 14, 2018 at 01:24:16PM -0800, kan.liang@linux.intel.com wrote:
> > > > > > From: Kan Liang <kan.liang@linux.intel.com>
> > > > > > 
> > > > > > 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
> > > > > 
> > > > 
> > > > 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

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

[Arnaldo Carvalho de Melo]

Em Wed, Nov 21, 2018 at 02:02:02PM +0100, Jiri Olsa escreveu:
> On Wed, Nov 21, 2018 at 09:54:08AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Nov 21, 2018 at 09:51:30AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Thu, Nov 15, 2018 at 04:14:42PM +0100, Jiri Olsa escreveu:
> > > > On Thu, Nov 15, 2018 at 08:33:27AM -0500, Liang, Kan wrote:
> > > > > 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 ;-)

Ok, Acked-by kept then. Thanks!

- Arnaldo

[tip:perf/core] perf vendor events: Add stepping in CPUID string for x86

[tip-bot for Kan Liang]

Commit-ID:  3b54411a4485c9f1d50f003810ecfceca82a09ae
Gitweb:     https://git.kernel.org/tip/3b54411a4485c9f1d50f003810ecfceca82a09ae
Author:     Kan Liang <kan.liang@linux.intel.com>
AuthorDate: Wed, 14 Nov 2018 13:24:15 -0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
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

Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/r/20181114212416.15665-1-kan.liang@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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(-)

diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
index fb0d71afee8b..af9a9f2600be 100644
--- a/tools/perf/arch/x86/util/header.c
+++ b/tools/perf/arch/x86/util/header.c
@@ -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;
+}
diff --git a/tools/perf/pmu-events/arch/x86/mapfile.csv b/tools/perf/pmu-events/arch/x86/mapfile.csv
index 7e3cce3bcf3b..183a42c99251 100644
--- a/tools/perf/pmu-events/arch/x86/mapfile.csv
+++ b/tools/perf/pmu-events/arch/x86/mapfile.csv
@@ -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
diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 7348eea0248f..c660625d7d4b 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -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];