| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <49b4abc7-e1f6-0419-362f-33c8f6818b44@linux.vnet.ibm.com>
Date: Thu, 23 Nov 2017 11:36:10 +0530
From: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
To: William Cohen <wcohen@...hat.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, peterz@...radead.org,
mingo@...hat.com, mpetlan@...hat.com,
alexander.shishkin@...ux.intel.com, jolsa@...hat.com,
namhyung@...nel.org, sukadev@...ux.vnet.ibm.com,
maddy@...ux.vnet.ibm.com, shriyak@...ux.vnet.ibm.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Subject: Re: [PATCH] Use more flexible pattern matching for CPU identification
for mapfile.csv
Hi William,
On 11/23/2017 12:56 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Nov 22, 2017 at 11:08:49AM -0500, William Cohen escreveu:
>> The powerpc cpuid information includes chip revision information.
>> Changes between chip revisions are usually minor bug fixes and usually
>> do not affect the operation of the performance monitoring hardware.
>> The original mapfile.csv matching requires enumerating every possible
>> cpuid string. When a new minor chip revision is produced a new entry
>> has to be added to the mapfile.csv and the code recompiled to allow
>> perf to have the implementation specific perf events for this new
>> minor revision. For users of various distibutions of Linux having to
>> wait for a new release of the kernel's perf tool to be built with
>> these trivial patches is inconvenient.
>>
>> Using regular expressions rather than exactly string matching of the
>> entire cpuid string allows developers to write mapfile.csv files that
>> do not require patches and recompiles for each of these minor version
>> changes. If special cases need to be made for some particular
>> versions, they can be placed earlier in the mapfile.csv file before
>> the more general matches.
This is a good idea. I think the patch can be optimized a bit.
Please see my comments.
> Looks ok, but you forgot to add lkml to the CC list, I'm doing that now
> and also adding Ravi Bangoria @ IBM, which sent yet another of those
> lines that would match your regexp.
>
> Ravi & IBM crew, please see if we can proceed with Will's regexp approach, ok?
>
> - Arnaldo
>
>> Signed-off-by: William Cohen <wcohen@...hat.com>
>> ---
>> tools/perf/pmu-events/arch/powerpc/mapfile.csv | 14 +++------
>> tools/perf/pmu-events/arch/x86/mapfile.csv | 5 +---
>> tools/perf/pmu-events/jevents.c | 39 +++++++++++++++++++++++++-
>> tools/perf/util/pmu.c | 20 ++++++++++++-
>> 4 files changed, 62 insertions(+), 16 deletions(-)
>>
>> diff --git a/tools/perf/pmu-events/arch/powerpc/mapfile.csv b/tools/perf/pmu-events/arch/powerpc/mapfile.csv
>> index a0f3a11ca19f..9ad0469057b8 100644
>> --- a/tools/perf/pmu-events/arch/powerpc/mapfile.csv
>> +++ b/tools/perf/pmu-events/arch/powerpc/mapfile.csv
>> @@ -13,13 +13,7 @@
>> #
>>
>> # Power8 entries
>> -004b0000,1,power8,core
>> -004b0201,1,power8,core
>> -004c0000,1,power8,core
>> -004d0000,1,power8,core
>> -004d0100,1,power8,core
>> -004d0200,1,power8,core
>> -004c0100,1,power8,core
>> -004e0100,1,power9,core
>> -004e0200,1,power9,core
>> -004e1200,1,power9,core
>> +004b[[:xdigit:]]\+,1,power8,core
>> +004c[[:xdigit:]]\+,1,power8,core
>> +004d[[:xdigit:]]\+,1,power8,core
>> +004e[[:xdigit:]]\+,1,power9,core
Instead of 3 regex for Power8, can we compact it into only 1:
004[bcd][[:xdigit:]]...
Both Power8 and Power9 revision number are 16 bits so,
instead of arbitrary number of xdigit, I think, we can limit
it to 4: [[:xdigit:]]{4}
AFAIK, you _have to_ use REG_EXTENDED in both regcomp()
and regexec() to make it work.
>> diff --git a/tools/perf/pmu-events/arch/x86/mapfile.csv b/tools/perf/pmu-events/arch/x86/mapfile.csv
>> index fe1a2c47cabf..93656f2fd53a 100644
>> --- a/tools/perf/pmu-events/arch/x86/mapfile.csv
>> +++ b/tools/perf/pmu-events/arch/x86/mapfile.csv
>> @@ -23,10 +23,7 @@ GenuineIntel-6-1E,v2,nehalemep,core
>> GenuineIntel-6-1F,v2,nehalemep,core
>> GenuineIntel-6-1A,v2,nehalemep,core
>> GenuineIntel-6-2E,v2,nehalemex,core
>> -GenuineIntel-6-4E,v24,skylake,core
>> -GenuineIntel-6-5E,v24,skylake,core
>> -GenuineIntel-6-8E,v24,skylake,core
>> -GenuineIntel-6-9E,v24,skylake,core
>> +GenuineIntel-6-[4589]E,v24,skylake,core
Ok, so all entries for powerpc are regex, but its not the case with x86?
>> GenuineIntel-6-37,v13,silvermont,core
>> GenuineIntel-6-4D,v13,silvermont,core
>> GenuineIntel-6-4C,v13,silvermont,core
>> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
>> index 9eb7047bafe4..4102664ac1bf 100644
>> --- a/tools/perf/pmu-events/jevents.c
>> +++ b/tools/perf/pmu-events/jevents.c
>> @@ -116,6 +116,43 @@ static void fixdesc(char *s)
>> *e = 0;
>> }
>>
>> +/* Add escapes for '\' so they are proper C strings. */
>> +static char *fixregex(char *s)
>> +{
>> + int len = 0;
>> + int esc_count = 0;
>> + char *fixed = NULL;
>> + char *p, *q;
>> +
>> + /* Count the number of '\' in string */
>> + for (p = s; *p; p++) {
>> + ++len;
>> + if (*p == '\\')
>> + ++esc_count;
>> + }
>> +
>> + if (esc_count == 0)
>> + return s;
>> +
>> + /* allocate space for a new string */
>> + fixed = (char *) malloc(len+1);
>> + if (!fixed)
>> + return NULL;
>> +
>> + /* copy over the characters */
>> + q = fixed;
>> + for (p = s; *p; p++) {
>> + if (*p == '\\') {
>> + *q = '\\';
>> + ++q;
>> + }
>> + *q = *p;
>> + ++q;
>> + }
>> + *q = '\0';
>> + return fixed;
>> +}
>> +
AFAICS, you are processing cpuid to add one more '\' if
you find '\' in regex. Can't you directly use '\\' in mapfile?
Something like:
004b[[:xdigit:]]\\+,1,power8,core
Then you don't need this function at all.
>> static struct msrmap {
>> const char *num;
>> const char *pname;
>> @@ -648,7 +685,7 @@ static int process_mapfile(FILE *outfp, char *fpath)
>> }
>> line[strlen(line)-1] = '\0';
>>
>> - cpuid = strtok_r(p, ",", &save);
>> + cpuid = fixregex(strtok_r(p, ",", &save));
>> version = strtok_r(NULL, ",", &save);
>> fname = strtok_r(NULL, ",", &save);
>> type = strtok_r(NULL, ",", &save);
>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>> index 07cb2ac041d7..56942b748d87 100644
>> --- a/tools/perf/util/pmu.c
>> +++ b/tools/perf/util/pmu.c
>> @@ -12,6 +12,7 @@
>> #include <dirent.h>
>> #include <api/fs/fs.h>
>> #include <locale.h>
>> +#include <regex.h>
>> #include "util.h"
>> #include "pmu.h"
>> #include "parse-events.h"
>> @@ -570,14 +571,31 @@ struct pmu_events_map *perf_pmu__find_map(void)
>>
>> i = 0;
>> for (;;) {
>> + regex_t re;
>> + regmatch_t pmatch[1];
>> + int match;
>> +
>> map = &pmu_events_map[i++];
>> if (!map->table) {
>> map = NULL;
>> break;
>> }
>>
>> - if (!strcmp(map->cpuid, cpuid))
>> + if (regcomp(&re, map->cpuid, 0) != 0) {
>> + /* Warn unable to generate match particular string. */
>> + pr_info("Invalid regular expression %s\n", map->cpuid);
>> break;
>> + }
>> +
>> + match = !regexec(&re, cpuid, 1, pmatch, REG_EXTENDED);
>> + regfree(&re);
>> + if (match) {
>> + size_t match_len = (pmatch[0].rm_eo - pmatch[0].rm_so);
>> +
>> + /* Verify the entire string matched. */
I think, if you can add '$' at the end of regex, you don't need this
check.
So, f.e. for powerpc, final content of the file may look like:
004[bcd][[:xdigit:]]{4}$,1,power8,core
004e[[:xdigit:]]{4}$,1,power9,core
Though, I haven't gone through x86 entries. Please make sure
my comments works for all archs before changing the patch.
Thanks,
Ravi
>> + if (match_len == strlen(cpuid))
>> + break;
>> + }
>> }
>> free(cpuid);
>> return map;
>> --
>> 2.13.6
Powered by blists - more mailing lists