Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <20231123042922.834425-1-irogers@google.com> <86cyw0zeiu.wl-maz@kernel.org>
 <CAP-5=fXjX2pNmmX3WOY=m0BqUHTR2YPKVki6bbgG3g1Btc2=Ng@mail.gmail.com> <86bkbkzc2p.wl-maz@kernel.org>
In-Reply-To: <86bkbkzc2p.wl-maz@kernel.org>
From:   Ian Rogers <irogers@google.com>
Date:   Thu, 23 Nov 2023 09:59:59 -0800
Message-ID: <CAP-5=fUU4ZRLUrg7h+ua0f9mJBONfTd2JuCRPaXq2CApk5-_fA@mail.gmail.com>
Subject: Re: [RFC PATCH v1] perf parse-events: Make legacy events lower
 priority than sysfs/json
To:     Marc Zyngier <maz@kernel.org>
Cc:     Mark Rutland <mark.rutland@arm.com>,
        Hector Martin <marcan@marcan.st>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        James Clark <james.clark@arm.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 Thu, Nov 23, 2023 at 8:09=E2=80=AFAM Marc Zyngier <maz@kernel.org> wrote=
:
>
> On Thu, 23 Nov 2023 15:27:54 +0000,
> Ian Rogers <irogers@google.com> wrote:
> >
> > On Thu, Nov 23, 2023 at 7:16=E2=80=AFAM Marc Zyngier <maz@kernel.org> w=
rote:
> > >
> > > Again, perf gets shipped in distros, and not necessary as the latest
> > > version. Rather, they tend to ship the version matching the kernel. N=
o
> > > backport, buggy perf.
> >
> > Please complain to the distros. I complained to Debian, we got rid of
> > the horrible wrapper script thing they did. I complained to two
> > separate Ubuntu people over the last two weeks as they still have
> > broken packaging even though they derive from Debian. Fedora is of
> > course perfect as Arnaldo oversees it :-)
>
> In this instance, I don't need to complain to anyone but you. And
> guess what: it is on Fedora that this issue was first discovered.
>
> I also don't see what distro packaging policy has anything to do with
> the issue at hand, but that's beside the point.

Because the latest perf tool is always improved and carries fixes,
just as say gcc or clang. We don't ask these tools to backport fixes
and then deliberately run out-of-date versions of them.

> >
> > > And again, I don't see a bug in the PMU driver.
> >
> > Whether the PMU driver is requested a legacy cycles event or the
> > cycles event as an event code, the PMU driver should support it.
> > Supporting legacy events is just something core PMU drivers do. This
> > workaround wouldn't be necessary were it not for this PMU bug.
>
> Again, *which* PMU bug? What is a legacy event, and when has this
> terminology made it into the kernel? Who has decided that a change was
> necessary? Why haven't you submitted patches upgrading all the PMU
> drivers to support whatever you are referring to?

I did fix ARM's PMU driver for extended types, James Clark took over
the patch. The term legacy has at least been in use in kernel source
code for over 11 years:
http://lkml.kernel.org/r/1337584373-2741-4-git-send-email-jolsa@redhat.com

An issue I face in fixing somebody's PMU driver is it is ever so
useful to be able to test. The work done with James was done blind by
me except for checking for regressions on a raspberry pi 4, which
isn't heterogeneous (nor is the 5 *sigh*). The fact there were bugs in
ARM's PMU driver for so long shows a lack of testing by ARM and we've
been going out of our way to increase testing. Something positive ARM
could do in this area is to update the parse-events test, yes the one
that is supposed to test issues like this, so that the hardcoded "cpu"
PMU assumption that works on most platforms who name their core PMU
"cpu" also works on ARM. For bonus points setting up testing so that
we know when things break would be useful. As mentioned in previous
emails I hope to work away from needing an actual machine to test the
perf tool's correctness, but we're a long way from that. There are
very many BIG.little Android devices in the field where the PMUs are
not set up as heterogeneous, ARM could contribute a CTS test to
Android to make sure this doesn't happen.

Thanks,
Ian

> > This change impacts every user of perf not just a partial fix to
> > workaround ARM PMU driver issues, see the updated parse-events test
> > for a list of what a simple test sees as a behavior change.
>
> When making far-reaching changes to a subsystem, I apply two rules:
>
> - I address everything that is affected, not just my pet architecture
>
> - I don't break other people's toys, which means compatibility is a
>   *must*, not a 'nice to have'
>
> By this standard, your complaining that "ARM is broken" doesn't hold.
> It was working just fine until your changes rendered perf unusable.
>
> Nonetheless, thank you for addressing it quickly. This is sincerely
> appreciated.
>
>         M.
>
> --
> Without deviation from the norm, progress is not possible.