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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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.

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
> 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
>  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;
> +}
> +
>  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. */
> +			if (match_len == strlen(cpuid))
> +				break;
> +		}
>  	}
>  	free(cpuid);
>  	return map;
> -- 
> 2.13.6