Re: [PATCH] Use more flexible pattern matching for CPU identification for mapfile.csv

[Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>]

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