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

From: Jiri Olsa
Date: Thu Nov 15 2018 - 15:02:16 EST


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