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 X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81E8FC32750 for ; Fri, 2 Aug 2019 14:26:20 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 519F520679 for ; Fri, 2 Aug 2019 14:26:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Hi2q3rKu" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 519F520679 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tEebtskqu3WqwD3vXinZMepwkbL33ySolxGWC+Wd1B4=; b=Hi2q3rKu/vx+Lq l5l2k9biYXuH5SMOxsMnRMUpFZkPk1VD9xxOP/BtTgbNVMjSO7JAyaRhjW1zYOMlVqxnwFGW6gL9H ubiRSeQCu6HluxwvSkpmDCKfWDsTCl91Vz0RUoRBWL//b3bGFrotcwx5G+nQTZR4tDzaxctfXZxSB IsqQ2bh6GvmXKeXs4clc82NsFHx716rAbQRUV4GPYg2esKa5+FZnl+SiWQAH2u4bMxI8vPMLMfKhG 0/emCTgFvEDCniGGRkmlQ4Ks3KjHWoMN6l6dMWP/cuiBL/ft7fAqjgy8k7e6haUQauRvDRtDlfeO7 VYuKClqwKsyY7qKil5/g==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1htYVf-0000UX-B8; Fri, 02 Aug 2019 14:26:19 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1htYVb-0000Tw-Og for linux-arm-kernel@lists.infradead.org; Fri, 02 Aug 2019 14:26:17 +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 720651570; Fri, 2 Aug 2019 07:26:13 -0700 (PDT) Received: from [10.1.197.45] (e112298-lin.cambridge.arm.com [10.1.197.45]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D9B173F575; Fri, 2 Aug 2019 07:26:11 -0700 (PDT) Subject: Re: [PATCH v4 2/9] arm64: perf: Remove PMU locking To: Will Deacon References: <1563351432-55652-1-git-send-email-julien.thierry@arm.com> <1563351432-55652-3-git-send-email-julien.thierry@arm.com> <20190801125821.23wt657bfs2k536f@willie-the-truck> From: Julien Thierry Message-ID: Date: Fri, 2 Aug 2019 15:26:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <20190801125821.23wt657bfs2k536f@willie-the-truck> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190802_072615_888151_83965197 X-CRM114-Status: GOOD ( 20.15 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: mark.rutland@arm.com, peterz@infradead.org, Catalin Marinas , will.deacon@arm.com, acme@kernel.org, alexander.shishkin@linux.intel.com, mingo@redhat.com, namhyung@kernel.org, sthotton@marvell.com, jolsa@redhat.com, linux-arm-kernel@lists.infradead.org, liwei391@huawei.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Will, On 01/08/2019 13:58, Will Deacon wrote: > On Wed, Jul 17, 2019 at 09:17:05AM +0100, Julien Thierry wrote: >> Since the PMU driver uses direct registers for counter >> setup/manipulation, locking around these operations is no longer needed. >> >> For operations that can be called with interrupts enabled, preemption >> still needs to be disabled to ensure the programming of the PMU is >> done on the expected CPU and not migrated mid-programming. >> >> Signed-off-by: Julien Thierry >> Cc: Will Deacon >> Cc: Mark Rutland >> Cc: Peter Zijlstra >> Cc: Ingo Molnar >> Cc: Arnaldo Carvalho de Melo >> Cc: Alexander Shishkin >> Cc: Jiri Olsa >> Cc: Namhyung Kim >> Cc: Catalin Marinas >> --- >> arch/arm64/kernel/perf_event.c | 30 ++---------------------------- >> 1 file changed, 2 insertions(+), 28 deletions(-) >> >> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c >> index 838758f..0e2cf5d 100644 >> --- a/arch/arm64/kernel/perf_event.c >> +++ b/arch/arm64/kernel/perf_event.c >> @@ -673,15 +673,10 @@ static inline u32 armv8pmu_getreset_flags(void) >> >> static void armv8pmu_enable_event(struct perf_event *event) >> { >> - unsigned long flags; >> - 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 >> @@ -702,21 +697,10 @@ static void armv8pmu_enable_event(struct perf_event *event) >> * Enable counter >> */ >> armv8pmu_enable_event_counter(event); >> - >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); >> } > > With the implicit ISBs now removed by virtue of addressing the counter > register directly, what prevents the programming of the evtype being > reordered with respect to disabling/enabling the counter? I agree, it seems an ISB is missing here. It should probably be fixed in the previous patch. However, I notice that even before that patch, there is no ISB between the enabling of the IRQ for the counter and the enabling of the counter itself. Meaning we might start counting events before the IRQ is enabled. Should we have something like the following? armv8pmu_disable_event_counter(event); isb(); armv8pmu_write_event_type(event); armv8pmu_enable_event_irq(event); isb(); armv8pmu_enable_event_counter(event); > >> static void armv8pmu_disable_event(struct perf_event *event) >> { >> - unsigned long flags; >> - 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 >> */ >> @@ -726,30 +710,20 @@ static void armv8pmu_disable_event(struct perf_event *event) >> * Disable interrupt for this counter >> */ >> armv8pmu_disable_event_irq(event); >> - >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); >> } >> >> static void armv8pmu_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); >> + WARN_ON_ONCE(preemptible()); >> /* Enable all counters */ >> armv8pmu_pmcr_write(armv8pmu_pmcr_read() | ARMV8_PMU_PMCR_E); >> - raw_spin_unlock_irqrestore(&events->pmu_lock, flags); >> } >> >> static void armv8pmu_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); >> + WARN_ON_ONCE(preemptible()); > > I don't think we need these WARN_ONs -- these are very much per-CPU > operations from the context of the perf core, so we'll be ok. > If they are not necessary we can get rid of them. Thanks, -- Julien Thierry _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel