Re: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for x86
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.li...@linux.intel.com wrote: > > > From: Kan Liang > > > > > > 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 > > > > Reviewed-by: Jiri Olsa > 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 1/2] perf vendor events: Add stepping in CPUID string for x86
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.li...@linux.intel.com wrote: > > > From: Kan Liang > > > > > > 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 > > > > Reviewed-by: Jiri Olsa > 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 1/2] perf vendor events: Add stepping in CPUID string for x86
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.li...@linux.intel.com wrote: From: Kan Liang 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 Reviewed-by: Jiri Olsa 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
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.li...@linux.intel.com wrote: From: Kan Liang 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 Reviewed-by: Jiri Olsa 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
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.li...@linux.intel.com wrote: > > > From: Kan Liang > > > > > > 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 > > > > Reviewed-by: Jiri Olsa > > > > 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
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.li...@linux.intel.com wrote: > > > From: Kan Liang > > > > > > 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 > > > > Reviewed-by: Jiri Olsa > > > > 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
On 11/15/2018 3:44 PM, Jiri Olsa wrote: On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@linux.intel.com wrote: From: Kan Liang 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 Reviewed-by: Jiri Olsa 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
On 11/15/2018 3:44 PM, Jiri Olsa wrote: On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@linux.intel.com wrote: From: Kan Liang 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 Reviewed-by: Jiri Olsa 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
On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@linux.intel.com wrote: > From: Kan Liang > > 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 Reviewed-by: Jiri Olsa thanks, jirka
Re: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for x86
On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@linux.intel.com wrote: > From: Kan Liang > > 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 Reviewed-by: Jiri Olsa thanks, jirka
Re: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for x86
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(, mapcpuid, REG_EXTENDED) != 0) {
> > > > > + /* Warn unable to generate match particular string. */
> > > > > + pr_info("Invalid regular expression %s\n", mapcpuid);
> > > > > + return 1;
> > > > > + }
> > > > > +
> > > > > + match = !regexec(, cpuid, 1, pmatch, 0);
> > > > > + regfree();
> > > > > + 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
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(, mapcpuid, REG_EXTENDED) != 0) {
> > > > > + /* Warn unable to generate match particular string. */
> > > > > + pr_info("Invalid regular expression %s\n", mapcpuid);
> > > > > + return 1;
> > > > > + }
> > > > > +
> > > > > + match = !regexec(, cpuid, 1, pmatch, 0);
> > > > > + regfree();
> > > > > + 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
+ /*
+* 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(, mapcpuid, REG_EXTENDED) != 0) {
+ /* Warn unable to generate match particular string. */
+ pr_info("Invalid regular expression %s\n", mapcpuid);
+ return 1;
+ }
+
+ match = !regexec(, cpuid, 1, pmatch, 0);
+ regfree();
+ 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
+ /*
+* 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(, mapcpuid, REG_EXTENDED) != 0) {
+ /* Warn unable to generate match particular string. */
+ pr_info("Invalid regular expression %s\n", mapcpuid);
+ return 1;
+ }
+
+ match = !regexec(, cpuid, 1, pmatch, 0);
+ regfree();
+ 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
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
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
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.li...@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
> > > #include
> > > #include
> > > +#include
> > > #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(, mapcpuid, REG_EXTENDED) != 0) {
> > > + /* Warn unable to generate match particular string. */
> > > + pr_info("Invalid regular expression %s\n", mapcpuid);
> > > + return 1;
> > > + }
> > > +
> > > + match = !regexec(, cpuid, 1, pmatch, 0);
> > > + regfree();
> > > + 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
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.li...@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
> > > #include
> > > #include
> > > +#include
> > > #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(, mapcpuid, REG_EXTENDED) != 0) {
> > > + /* Warn unable to generate match particular string. */
> > > + pr_info("Invalid regular expression %s\n", mapcpuid);
> > > + return 1;
> > > + }
> > > +
> > > + match = !regexec(, cpuid, 1, pmatch, 0);
> > > + regfree();
> > > + 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
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.li...@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
#include
#include
+#include
#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
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.li...@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
#include
#include
+#include
#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
On 11/15/2018 8:53 AM, Jiri Olsa wrote:
On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@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
#include
#include
+#include
#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(, mapcpuid, REG_EXTENDED) != 0) {
+ /* Warn unable to generate match particular string. */
+ pr_info("Invalid regular expression %s\n", mapcpuid);
+ return 1;
+ }
+
+ match = !regexec(, cpuid, 1, pmatch, 0);
+ regfree();
+ 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 1/2] perf vendor events: Add stepping in CPUID string for x86
On 11/15/2018 8:53 AM, Jiri Olsa wrote:
On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@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
#include
#include
+#include
#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(, mapcpuid, REG_EXTENDED) != 0) {
+ /* Warn unable to generate match particular string. */
+ pr_info("Invalid regular expression %s\n", mapcpuid);
+ return 1;
+ }
+
+ match = !regexec(, cpuid, 1, pmatch, 0);
+ regfree();
+ 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 1/2] perf vendor events: Add stepping in CPUID string for x86
On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@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
> #include
> #include
> +#include
>
> #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(, mapcpuid, REG_EXTENDED) != 0) {
> + /* Warn unable to generate match particular string. */
> + pr_info("Invalid regular expression %s\n", mapcpuid);
> + return 1;
> + }
> +
> + match = !regexec(, cpuid, 1, pmatch, 0);
> + regfree();
> + 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
On Wed, Nov 14, 2018 at 01:24:15PM -0800, kan.li...@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
> #include
> #include
> +#include
>
> #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(, mapcpuid, REG_EXTENDED) != 0) {
> + /* Warn unable to generate match particular string. */
> + pr_info("Invalid regular expression %s\n", mapcpuid);
> + return 1;
> + }
> +
> + match = !regexec(, cpuid, 1, pmatch, 0);
> + regfree();
> + 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
