Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <bc265120-0d48-bcab-a58e-3b94f8c540ce@arm.com>
References: <20200617113851.607706-1-alexandru.elisei@arm.com> <20200617113851.607706-4-alexandru.elisei@arm.com> <159242503203.62212.1690942414916053920@swboyd.mtv.corp.google.com> <bc265120-0d48-bcab-a58e-3b94f8c540ce@arm.com>
Subject: Re: [PATCH v5 3/7] arm64: perf: Remove PMU locking
From:   Stephen Boyd <swboyd@chromium.org>
Cc:     mark.rutland@arm.com, will@kernel.org,
        Julien Thierry <julien.thierry@arm.com>,
        Peter Zijlstra <peterz@infradead.org>, maz@kernel.org,
        Will Deacon <will.deacon@arm.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Ingo Molnar <mingo@redhat.com>, catalin.marinas@arm.com,
        Namhyung Kim <namhyung@kernel.org>,
        Jiri Olsa <jolsa@redhat.com>
To:     Alexandru Elisei <alexandru.elisei@arm.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Date:   Fri, 19 Jun 2020 01:29:59 -0700
Message-ID: <159255539947.62212.6059916295459835174@swboyd.mtv.corp.google.com>
User-Agent: alot/0.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Alexandru Elisei (2020-06-18 03:51:31)
> Hi,
>=20
> On 6/17/20 9:17 PM, Stephen Boyd wrote:
> > Quoting Alexandru Elisei (2020-06-17 04:38:47)
> >> From: Julien Thierry <julien.thierry@arm.com>
> >>
> >> The PMU is disabled and enabled, and the counters are programmed from
> >> contexts where interrupts or preemption is disabled.
> >>
> >> The functions to toggle the PMU and to program the PMU counters access=
 the
> >> registers directly and don't access data modified by the interrupt han=
dler.
> >> That, and the fact that they're always called from non-preemptible
> >> contexts, means that we don't need to disable interrupts or use a spin=
lock.
> > Maybe we should add a lockdep assertion that the code isn't preemptible?
> > I.e. add a cant_sleep() call? Or is it more that we don't need locking
> > because we're just doing register accesses and don't need to protect
> > those accesses from each other?
>=20
> It's both. The spinlocks were there to protect the functions from being p=
reempted
> and possibly migrated to another CPU, and from being interrupted by the P=
MU irq
> handler.
>=20
> There was no data race with the interrupt handler, but before the previou=
s patch
> ("arm64: perf: Avoid PMXEV* indirection"), in order to read/write/program=
 a
> counter, one had to write the counter number to a counter selection regis=
ter, and
> then write/read the desired value from another register. This was done fr=
om both
> the armv8pmu_{enable,disable}_event() functions and the irq handler, and =
the
> spinlock was necessary. Now that we can access a counter using a single r=
egister
> access, there's no need to protect the functions from being interrupted b=
y the IRQ
> handler. As for armv8pmu_{start,stop}(), they consist of one register wri=
te, so
> it's also safe for the irq handler to interrupt them.
>=20
> For the preemption part of the locking. The armv8pmu_{enable,disable}_eve=
nt(),
> when called by the perf core code via the pmu->{start,stop,add,del} callb=
acks, are
> guaranteed to be called with IRQs and preemption disabled, as per the com=
ment in
> include/linux/perf_event.h. They are also called from the arm_pmu driver =
by the
> CPU PM notifiers, which should also be executed with interrupts disabled.=
 Should
> we check here that the top level code respects these guarantees?
>=20
> The armv8pmu_{start,stop}() functions are called from the irq handler, so=
 we're
> safe from preemption in this case. They are also called via
> pmu->pmu_{enable,disable} callbacks, and I didn't find an explicit contra=
ct
> regarding preemption in include/linux/perf_event.h. I've checked the othe=
r call
> sites, and I didn't find any instances where they are called with preempt=
ion
> enabled, which makes sense as we don't want to disable the PMU on a anoth=
er CPU by
> accident.

If they're all callbacks then it's overkill to add this. Presumably it
is better to enforce this wherever the callbacks are called from so as
to not litter the callee with random cant_sleep() calls. Probably best
to ignore my suggestion.

>=20
> I would be inclined to add cant_sleep() calls to armv8pmu_{start,stop}().=
 In the
> previous iteration, there were WARN_ONs in these functions, and Will said=
 [1] they
> can be removed because they are per-CPU operations. Will, what do you thi=
nk about
> adding the lockdep assertions?
>=20
> [1] https://www.spinics.net/lists/arm-kernel/msg745161.html
>=20

If I read it correctly Will is saying the same thing in that thread.