lkml.org 
[lkml]   [2018]   [Nov]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] perf vendor events: Add stepping in CPUID string for x86
From
Date


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

\
 
 \ /
  Last update: 2018-11-15 15:27    [W:0.154 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site