From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A39E1C4167B for ; Mon, 4 Dec 2023 09:46:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=7x00xrJDlBUjOARxEcih6Z4YY7zyoGNTs+g8ToKF1rY=; b=z23Gtq5/GyFEVU 9d2p5xIrkM7eHbr6Xe6IMtO488kXon4MGYx2N+VXTj5XXj5z9torwe306QizzbQ0NZWVmUAJHZTV7 ff3zpZoljLgkAfnewlwyqWirX4T591F25xzKoXFDdyJhbA4vpwNsb8rnxaXyzmt68i98szY39AgHU U0CTeLT3BoFYfm1l3Gobb9p5jxx6PuxTjaiDUI2LdwuDgPKP+417cq1OYHoyzCDxi9RmEVNygxJ3+ Fpyfy6RUA+IYUQJkkUVHUNkCHuL4Rfh3nI5WcY3eaeIUIeLQv57acdR3FpZSxjI7ExMIu4k9JEkge JNPvjedUFdnIzNwQjvfg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rA5WM-003RbK-2r; Mon, 04 Dec 2023 09:45:46 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rA5W6-003RZs-0y for linux-arm-kernel@lists.infradead.org; Mon, 04 Dec 2023 09:45:44 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E018EFEC; Mon, 4 Dec 2023 01:46:14 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.44.129]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8F6443F6C4; Mon, 4 Dec 2023 01:45:26 -0800 (PST) Date: Mon, 4 Dec 2023 09:45:21 +0000 From: Mark Rutland To: Anshuman Khandual Cc: linux-arm-kernel@lists.infradead.org, Will Deacon , Russell King , linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] arm: perf: Remove PMU locking Message-ID: References: <20231115092805.737822-1-anshuman.khandual@arm.com> <20231115092805.737822-2-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20231115092805.737822-2-anshuman.khandual@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231204_014531_216748_912ADAD2 X-CRM114-Status: GOOD ( 32.63 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Nov 15, 2023 at 02:58:04PM +0530, Anshuman Khandual wrote: > 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 handler. > 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 spinlock. > > This is very similar to an earlier change on arm64 platform. > > commit 2a0e2a02e4b7 ("arm64: perf: Remove PMU locking"). I realise the commit message is a copy of the wording from 2a0e2a02e4b7, but some of this isn't quite right; could we please replace that with: | Currently the 32-bit arm PMU drivers use the pmu_hw_events::lock spinlock in | their arm_pmu::{start,stop,enable,disable}() callbacks to protect hardware | state and event data. | | This locking is not necessary as the perf core code already provides mutual | exclusion, disabling interrupts to serialize against the IRQ handler, and | using perf_event_context::lock to protect against concurrent modifications of | events cross-cpu. | | The locking was removed from the arm64 (now PMUv3) PMU driver in commit: | | 2a0e2a02e4b7 ("arm64: perf: Remove PMU locking") | | ... and the same reasoning applies to all the 32-bit PMU drivers. | | Remove the locking from the 32-bit PMU drivers. With that wording: Acked-by: Mark Rutland Mark. > Cc: Mark Rutland > Cc: Will Deacon > Cc: Russell King > Cc: linux-perf-users@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-arm-kernel@lists.infradead.org > Signed-off-by: Anshuman Khandual > --- > arch/arm/kernel/perf_event_v6.c | 28 ++++-------------- > arch/arm/kernel/perf_event_v7.c | 44 ----------------------------- > arch/arm/kernel/perf_event_xscale.c | 44 ++++++----------------------- > 3 files changed, 13 insertions(+), 103 deletions(-) > > diff --git a/arch/arm/kernel/perf_event_v6.c b/arch/arm/kernel/perf_event_v6.c > index 1ae99deeec54..8fc080c9e4fb 100644 > --- a/arch/arm/kernel/perf_event_v6.c > +++ b/arch/arm/kernel/perf_event_v6.c > @@ -268,10 +268,8 @@ static inline void armv6pmu_write_counter(struct perf_event *event, u64 value) > > static void armv6pmu_enable_event(struct perf_event *event) > { > - unsigned long val, mask, evt, flags; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long val, mask, evt; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > if (ARMV6_CYCLE_COUNTER == idx) { > @@ -294,12 +292,10 @@ static void armv6pmu_enable_event(struct perf_event *event) > * Mask out the current event and set the counter to count the event > * that we're interested in. > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = armv6_pmcr_read(); > val &= ~mask; > val |= evt; > armv6_pmcr_write(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static irqreturn_t > @@ -362,26 +358,20 @@ armv6pmu_handle_irq(struct arm_pmu *cpu_pmu) > > static void armv6pmu_start(struct arm_pmu *cpu_pmu) > { > - unsigned long flags, val; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > + unsigned long val; > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = armv6_pmcr_read(); > val |= ARMV6_PMCR_ENABLE; > armv6_pmcr_write(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void armv6pmu_stop(struct arm_pmu *cpu_pmu) > { > - unsigned long flags, val; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > + unsigned long val; > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = armv6_pmcr_read(); > val &= ~ARMV6_PMCR_ENABLE; > armv6_pmcr_write(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static int > @@ -419,10 +409,8 @@ static void armv6pmu_clear_event_idx(struct pmu_hw_events *cpuc, > > static void armv6pmu_disable_event(struct perf_event *event) > { > - unsigned long val, mask, evt, flags; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long val, mask, evt; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > if (ARMV6_CYCLE_COUNTER == idx) { > @@ -444,20 +432,16 @@ static void armv6pmu_disable_event(struct perf_event *event) > * of ETM bus signal assertion cycles. The external reporting should > * be disabled and so this should never increment. > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = armv6_pmcr_read(); > val &= ~mask; > val |= evt; > armv6_pmcr_write(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void armv6mpcore_pmu_disable_event(struct perf_event *event) > { > - unsigned long val, mask, flags, evt = 0; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long val, mask, evt = 0; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > if (ARMV6_CYCLE_COUNTER == idx) { > @@ -475,12 +459,10 @@ static void armv6mpcore_pmu_disable_event(struct perf_event *event) > * Unlike UP ARMv6, we don't have a way of stopping the counters. We > * simply disable the interrupt reporting. > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = armv6_pmcr_read(); > val &= ~mask; > val |= evt; > armv6_pmcr_write(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static int armv6_map_event(struct perf_event *event) > diff --git a/arch/arm/kernel/perf_event_v7.c b/arch/arm/kernel/perf_event_v7.c > index eb2190477da1..c890354b04e9 100644 > --- a/arch/arm/kernel/perf_event_v7.c > +++ b/arch/arm/kernel/perf_event_v7.c > @@ -870,10 +870,8 @@ static void armv7_pmnc_dump_regs(struct arm_pmu *cpu_pmu) > > static void armv7pmu_enable_event(struct perf_event *event) > { > - unsigned long flags; > struct hw_perf_event *hwc = &event->hw; > struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > if (!armv7_pmnc_counter_valid(cpu_pmu, idx)) { > @@ -886,7 +884,6 @@ static void armv7pmu_enable_event(struct perf_event *event) > * Enable counter and interrupt, and set the counter to count > * the event that we're interested in. > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > > /* > * Disable counter > @@ -910,16 +907,12 @@ static void armv7pmu_enable_event(struct perf_event *event) > * Enable counter > */ > armv7_pmnc_enable_counter(idx); > - > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void armv7pmu_disable_event(struct perf_event *event) > { > - unsigned long flags; > struct hw_perf_event *hwc = &event->hw; > struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > if (!armv7_pmnc_counter_valid(cpu_pmu, idx)) { > @@ -931,7 +924,6 @@ static void armv7pmu_disable_event(struct perf_event *event) > /* > * Disable counter and interrupt > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > > /* > * Disable counter > @@ -942,8 +934,6 @@ static void armv7pmu_disable_event(struct perf_event *event) > * Disable interrupt for this counter > */ > armv7_pmnc_disable_intens(idx); > - > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static irqreturn_t armv7pmu_handle_irq(struct arm_pmu *cpu_pmu) > @@ -1009,24 +999,14 @@ static irqreturn_t armv7pmu_handle_irq(struct arm_pmu *cpu_pmu) > > static void armv7pmu_start(struct arm_pmu *cpu_pmu) > { > - unsigned long flags; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > - > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > /* Enable all counters */ > armv7_pmnc_write(armv7_pmnc_read() | ARMV7_PMNC_E); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void armv7pmu_stop(struct arm_pmu *cpu_pmu) > { > - unsigned long flags; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > - > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > /* Disable all counters */ > armv7_pmnc_write(armv7_pmnc_read() & ~ARMV7_PMNC_E); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static int armv7pmu_get_event_idx(struct pmu_hw_events *cpuc, > @@ -1492,14 +1472,10 @@ static void krait_clearpmu(u32 config_base) > > static void krait_pmu_disable_event(struct perf_event *event) > { > - unsigned long flags; > struct hw_perf_event *hwc = &event->hw; > int idx = hwc->idx; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > > /* Disable counter and interrupt */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > > /* Disable counter */ > armv7_pmnc_disable_counter(idx); > @@ -1512,23 +1488,17 @@ static void krait_pmu_disable_event(struct perf_event *event) > > /* Disable interrupt for this counter */ > armv7_pmnc_disable_intens(idx); > - > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void krait_pmu_enable_event(struct perf_event *event) > { > - unsigned long flags; > struct hw_perf_event *hwc = &event->hw; > int idx = hwc->idx; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > > /* > * Enable counter and interrupt, and set the counter to count > * the event that we're interested in. > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > > /* Disable counter */ > armv7_pmnc_disable_counter(idx); > @@ -1548,8 +1518,6 @@ static void krait_pmu_enable_event(struct perf_event *event) > > /* Enable counter */ > armv7_pmnc_enable_counter(idx); > - > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void krait_pmu_reset(void *info) > @@ -1825,14 +1793,10 @@ static void scorpion_clearpmu(u32 config_base) > > static void scorpion_pmu_disable_event(struct perf_event *event) > { > - unsigned long flags; > struct hw_perf_event *hwc = &event->hw; > int idx = hwc->idx; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > > /* Disable counter and interrupt */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > > /* Disable counter */ > armv7_pmnc_disable_counter(idx); > @@ -1845,23 +1809,17 @@ static void scorpion_pmu_disable_event(struct perf_event *event) > > /* Disable interrupt for this counter */ > armv7_pmnc_disable_intens(idx); > - > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void scorpion_pmu_enable_event(struct perf_event *event) > { > - unsigned long flags; > struct hw_perf_event *hwc = &event->hw; > int idx = hwc->idx; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > > /* > * Enable counter and interrupt, and set the counter to count > * the event that we're interested in. > */ > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > > /* Disable counter */ > armv7_pmnc_disable_counter(idx); > @@ -1881,8 +1839,6 @@ static void scorpion_pmu_enable_event(struct perf_event *event) > > /* Enable counter */ > armv7_pmnc_enable_counter(idx); > - > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void scorpion_pmu_reset(void *info) > diff --git a/arch/arm/kernel/perf_event_xscale.c b/arch/arm/kernel/perf_event_xscale.c > index f6cdcacfb96d..7a2ba1c689a7 100644 > --- a/arch/arm/kernel/perf_event_xscale.c > +++ b/arch/arm/kernel/perf_event_xscale.c > @@ -203,10 +203,8 @@ xscale1pmu_handle_irq(struct arm_pmu *cpu_pmu) > > static void xscale1pmu_enable_event(struct perf_event *event) > { > - unsigned long val, mask, evt, flags; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long val, mask, evt; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > switch (idx) { > @@ -229,20 +227,16 @@ static void xscale1pmu_enable_event(struct perf_event *event) > return; > } > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = xscale1pmu_read_pmnc(); > val &= ~mask; > val |= evt; > xscale1pmu_write_pmnc(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void xscale1pmu_disable_event(struct perf_event *event) > { > - unsigned long val, mask, evt, flags; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long val, mask, evt; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > switch (idx) { > @@ -263,12 +257,10 @@ static void xscale1pmu_disable_event(struct perf_event *event) > return; > } > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = xscale1pmu_read_pmnc(); > val &= ~mask; > val |= evt; > xscale1pmu_write_pmnc(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static int > @@ -300,26 +292,20 @@ static void xscalepmu_clear_event_idx(struct pmu_hw_events *cpuc, > > static void xscale1pmu_start(struct arm_pmu *cpu_pmu) > { > - unsigned long flags, val; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > + unsigned long val; > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = xscale1pmu_read_pmnc(); > val |= XSCALE_PMU_ENABLE; > xscale1pmu_write_pmnc(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void xscale1pmu_stop(struct arm_pmu *cpu_pmu) > { > - unsigned long flags, val; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > + unsigned long val; > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = xscale1pmu_read_pmnc(); > val &= ~XSCALE_PMU_ENABLE; > xscale1pmu_write_pmnc(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static inline u64 xscale1pmu_read_counter(struct perf_event *event) > @@ -549,10 +535,8 @@ xscale2pmu_handle_irq(struct arm_pmu *cpu_pmu) > > static void xscale2pmu_enable_event(struct perf_event *event) > { > - unsigned long flags, ien, evtsel; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long ien, evtsel; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > ien = xscale2pmu_read_int_enable(); > @@ -587,18 +571,14 @@ static void xscale2pmu_enable_event(struct perf_event *event) > return; > } > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > xscale2pmu_write_event_select(evtsel); > xscale2pmu_write_int_enable(ien); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void xscale2pmu_disable_event(struct perf_event *event) > { > - unsigned long flags, ien, evtsel, of_flags; > - struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu); > + unsigned long ien, evtsel, of_flags; > struct hw_perf_event *hwc = &event->hw; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > int idx = hwc->idx; > > ien = xscale2pmu_read_int_enable(); > @@ -638,11 +618,9 @@ static void xscale2pmu_disable_event(struct perf_event *event) > return; > } > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > xscale2pmu_write_event_select(evtsel); > xscale2pmu_write_int_enable(ien); > xscale2pmu_write_overflow_flags(of_flags); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static int > @@ -663,26 +641,20 @@ xscale2pmu_get_event_idx(struct pmu_hw_events *cpuc, > > static void xscale2pmu_start(struct arm_pmu *cpu_pmu) > { > - unsigned long flags, val; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > + unsigned long val; > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = xscale2pmu_read_pmnc() & ~XSCALE_PMU_CNT64; > val |= XSCALE_PMU_ENABLE; > xscale2pmu_write_pmnc(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static void xscale2pmu_stop(struct arm_pmu *cpu_pmu) > { > - unsigned long flags, val; > - struct pmu_hw_events *events = this_cpu_ptr(cpu_pmu->hw_events); > + unsigned long val; > > - raw_spin_lock_irqsave(&events->pmu_lock, flags); > val = xscale2pmu_read_pmnc(); > val &= ~XSCALE_PMU_ENABLE; > xscale2pmu_write_pmnc(val); > - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); > } > > static inline u64 xscale2pmu_read_counter(struct perf_event *event) > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel