Message-ID: <012d38fe-6635-82e8-5273-a4f073f04ffa@redhat.com>
Date:   Mon, 27 Nov 2017 11:34:28 -0500
From:   William Cohen <wcohen@...hat.com>
To:     Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.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>
Subject: Re: [PATCH] Use more flexible pattern matching for CPU identification
 for mapfile.csv

On 11/23/2017 01:06 AM, Ravi Bangoria wrote:
> 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.

Hi Ravi,

Thanks for the comments.  I made a some changes based on the comments and will be sending v2 shortly.  
> 
>> 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}

revised the power mapfile.cvs based on the comment above.

> 
> AFAIK, you _have to_ use REG_EXTENDED in both regcomp()
> and regexec() to make it work.

The missing REG_EXTENDED was an oversight.  Corrrected in v2 patch.

> 
>>> 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?

The change is the Intel mapfile.csv was mainly for testing purposes, so I could check that the regular expression logic was working on the x86 test machine.  Changing the x86 mapfile.cvs isn't as big a win as for the power.  The version information is separate for the x86 processors.  There are some entries that can be combined like the skylake, but many are just a single entry.

> 
>>>   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.

Initially during development fixregex didn't exist. However, it was added in the interest of making the code easier to use. The regular expressions in the mapfile.csv are plain regular expressions, not regular expressions where one needs to add '\' to each of the '\'. 

> 
>>>   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.

The check for entire cpuid being matched is a design decision to avoid the chance of accidentally doing a partial match. This could certainly be the case with an incorrect regex for set of hex digits like power.  It seemed preferable to people putting a '^' and '$' on each entry in the mapfile.csv files.

> 
> 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.

The revised patch has been tested on power9 and a x86 skylake machine.  Currently, those are the only archtectures that use mapfile.csv files.

Thanks,

-Will

> 
> Thanks,
> Ravi
> 
>>> +            if (match_len == strlen(cpuid))
>>> +                break;
>>> +        }
>>>       }
>>>       free(cpuid);
>>>       return map;
>>> -- 
>>> 2.13.6
> 

Powered by blists - more mailing lists