From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752550AbdKWGGg (ORCPT ); Thu, 23 Nov 2017 01:06:36 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:35496 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751276AbdKWGGf (ORCPT ); Thu, 23 Nov 2017 01:06:35 -0500 Subject: Re: [PATCH] Use more flexible pattern matching for CPU identification for mapfile.csv To: William Cohen Cc: Arnaldo Carvalho de Melo , peterz@infradead.org, mingo@redhat.com, mpetlan@redhat.com, alexander.shishkin@linux.intel.com, jolsa@redhat.com, namhyung@kernel.org, sukadev@linux.vnet.ibm.com, maddy@linux.vnet.ibm.com, shriyak@linux.vnet.ibm.com, Linux Kernel Mailing List , Ravi Bangoria References: <20171122160849.8400-1-wcohen@redhat.com> <20171122192659.GB2383@kernel.org> From: Ravi Bangoria Date: Thu, 23 Nov 2017 11:36:10 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171122192659.GB2383@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17112306-0016-0000-0000-000005049E99 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17112306-0017-0000-0000-0000284071AD Message-Id: <49b4abc7-e1f6-0419-362f-33c8f6818b44@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-23_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1711230086 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >> #include >> #include >> +#include >> #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