From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Ian Rogers <irogers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
James Clark <james.clark@arm.com>,
Andrii Nakryiko <andrii@kernel.org>,
Eduard Zingerman <eddyz87@gmail.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
Ahmad Yasin <ahmad.yasin@intel.com>,
Stephane Eranian <eranian@google.com>,
Andi Kleen <ak@linux.intel.com>,
Perry Taylor <perry.taylor@intel.com>,
Samantha Alt <samantha.alt@intel.com>,
Caleb Biggers <caleb.biggers@intel.com>,
Weilin Wang <weilin.wang@intel.com>,
Edward Baker <edward.baker@intel.com>,
"Liang, Kan" <kan.liang@linux.intel.com>
Subject: Re: [PATCH v1 1/2] perf expr: Make the evaluation of & and | logical and lazy
Date: Mon, 5 Jun 2023 16:30:51 -0300 [thread overview]
Message-ID: <ZH436w4Sesq57iu9@kernel.org> (raw)
In-Reply-To: <CAP-5=fX_j-iwuHBekCDFzQUGaOqigTXtyUa6npNCuR7ktF-3_A@mail.gmail.com>
Em Mon, Jun 05, 2023 at 10:32:18AM -0700, Ian Rogers escreveu:
> On Thu, May 18, 2023 at 12:47 PM Liang, Kan <kan.liang@linux.intel.com> wrote:
> >
> >
> >
> > On 2023-05-04 3:58 p.m., Ian Rogers wrote:
> > > Currently the & and | operators are only used in metric thresholds
> > > like (from the tma_retiring metric):
> > > tma_retiring > 0.7 | tma_heavy_operations > 0.1
> > >
> > > Thresholds are always computed when present, but a lack events may
> > > mean the threshold can't be computed. This happens with the option
> > > --metric-no-threshold for say the metric tma_retiring on Tigerlake
> > > model CPUs. To fully compute the threshold tma_heavy_operations is
> > > needed and it needs the extra events of IDQ.MS_UOPS,
> > > UOPS_DECODED.DEC0, cpu/UOPS_DECODED.DEC0,cmask=1/ and
> > > IDQ.MITE_UOPS. So --metric-no-threshold is a useful option to reduce
> > > the number of events needed and potentially multiplexing of events.
> > >
> > > Rather than just fail threshold computations like this, we may know a
> > > result from just the left or right-hand side. So, for tma_retiring if
> > > its value is "> 0.7" we know it is over the threshold. This allows the
> > > metric to have the threshold coloring, when possible, without all the
> > > counters being programmed.
> > >
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> >
> > The patch works well on my machine.
> >
> > Tested-by: Kan Liang <kan.liang@linux.intel.com>
> >
> > Thanks,
> > Kan
>
> Arnaldo, could we take this set?
Thanks, applied.
- Arnaldo
> Thanks,
> Ian
>
> > > tools/perf/tests/expr.c | 40 +++++++++++++++++++
> > > tools/perf/util/expr.y | 86 +++++++++++++++++++++++++++++++++--------
> > > 2 files changed, 109 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/tools/perf/tests/expr.c b/tools/perf/tests/expr.c
> > > index cbf0e0c74906..45c7fedb797a 100644
> > > --- a/tools/perf/tests/expr.c
> > > +++ b/tools/perf/tests/expr.c
> > > @@ -184,6 +184,46 @@ static int test__expr(struct test_suite *t __maybe_unused, int subtest __maybe_u
> > > NULL, ctx) == 0);
> > > TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0);
> > >
> > > + /* The expression is a constant 0.0 without needing to evaluate EVENT1. */
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("0 & EVENT1 > 0", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0);
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("EVENT1 > 0 & 0", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0);
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("1 & EVENT1 > 0", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1);
> > > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1", &val_ptr));
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("EVENT1 > 0 & 1", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1);
> > > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1", &val_ptr));
> > > +
> > > + /* The expression is a constant 1.0 without needing to evaluate EVENT1. */
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("1 | EVENT1 > 0", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0);
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("EVENT1 > 0 | 1", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 0);
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("0 | EVENT1 > 0", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1);
> > > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1", &val_ptr));
> > > + expr__ctx_clear(ctx);
> > > + TEST_ASSERT_VAL("find ids",
> > > + expr__find_ids("EVENT1 > 0 | 0", NULL, ctx) == 0);
> > > + TEST_ASSERT_VAL("find ids", hashmap__size(ctx->ids) == 1);
> > > + TEST_ASSERT_VAL("find ids", hashmap__find(ctx->ids, "EVENT1", &val_ptr));
> > > +
> > > /* Test toplogy constants appear well ordered. */
> > > expr__ctx_clear(ctx);
> > > TEST_ASSERT_VAL("#num_cpus", expr__parse(&num_cpus, ctx, "#num_cpus") == 0);
> > > diff --git a/tools/perf/util/expr.y b/tools/perf/util/expr.y
> > > index 250e444bf032..6b110f9f95c9 100644
> > > --- a/tools/perf/util/expr.y
> > > +++ b/tools/perf/util/expr.y
> > > @@ -123,20 +123,6 @@ static struct ids handle_id(struct expr_parse_ctx *ctx, char *id,
> > > * constant value using OP. Its invariant that there are no ids. If computing
> > > * ids for non-constants union the set of IDs that must be computed.
> > > */
> > > -#define BINARY_LONG_OP(RESULT, OP, LHS, RHS) \
> > > - if (!compute_ids || (is_const(LHS.val) && is_const(RHS.val))) { \
> > > - assert(LHS.ids == NULL); \
> > > - assert(RHS.ids == NULL); \
> > > - if (isnan(LHS.val) || isnan(RHS.val)) { \
> > > - RESULT.val = NAN; \
> > > - } else { \
> > > - RESULT.val = (long)LHS.val OP (long)RHS.val; \
> > > - } \
> > > - RESULT.ids = NULL; \
> > > - } else { \
> > > - RESULT = union_expr(LHS, RHS); \
> > > - }
> > > -
> > > #define BINARY_OP(RESULT, OP, LHS, RHS) \
> > > if (!compute_ids || (is_const(LHS.val) && is_const(RHS.val))) { \
> > > assert(LHS.ids == NULL); \
> > > @@ -213,9 +199,75 @@ expr: NUMBER
> > > }
> > > | ID { $$ = handle_id(ctx, $1, compute_ids, /*source_count=*/false); }
> > > | SOURCE_COUNT '(' ID ')' { $$ = handle_id(ctx, $3, compute_ids, /*source_count=*/true); }
> > > -| expr '|' expr { BINARY_LONG_OP($$, |, $1, $3); }
> > > -| expr '&' expr { BINARY_LONG_OP($$, &, $1, $3); }
> > > -| expr '^' expr { BINARY_LONG_OP($$, ^, $1, $3); }
> > > +| expr '|' expr
> > > +{
> > > + if (is_const($1.val) && is_const($3.val)) {
> > > + assert($1.ids == NULL);
> > > + assert($3.ids == NULL);
> > > + $$.ids = NULL;
> > > + $$.val = (fpclassify($1.val) == FP_ZERO && fpclassify($3.val) == FP_ZERO) ? 0 : 1;
> > > + } else if (is_const($1.val)) {
> > > + assert($1.ids == NULL);
> > > + if (fpclassify($1.val) == FP_ZERO) {
> > > + $$ = $3;
> > > + } else {
> > > + $$.val = 1;
> > > + $$.ids = NULL;
> > > + ids__free($3.ids);
> > > + }
> > > + } else if (is_const($3.val)) {
> > > + assert($3.ids == NULL);
> > > + if (fpclassify($3.val) == FP_ZERO) {
> > > + $$ = $1;
> > > + } else {
> > > + $$.val = 1;
> > > + $$.ids = NULL;
> > > + ids__free($1.ids);
> > > + }
> > > + } else {
> > > + $$ = union_expr($1, $3);
> > > + }
> > > +}
> > > +| expr '&' expr
> > > +{
> > > + if (is_const($1.val) && is_const($3.val)) {
> > > + assert($1.ids == NULL);
> > > + assert($3.ids == NULL);
> > > + $$.val = (fpclassify($1.val) != FP_ZERO && fpclassify($3.val) != FP_ZERO) ? 1 : 0;
> > > + $$.ids = NULL;
> > > + } else if (is_const($1.val)) {
> > > + assert($1.ids == NULL);
> > > + if (fpclassify($1.val) != FP_ZERO) {
> > > + $$ = $3;
> > > + } else {
> > > + $$.val = 0;
> > > + $$.ids = NULL;
> > > + ids__free($3.ids);
> > > + }
> > > + } else if (is_const($3.val)) {
> > > + assert($3.ids == NULL);
> > > + if (fpclassify($3.val) != FP_ZERO) {
> > > + $$ = $1;
> > > + } else {
> > > + $$.val = 0;
> > > + $$.ids = NULL;
> > > + ids__free($1.ids);
> > > + }
> > > + } else {
> > > + $$ = union_expr($1, $3);
> > > + }
> > > +}
> > > +| expr '^' expr
> > > +{
> > > + if (is_const($1.val) && is_const($3.val)) {
> > > + assert($1.ids == NULL);
> > > + assert($3.ids == NULL);
> > > + $$.val = (fpclassify($1.val) == FP_ZERO) != (fpclassify($3.val) == FP_ZERO) ? 1 : 0;
> > > + $$.ids = NULL;
> > > + } else {
> > > + $$ = union_expr($1, $3);
> > > + }
> > > +}
> > > | expr '<' expr { BINARY_OP($$, <, $1, $3); }
> > > | expr '>' expr { BINARY_OP($$, >, $1, $3); }
> > > | expr '+' expr { BINARY_OP($$, +, $1, $3); }
--
- Arnaldo
prev parent reply other threads:[~2023-06-05 19:30 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-04 19:58 [PATCH v1 1/2] perf expr: Make the evaluation of & and | logical and lazy Ian Rogers
2023-05-04 19:58 ` [PATCH v1 2/2] perf stat: Document --metric-no-threshold and threshold colors Ian Rogers
2023-05-18 19:52 ` Liang, Kan
2023-05-15 16:03 ` [PATCH v1 1/2] perf expr: Make the evaluation of & and | logical and lazy Ian Rogers
2023-05-16 7:40 ` Jiri Olsa
2023-05-18 19:47 ` Liang, Kan
2023-06-05 17:32 ` Ian Rogers
2023-06-05 19:30 ` Arnaldo Carvalho de Melo [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZH436w4Sesq57iu9@kernel.org \
--to=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ahmad.yasin@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=andrii@kernel.org \
--cc=caleb.biggers@intel.com \
--cc=eddyz87@gmail.com \
--cc=edward.baker@intel.com \
--cc=eranian@google.com \
--cc=irogers@google.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=perry.taylor@intel.com \
--cc=peterz@infradead.org \
--cc=samantha.alt@intel.com \
--cc=weilin.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.