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

[Jiri Olsa <jolsa@redhat.com>]

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