From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F340621D583; Wed, 5 Feb 2025 05:26:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738733217; cv=none; b=LlZlsQHdsc/jA5EW9KCJAqJTI05bz7hmkCUsNsmvDEdjAAHXau51XuZFq0xK1/y5M226orrqqEOV1ZTKBDI2uQcddRCLAu5iFgSrDGaMWCkuIMVlrUPlqGsn89T9kWTQXXjqDg+m8SZF8kXqbxODLXHxFMaKPNBJ6zwX7JXZ/Wc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738733217; c=relaxed/simple; bh=hSH/r3YeLaiBL1FD9BdDhseoi/ETyYAwAYwQsByKGwg=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=gIcJ/FZeDdKz8ATKvBBwUT9rDDKQruMSnaurDUAqro7gfqtu8jWc1l0r603OybAtbT1wocFZS7KzbH+OwQVaJILyY5XD1txPJlMkYvBj0GwhwZqIf6HHqPF09j4Tw4iGXQU4+lvhpubWJDofeNdYm9mRmldrQQEk976zUQtwlo0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=hD/+uLxI; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="hD/+uLxI" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0199EC4CED1; Wed, 5 Feb 2025 05:26:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1738733216; bh=hSH/r3YeLaiBL1FD9BdDhseoi/ETyYAwAYwQsByKGwg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=hD/+uLxIfUVqFnyBNvw1ne2jB8XQkg1C8tYuLMtgZUg0ISrcPRc6D1qTs6KySfqMG A3hfSzQSjTVwOnyBMBIaHdK7ws5/6Bu9bKEMQ0lC3cv7MEa2IfvdK++WtPM1B4drnZ U/a5raaj7xhrwKasokguzWDafKbPHAdtqnnOyWhcoppqSYBknC7zaXVgTtAPjh4/Vb xKHAD6UNpZWhiVAApwyy1caerG/oTLM4iEFjYm6zUTkp/oCKVGfWCo94j+v3CWW1AB ye1W+jU+i2aDNPj3hxN7ho8V+Adbu58Q/fars+kyD8CbSYlibydK6NNsqI+AKeBch5 wiIBwPnmXJkBg== Date: Tue, 4 Feb 2025 21:26:54 -0800 From: Namhyung Kim To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Kan Liang , James Clark , Ze Gao , Weilin Wang , Jean-Philippe Romain , Junhao He , Yicong Yang , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/5] perf pmu: Rename name matching for no suffix or wildcard variants Message-ID: References: <20250201074320.746259-1-irogers@google.com> <20250201074320.746259-4-irogers@google.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On Mon, Feb 03, 2025 at 03:10:04PM -0800, Ian Rogers wrote: > On Mon, Feb 3, 2025 at 3:00 PM Namhyung Kim wrote: > > > > Hi Ian, > > > > On Fri, Jan 31, 2025 at 11:43:18PM -0800, Ian Rogers wrote: > > > Wildcard PMU naming will match a name like pmu_1 to a PMU name like > > > pmu_10 but not to a PMU name like pmu_2 as the suffix forms part of > > > the match. No suffix matching will match pmu_10 to either pmu_1 or > > > pmu_2. Add or rename matching functions on PMU to make it clearer what > > > kind of matching is being performed. > > > > > > Signed-off-by: Ian Rogers > > > --- > > > tools/perf/pmu-events/empty-pmu-events.c | 8 +- > > > tools/perf/pmu-events/jevents.py | 8 +- > > > tools/perf/tests/pmu.c | 85 ++++---- > > > tools/perf/util/parse-events.c | 2 +- > > > tools/perf/util/pmu.c | 256 ++++++++++++++++------- > > > tools/perf/util/pmu.h | 5 +- > > > 6 files changed, 235 insertions(+), 129 deletions(-) > > > > > [...] > > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > > > index 6206c8fe2bf9..c2a15b0259cf 100644 > > > --- a/tools/perf/util/pmu.c > > > +++ b/tools/perf/util/pmu.c > > > @@ -847,21 +847,23 @@ static size_t pmu_deduped_name_len(const struct perf_pmu *pmu, const char *name, > > > } > > > > > > /** > > > - * perf_pmu__match_ignoring_suffix - Does the pmu_name match tok ignoring any > > > - * trailing suffix? The Suffix must be in form > > > - * tok_{digits}, or tok{digits}. > > > + * perf_pmu__match_wildcard - Does the pmu_name start with tok and is then only > > > + * followed by nothing or a suffix? tok may contain > > > + * part of a suffix. > > > * @pmu_name: The pmu_name with possible suffix. > > > - * @tok: The possible match to pmu_name without suffix. > > > + * @tok: The wildcard argument to match. > > > */ > > > -static bool perf_pmu__match_ignoring_suffix(const char *pmu_name, const char *tok) > > > +static bool perf_pmu__match_wildcard(const char *pmu_name, const char *tok) > > > > It'd be nice if we use a function prefix with "__" only if the first > > argument is the corresponding type. How about renaming it to > > pmu_name_match_wildcard() and others? > > > > Otherwise, looks good to me. > > I think the intent is to make them look like (in the C++ sense) static > member functions. I was following the existing convention as with > perf_pmu__event_source_devices_scnprintf, > perf_pmu__pathname_scnprintf, perf_pmu__convert_scale, > perf_pmu__event_source_devices_fd, perf_pmu__pathname_fd, all the > perf_pmus functions, etc. I still think it's better to match the prefix with the data type but we can fix them all later. Thanks, Namhyung > > > > > { > > > const char *p, *suffix; > > > bool has_hex = false; > > > + size_t tok_len = strlen(tok); > > > > > > - if (strncmp(pmu_name, tok, strlen(tok))) > > > + /* Check start of pmu_name for equality. */ > > > + if (strncmp(pmu_name, tok, tok_len)) > > > return false; > > > > > > - suffix = p = pmu_name + strlen(tok); > > > + suffix = p = pmu_name + tok_len; > > > if (*p == 0) > > > return true; > > > > > > @@ -887,60 +889,84 @@ static bool perf_pmu__match_ignoring_suffix(const char *pmu_name, const char *to > > > } > > > > > > /** > > > - * pmu_uncore_alias_match - does name match the PMU name? > > > - * @pmu_name: the json struct pmu_event name. This may lack a suffix (which > > > + * perf_pmu__match_ignoring_suffix_uncore - Does the pmu_name match tok ignoring > > > + * any trailing suffix on pmu_name and > > > + * tok? The Suffix must be in form > > > + * tok_{digits}, or tok{digits}. > > > + * @pmu_name: The pmu_name with possible suffix. > > > + * @tok: The possible match to pmu_name. > > > + */ > > > +static bool perf_pmu__match_ignoring_suffix_uncore(const char *pmu_name, const char *tok) > > > +{ > > > + size_t pmu_name_len, tok_len; > > > + > > > + /* For robustness, check for NULL. */ > > > + if (pmu_name == NULL) > > > + return tok == NULL; > > > + > > > + /* uncore_ prefixes are ignored. */ > > > + if (!strncmp(pmu_name, "uncore_", 7)) > > > + pmu_name += 7; > > > + if (!strncmp(tok, "uncore_", 7)) > > > + tok += 7; > > > + > > > + pmu_name_len = pmu_name_len_no_suffix(pmu_name); > > > + tok_len = pmu_name_len_no_suffix(tok); > > > + if (pmu_name_len != tok_len) > > > + return false; > > > + > > > + return strncmp(pmu_name, tok, pmu_name_len) == 0; > > > +} > > > + > > > + > > > +/** > > > + * perf_pmu__match_wildcard_uncore - does to_match match the PMU's name? > > > + * @pmu_name: The pmu->name or pmu->alias to match against. > > > + * @to_match: the json struct pmu_event name. This may lack a suffix (which > > > * matches) or be of the form "socket,pmuname" which will match > > > * "socketX_pmunameY". > > > - * @name: a real full PMU name as from sysfs. > > > */ > > > -static bool pmu_uncore_alias_match(const char *pmu_name, const char *name) > > > +static bool perf_pmu__match_wildcard_uncore(const char *pmu_name, const char *to_match) > > > { > > > - char *tmp = NULL, *tok, *str; > > > - bool res; > > > - > > > - if (strchr(pmu_name, ',') == NULL) > > > - return perf_pmu__match_ignoring_suffix(name, pmu_name); > > > + char *mutable_to_match, *tok, *tmp; > > > > > > - str = strdup(pmu_name); > > > - if (!str) > > > + if (!pmu_name) > > > return false; > > > > > > - /* > > > - * uncore alias may be from different PMU with common prefix > > > - */ > > > - tok = strtok_r(str, ",", &tmp); > > > - if (strncmp(pmu_name, tok, strlen(tok))) { > > > - res = false; > > > - goto out; > > > - } > > > + /* uncore_ prefixes are ignored. */ > > > + if (!strncmp(pmu_name, "uncore_", 7)) > > > + pmu_name += 7; > > > + if (!strncmp(to_match, "uncore_", 7)) > > > + to_match += 7; > > > > > > - /* > > > - * Match more complex aliases where the alias name is a comma-delimited > > > - * list of tokens, orderly contained in the matching PMU name. > > > - * > > > - * Example: For alias "socket,pmuname" and PMU "socketX_pmunameY", we > > > - * match "socket" in "socketX_pmunameY" and then "pmuname" in > > > - * "pmunameY". > > > - */ > > > - while (1) { > > > - char *next_tok = strtok_r(NULL, ",", &tmp); > > > + if (strchr(to_match, ',') == NULL) > > > + return perf_pmu__match_wildcard(pmu_name, to_match); > > > > > > - name = strstr(name, tok); > > > - if (!name || > > > - (!next_tok && !perf_pmu__match_ignoring_suffix(name, tok))) { > > > - res = false; > > > - goto out; > > > + /* Process comma separated list of PMU name components. */ > > > + mutable_to_match = strdup(to_match); > > > + if (!mutable_to_match) > > > + return false; > > > + > > > + tok = strtok_r(mutable_to_match, ",", &tmp); > > > + while (tok) { > > > + size_t tok_len = strlen(tok); > > > + > > > + if (strncmp(pmu_name, tok, tok_len)) { > > > + /* Mismatch between part of pmu_name and tok. */ > > > + free(mutable_to_match); > > > + return false; > > > } > > > - if (!next_tok) > > > - break; > > > - tok = next_tok; > > > - name += strlen(tok); > > > + /* Move pmu_name forward over tok and suffix. */ > > > + pmu_name += tok_len; > > > + while (*pmu_name != '\0' && isdigit(*pmu_name)) > > > + pmu_name++; > > > + if (*pmu_name == '_') > > > + pmu_name++; > > > + > > > + tok = strtok_r(NULL, ",", &tmp); > > > } > > > - > > > - res = true; > > > -out: > > > - free(str); > > > - return res; > > > + free(mutable_to_match); > > > + return *pmu_name == '\0'; > > > } > > > > > > bool pmu_uncore_identifier_match(const char *compat, const char *id) > > > @@ -1003,11 +1029,19 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe, > > > { > > > struct perf_pmu *pmu = vdata; > > > > > > - if (!pe->compat || !pe->pmu) > > > + if (!pe->compat || !pe->pmu) { > > > + /* No data to match. */ > > > return 0; > > > + } > > > + > > > + if (!perf_pmu__match_wildcard_uncore(pmu->name, pe->pmu) && > > > + !perf_pmu__match_wildcard_uncore(pmu->alias_name, pe->pmu)) { > > > + /* PMU name/alias_name don't match. */ > > > + return 0; > > > + } > > > > > > - if (pmu_uncore_alias_match(pe->pmu, pmu->name) && > > > - pmu_uncore_identifier_match(pe->compat, pmu->id)) { > > > + if (pmu_uncore_identifier_match(pe->compat, pmu->id)) { > > > + /* Id matched. */ > > > perf_pmu__new_alias(pmu, > > > pe->name, > > > pe->desc, > > > @@ -1016,7 +1050,6 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe, > > > pe, > > > EVENT_SRC_SYS_JSON); > > > } > > > - > > > return 0; > > > } > > > > > > @@ -1974,15 +2007,82 @@ int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus, > > > return ret; > > > } > > > > > > -bool pmu__name_match(const struct perf_pmu *pmu, const char *pmu_name) > > > +static bool perf_pmu___name_match(const struct perf_pmu *pmu, const char *to_match, bool wildcard) > > > { > > > - return !strcmp(pmu->name, pmu_name) || > > > - (pmu->is_uncore && pmu_uncore_alias_match(pmu_name, pmu->name)) || > > > + const char *names[2] = { > > > + pmu->name, > > > + pmu->alias_name, > > > + }; > > > + if (pmu->is_core) { > > > + for (size_t i = 0; i < ARRAY_SIZE(names); i++) { > > > + const char *name = names[i]; > > > + > > > + if (!name) > > > + continue; > > > + > > > + if (!strcmp(name, to_match)) { > > > + /* Exact name match. */ > > > + return true; > > > + } > > > + } > > > + if (!strcmp(to_match, "default_core")) { > > > + /* > > > + * jevents and tests use default_core as a marker for any core > > > + * PMU as the PMU name varies across architectures. > > > + */ > > > + return true; > > > + } > > > + return false; > > > + } > > > + if (!pmu->is_uncore) { > > > /* > > > - * jevents and tests use default_core as a marker for any core > > > - * PMU as the PMU name varies across architectures. > > > + * PMU isn't core or uncore, some kind of broken CPU mask > > > + * situation. Only match exact name. > > > */ > > > - (pmu->is_core && !strcmp(pmu_name, "default_core")); > > > + for (size_t i = 0; i < ARRAY_SIZE(names); i++) { > > > + const char *name = names[i]; > > > + > > > + if (!name) > > > + continue; > > > + > > > + if (!strcmp(name, to_match)) { > > > + /* Exact name match. */ > > > + return true; > > > + } > > > + } > > > + return false; > > > + } > > > + for (size_t i = 0; i < ARRAY_SIZE(names); i++) { > > > + const char *name = names[i]; > > > + > > > + if (wildcard && perf_pmu__match_wildcard_uncore(name, to_match)) > > > + return true; > > > + if (!wildcard && perf_pmu__match_ignoring_suffix_uncore(name, to_match)) > > > + return true; > > > + } > > > + return false; > > > +} > > > + > > > +/** > > > + * perf_pmu__name_wildcard_match - Called by the jevents generated code to see > > > + * if pmu matches the json to_match string. > > > + * @pmu: The pmu whose name/alias to match. > > > + * @to_match: The possible match to pmu_name. > > > + */ > > > +bool perf_pmu__name_wildcard_match(const struct perf_pmu *pmu, const char *to_match) > > > +{ > > > + return perf_pmu___name_match(pmu, to_match, /*wildcard=*/true); > > > +} > > > + > > > +/** > > > + * perf_pmu__name_no_suffix_match - Does pmu's name match to_match ignoring any > > > + * trailing suffix on the pmu_name and/or tok? > > > + * @pmu: The pmu whose name/alias to match. > > > + * @to_match: The possible match to pmu_name. > > > + */ > > > +bool perf_pmu__name_no_suffix_match(const struct perf_pmu *pmu, const char *to_match) > > > +{ > > > + return perf_pmu___name_match(pmu, to_match, /*wildcard=*/false); > > > } > > > > > > bool perf_pmu__is_software(const struct perf_pmu *pmu) > > > @@ -2229,29 +2329,31 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config, > > > name ?: "N/A", buf, config_name, config); > > > } > > > > > > -bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok) > > > +bool perf_pmu__wildcard_match(const struct perf_pmu *pmu, const char *wildcard_to_match) > > > { > > > - const char *name = pmu->name; > > > - bool need_fnmatch = strisglob(tok); > > > + const char *names[2] = { > > > + pmu->name, > > > + pmu->alias_name, > > > + }; > > > + bool need_fnmatch = strisglob(wildcard_to_match); > > > > > > - if (!strncmp(tok, "uncore_", 7)) > > > - tok += 7; > > > - if (!strncmp(name, "uncore_", 7)) > > > - name += 7; > > > + if (!strncmp(wildcard_to_match, "uncore_", 7)) > > > + wildcard_to_match += 7; > > > > > > - if (perf_pmu__match_ignoring_suffix(name, tok) || > > > - (need_fnmatch && !fnmatch(tok, name, 0))) > > > - return true; > > > + for (size_t i = 0; i < ARRAY_SIZE(names); i++) { > > > + const char *pmu_name = names[i]; > > > > > > - name = pmu->alias_name; > > > - if (!name) > > > - return false; > > > + if (!pmu_name) > > > + continue; > > > > > > - if (!strncmp(name, "uncore_", 7)) > > > - name += 7; > > > + if (!strncmp(pmu_name, "uncore_", 7)) > > > + pmu_name += 7; > > > > > > - return perf_pmu__match_ignoring_suffix(name, tok) || > > > - (need_fnmatch && !fnmatch(tok, name, 0)); > > > + if (perf_pmu__match_wildcard(pmu_name, wildcard_to_match) || > > > + (need_fnmatch && !fnmatch(wildcard_to_match, pmu_name, 0))) > > > + return true; > > > + } > > > + return false; > > > } > > > > > > int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size) > > > diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h > > > index edd36c20aedc..f5306428c03f 100644 > > > --- a/tools/perf/util/pmu.h > > > +++ b/tools/perf/util/pmu.h > > > @@ -240,7 +240,8 @@ bool perf_pmu__have_event(struct perf_pmu *pmu, const char *name); > > > size_t perf_pmu__num_events(struct perf_pmu *pmu); > > > int perf_pmu__for_each_event(struct perf_pmu *pmu, bool skip_duplicate_pmus, > > > void *state, pmu_event_callback cb); > > > -bool pmu__name_match(const struct perf_pmu *pmu, const char *pmu_name); > > > +bool perf_pmu__name_wildcard_match(const struct perf_pmu *pmu, const char *to_match); > > > +bool perf_pmu__name_no_suffix_match(const struct perf_pmu *pmu, const char *to_match); > > > > > > /** > > > * perf_pmu_is_software - is the PMU a software PMU as in it uses the > > > @@ -275,7 +276,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config, > > > const char *config_name); > > > void perf_pmu__warn_invalid_formats(struct perf_pmu *pmu); > > > > > > -bool perf_pmu__match(const struct perf_pmu *pmu, const char *tok); > > > +bool perf_pmu__wildcard_match(const struct perf_pmu *pmu, const char *wildcard_to_match); > > > > > > int perf_pmu__event_source_devices_scnprintf(char *pathname, size_t size); > > > int perf_pmu__pathname_scnprintf(char *buf, size_t size, > > > -- > > > 2.48.1.362.g079036d154-goog > > >