Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230429053506.1962559-1-irogers@google.com> <20230429053506.1962559-15-irogers@google.com>
 <e3a87977-02a1-eb34-593e-844adc6669b7@amd.com>
In-Reply-To: <e3a87977-02a1-eb34-593e-844adc6669b7@amd.com>
From:   Ian Rogers <irogers@google.com>
Date:   Tue, 2 May 2023 09:19:32 -0700
Message-ID: <CAP-5=fWY2SYdiogQoxEkAMAc8+_S5Mt3PgO1_=AhbKY3aicgYg@mail.gmail.com>
Subject: Re: [PATCH v3 14/46] perf test: Mask config then test
To:     Ravi Bangoria <ravi.bangoria@amd.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Kan Liang <kan.liang@linux.intel.com>,
        Ahmad Yasin <ahmad.yasin@intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.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>,
        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>,
        Florian Fischer <florian.fischer@muhq.space>,
        Rob Herring <robh@kernel.org>,
        Zhengjun Xing <zhengjun.xing@linux.intel.com>,
        John Garry <john.g.garry@oracle.com>,
        Kajol Jain <kjain@linux.ibm.com>,
        Sumanth Korikkar <sumanthk@linux.ibm.com>,
        Thomas Richter <tmricht@linux.ibm.com>,
        Tiezhu Yang <yangtiezhu@loongson.cn>,
        Leo Yan <leo.yan@linaro.org>,
        Yang Jihong <yangjihong1@huawei.com>,
        James Clark <james.clark@arm.com>,
        Suzuki Poulouse <suzuki.poulose@arm.com>,
        Kang Minchul <tegongkang@gmail.com>,
        Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, May 2, 2023 at 3:44=E2=80=AFAM Ravi Bangoria <ravi.bangoria@amd.com=
> wrote:
>
> > @@ -87,7 +92,7 @@ static int test__checkevent_raw(struct evlist *evlist=
)
> >
> >       TEST_ASSERT_VAL("wrong number of entries", 1 =3D=3D evlist->core.=
nr_entries);
> >       TEST_ASSERT_VAL("wrong type", PERF_TYPE_RAW =3D=3D evsel->core.at=
tr.type);
> > -     TEST_ASSERT_VAL("wrong config", 0x1a =3D=3D evsel->core.attr.conf=
ig);
> > +     TEST_ASSERT_VAL("wrong config", test_config(evsel, 0x1a));
>
> Extended 'type' via 'config' is applicable only to HW and HW_CACHE types.
> I think we should not apply test_config() for other types. Even if they
> are ineffective in these tests (because we are not setting upper bits),
> they are semantically wrong IMO.
>
> In fact, AMD EventSelect is 12 bits and upper 4 bits are located at 35:32
> in PerfEvtSel register. So for RAW events, upper bits MUST NOT be ignored=
.

Thanks Ravi, this is important and I'll update the tests as you
suggest. I'd been iterating on this, which is why the test is this
way. We have two parsing cases, one where the type is explicitly given
like:
$ perf stat -e '4:0xC0' true
and another where the raw config is wildcard applied like:
$ perf stat -e 'rC0' true
The question is which PMUs should the 0xC0 raw event be opened on? The
answer in the code is to ignore all PMUs that aren't the "core" PMUs,
so cpu, cpu_atom, cpu_core, armv8... This matches existing behavior
but it wasn't clear to me that this was intentional. I think in the
future the wildcard application can be made more efficient to ignore
unnecessary PMUs and avoid the sysfs scan. Something that is a bit of
an issue in the event parsing is what exactly should be applied by
wildcard matching and there are inconsistencies.

Thanks,
Ian

> > @@ -139,8 +142,7 @@ static int test__checkevent_symbolic_alias(struct e=
vlist *evlist)
> >
> >       TEST_ASSERT_VAL("wrong number of entries", 1 =3D=3D evlist->core.=
nr_entries);
> >       TEST_ASSERT_VAL("wrong type", PERF_TYPE_SOFTWARE =3D=3D evsel->co=
re.attr.type);
> > -     TEST_ASSERT_VAL("wrong config",
> > -                     PERF_COUNT_SW_PAGE_FAULTS =3D=3D evsel->core.attr=
.config);
> > +     TEST_ASSERT_VAL("wrong config", test_config(evsel, PERF_COUNT_SW_=
PAGE_FAULTS));
>
> Ditto for PERF_TYPE_SOFTWARE. We should not use test_config().
>
>
> > @@ -160,7 +162,7 @@ static int test__checkevent_breakpoint(struct evlis=
t *evlist)
> >
> >       TEST_ASSERT_VAL("wrong number of entries", 1 =3D=3D evlist->core.=
nr_entries);
> >       TEST_ASSERT_VAL("wrong type", PERF_TYPE_BREAKPOINT =3D=3D evsel->=
core.attr.type);
> > -     TEST_ASSERT_VAL("wrong config", 0 =3D=3D evsel->core.attr.config)=
;
> > +     TEST_ASSERT_VAL("wrong config", test_config(evsel, 0));
>
> Ditto for PERF_TYPE_BREAKPOINT.