Message-ID: <CAP-5=fVZxtgxj62EFNzweNfENVHrc8DwExcUGH-3odz=ZmMrQA@mail.gmail.com>
Date:   Tue, 29 Aug 2023 16:46:23 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     James Clark <james.clark@....com>
Cc:     linux-perf-users@...r.kernel.org, john.g.garry@...cle.com,
        renyu.zj@...ux.alibaba.com, acme@...nel.org,
        Will Deacon <will@...nel.org>,
        Mike Leach <mike.leach@...aro.org>,
        Leo Yan <leo.yan@...aro.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Haixin Yu <yuhaixin.yhx@...ux.alibaba.com>,
        Nick Forrington <nick.forrington@....com>,
        Kajol Jain <kjain@...ux.ibm.com>,
        Eduard Zingerman <eddyz87@...il.com>,
        Sohom Datta <sohomdatta1@...il.com>,
        Rob Herring <robh@...nel.org>, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6 4/6] perf jevents: Add a new expression builtin strcmp_cpuid_str()

On Wed, Aug 16, 2023 at 4:52 AM James Clark <james.clark@....com> wrote:
>
> This will allow writing formulas that are conditional on a specific
> CPU type or CPU version. It calls through to the existing
> strcmp_cpuid_str() function in Perf which has a default weak version,
> and an arch specific version for x86 and arm64.
>
> The function takes an 'ID' type value, which is a string. But in this
> case Arm CPU IDs are hex numbers prefixed with '0x'. metric.py
> assumes strings are only used by event names, and that they can't start
> with a number ('0'), so an additional change has to be made to the
> regex to convert hex numbers back to 'ID' types.
>
> Signed-off-by: James Clark <james.clark@....com>
> ---
>  tools/perf/arch/arm64/util/pmu.c | 18 +-----------------
>  tools/perf/pmu-events/metric.py  | 17 +++++++++++++++--
>  tools/perf/util/expr.c           | 18 ++++++++++++++++++
>  tools/perf/util/expr.h           |  1 +
>  tools/perf/util/expr.l           |  1 +
>  tools/perf/util/expr.y           |  8 +++++++-
>  tools/perf/util/pmu.c            | 17 +++++++++++++++++
>  tools/perf/util/pmu.h            |  1 +
>  8 files changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
> index 512a8f13c4de..615084eb88d8 100644
> --- a/tools/perf/arch/arm64/util/pmu.c
> +++ b/tools/perf/arch/arm64/util/pmu.c
> @@ -2,28 +2,12 @@
>
>  #include <internal/cpumap.h>
>  #include "../../../util/cpumap.h"
> +#include "../../../util/header.h"
>  #include "../../../util/pmu.h"
>  #include "../../../util/pmus.h"
>  #include <api/fs/fs.h>
>  #include <math.h>
>
> -static struct perf_pmu *pmu__find_core_pmu(void)
> -{
> -       struct perf_pmu *pmu = NULL;
> -
> -       while ((pmu = perf_pmus__scan_core(pmu))) {
> -               /*
> -                * The cpumap should cover all CPUs. Otherwise, some CPUs may
> -                * not support some events or have different event IDs.
> -                */
> -               if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)
> -                       return NULL;
> -
> -               return pmu;
> -       }
> -       return NULL;
> -}
> -
>  const struct pmu_metrics_table *pmu_metrics_table__find(void)
>  {
>         struct perf_pmu *pmu = pmu__find_core_pmu();
> diff --git a/tools/perf/pmu-events/metric.py b/tools/perf/pmu-events/metric.py
> index 85a3545f5b6a..0e9ec65d92ae 100644
> --- a/tools/perf/pmu-events/metric.py
> +++ b/tools/perf/pmu-events/metric.py
> @@ -413,6 +413,10 @@ def has_event(event: Event) -> Function:
>    # pylint: disable=invalid-name
>    return Function('has_event', event)
>
> +def strcmp_cpuid_str(event: str) -> Function:
> +  # pylint: disable=redefined-builtin
> +  # pylint: disable=invalid-name
> +  return Function('strcmp_cpuid_str', event)
>
>  class Metric:
>    """An individual metric that will specifiable on the perf command line."""
> @@ -541,14 +545,23 @@ def ParsePerfJson(orig: str) -> Expression:
>    """
>    # pylint: disable=eval-used
>    py = orig.strip()
> +  # First try to convert everything that looks like a string (event name) into Event(r"EVENT_NAME").
> +  # This isn't very selective so is followed up by converting some unwanted conversions back again
>    py = re.sub(r'([a-zA-Z][^-+/\* \\\(\),]*(?:\\.[^-+/\* \\\(\),]*)*)',
>                r'Event(r"\1")', py)
> +  # If it started with a # it should have been a literal, rather than an event name
>    py = re.sub(r'#Event\(r"([^"]*)"\)', r'Literal("#\1")', py)
> +  # Convert accidentally converted hex constants ("0Event(r"xDEADBEEF)"") back to a constant,
> +  # but keep it wrapped in Event(), otherwise Python drops the 0x prefix and it gets interpreted as
> +  # a double by the Bison parser
> +  py = re.sub(r'0Event\(r"[xX]([0-9a-fA-F]*)"\)', r'Event("0x\1")', py)
> +  # Convert accidentally converted scientific notation constants back
>    py = re.sub(r'([0-9]+)Event\(r"(e[0-9]+)"\)', r'\1\2', py)
> -  keywords = ['if', 'else', 'min', 'max', 'd_ratio', 'source_count', 'has_event']
> +  # Convert all the known keywords back from events to just the keyword
> +  keywords = ['if', 'else', 'min', 'max', 'd_ratio', 'source_count', 'has_event', 'strcmp_cpuid_str',
> +              'cpuid_not_more_than']
>    for kw in keywords:
>      py = re.sub(rf'Event\(r"{kw}"\)', kw, py)
> -
>    try:
>      parsed = ast.parse(py, mode='eval')
>    except SyntaxError as e:
> diff --git a/tools/perf/util/expr.c b/tools/perf/util/expr.c
> index 7410a165f68b..0985a3cbc6f9 100644
> --- a/tools/perf/util/expr.c
> +++ b/tools/perf/util/expr.c
> @@ -13,6 +13,8 @@
>  #include <util/expr-bison.h>
>  #include <util/expr-flex.h>
>  #include "util/hashmap.h"
> +#include "util/header.h"
> +#include "util/pmu.h"
>  #include "smt.h"
>  #include "tsc.h"
>  #include <api/fs/fs.h>
> @@ -495,3 +497,19 @@ double expr__has_event(const struct expr_parse_ctx *ctx, bool compute_ids, const
>         evlist__delete(tmp);
>         return ret;
>  }
> +
> +double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx __maybe_unused,
> +                      bool compute_ids __maybe_unused, const char *test_id)
> +{
> +       double ret;
> +       struct perf_pmu *pmu = pmu__find_core_pmu();
> +       char *cpuid = perf_pmu__getcpuid(pmu);
> +
> +       if (!cpuid)
> +               return NAN;
> +
> +       ret = !strcmp_cpuid_str(test_id, cpuid);
> +
> +       free(cpuid);
> +       return ret;
> +}
> diff --git a/tools/perf/util/expr.h b/tools/perf/util/expr.h
> index 3c1e49b3e35d..c0cec29ddc29 100644
> --- a/tools/perf/util/expr.h
> +++ b/tools/perf/util/expr.h
> @@ -55,5 +55,6 @@ double expr_id_data__value(const struct expr_id_data *data);
>  double expr_id_data__source_count(const struct expr_id_data *data);
>  double expr__get_literal(const char *literal, const struct expr_scanner_ctx *ctx);
>  double expr__has_event(const struct expr_parse_ctx *ctx, bool compute_ids, const char *id);
> +double expr__strcmp_cpuid_str(const struct expr_parse_ctx *ctx, bool compute_ids, const char *id);
>
>  #endif
> diff --git a/tools/perf/util/expr.l b/tools/perf/util/expr.l
> index dbb117414710..0feef0726c48 100644
> --- a/tools/perf/util/expr.l
> +++ b/tools/perf/util/expr.l
> @@ -114,6 +114,7 @@ if          { return IF; }
>  else           { return ELSE; }
>  source_count   { return SOURCE_COUNT; }
>  has_event      { return HAS_EVENT; }
> +strcmp_cpuid_str       { return STRCMP_CPUID_STR; }

For the sake of coverage, it'd be nice to have a test case like:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/tests/expr.c?h=tmp.perf-tools-next#n257

>  {literal}      { return literal(yyscanner, sctx); }
>  {number}       { return value(yyscanner); }
>  {symbol}       { return str(yyscanner, ID, sctx->runtime); }
> diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> index 65d54a6f29ad..6c93b358cc2d 100644
> --- a/tools/perf/util/expr.y
> +++ b/tools/perf/util/expr.y
> @@ -39,7 +39,7 @@ int expr_lex(YYSTYPE * yylval_param , void *yyscanner);
>         } ids;
>  }
>
> -%token ID NUMBER MIN MAX IF ELSE LITERAL D_RATIO SOURCE_COUNT HAS_EVENT EXPR_ERROR
> +%token ID NUMBER MIN MAX IF ELSE LITERAL D_RATIO SOURCE_COUNT HAS_EVENT STRCMP_CPUID_STR EXPR_ERROR
>  %left MIN MAX IF
>  %left '|'
>  %left '^'
> @@ -207,6 +207,12 @@ expr: NUMBER
>         $$.ids = NULL;
>         free($3);
>  }
> +| STRCMP_CPUID_STR '(' ID ')'
> +{
> +       $$.val = expr__strcmp_cpuid_str(ctx, compute_ids, $3);
> +       $$.ids = NULL;
> +       free($3);
> +}
>  | expr '|' expr
>  {
>         if (is_const($1.val) && is_const($3.val)) {
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index d5406effc169..a7f05e4dda97 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1790,3 +1790,20 @@ void perf_pmu__delete(struct perf_pmu *pmu)
>         zfree(&pmu->alias_name);
>         free(pmu);
>  }
> +
> +struct perf_pmu *pmu__find_core_pmu(void)

I think as this is scanning all PMUs it more logically belongs in
pmus.c than pmu.c.

> +{
> +       struct perf_pmu *pmu = NULL;
> +
> +       while ((pmu = perf_pmus__scan_core(pmu))) {
> +               /*
> +                * The cpumap should cover all CPUs. Otherwise, some CPUs may
> +                * not support some events or have different event IDs.
> +                */
> +               if (RC_CHK_ACCESS(pmu->cpus)->nr != cpu__max_cpu().cpu)

The RC_CHK_ACCESS can be avoided using perf_cpu_map__nr.

Not all CPUs need to be online. I think the number for the PMU is
bound by the number online while cpu__max_cpu gives the maximum
present. It is common to workaround SMT vulnerabilities by making
cores offline in which case this would never be true. I think the
following is nearly equivalent and simpler:

perf_pmus__num_core_pmus > 1 ? NULL : perf_pmus__scan_core(NULL);

Thanks,
Ian

> +                       return NULL;
> +
> +               return pmu;
> +       }
> +       return NULL;
> +}
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 6b414cecbad2..7ff925224165 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -289,5 +289,6 @@ int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename,
>  struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char *lookup_name);
>  struct perf_pmu *perf_pmu__create_placeholder_core_pmu(struct list_head *core_pmus);
>  void perf_pmu__delete(struct perf_pmu *pmu);
> +struct perf_pmu *pmu__find_core_pmu(void);
>
>  #endif /* __PMU_H */
> --
> 2.34.1
>

Powered by blists - more mailing lists