Messages in this thread Patch in this message | | | Subject | Re: [PATCH] Use more flexible pattern matching for CPU identification for mapfile.csv | From | Ravi Bangoria <> | Date | Thu, 23 Nov 2017 11:36:10 +0530 |
| |
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@redhat.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
| |