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 DA8F9C54EBD for ; Thu, 12 Jan 2023 14:31:13 +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=z9zqmdUQzNuZzgXDKbEVVRMZxgbqx1O0RbilpOjAh4Q=; b=n5xA2uBRTbr/Mr 7sufjzoGVYlzak4bpMja9rEadFXkEbJNgLWi40Jf3k6Iiba0lcMn8nXGk3su1MQoU9IsxXnqk20Bv 6okxGA6QF/7ALDCVlGjLRZAKKrU79h2RYTBfB2m7G4qTAI0ZcXLWuhrI8cbv4cE14BTPFwwO4XHW4 F6TZWRUCx+r1uAzQnFdVurgAr9KaIXLe53Fi1e+X0O80Xg2ggJkvpj3mg6Ep+A1L71eyo4rX2I8YO Gd8V5YDzd99IPJuty66vMLKrR99jmH7/ffMcvoAb79/j2DsQ+aqdT7xaIDEQxBOva0EcbTQQ8I6g1 VDsPho09fMY2kovSD67Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pFyai-00FN3y-Og; Thu, 12 Jan 2023 14:30:04 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pFyab-00FN10-71 for linux-arm-kernel@lists.infradead.org; Thu, 12 Jan 2023 14:29:59 +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 3DC0913D5; Thu, 12 Jan 2023 06:30:35 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.43.206]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2DAEE3F67D; Thu, 12 Jan 2023 06:29:52 -0800 (PST) Date: Thu, 12 Jan 2023 14:29:49 +0000 From: Mark Rutland To: Anshuman Khandual Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon Subject: Re: [PATCH V7 5/6] arm64/perf: Add branch stack support in ARMV8 PMU Message-ID: References: <20230105031039.207972-1-anshuman.khandual@arm.com> <20230105031039.207972-6-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230105031039.207972-6-anshuman.khandual@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230112_062957_372369_E7EC9147 X-CRM114-Status: GOOD ( 28.88 ) 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 Thu, Jan 05, 2023 at 08:40:38AM +0530, Anshuman Khandual wrote: > This enables support for branch stack sampling event in ARMV8 PMU, checking > has_branch_stack() on the event inside 'struct arm_pmu' callbacks. Although > these branch stack helpers armv8pmu_branch_XXXXX() are just dummy functions > for now. While here, this also defines arm_pmu's sched_task() callback with > armv8pmu_sched_task(), which resets the branch record buffer on a sched_in. > > Cc: Catalin Marinas > Cc: Will Deacon > Cc: Mark Rutland > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual > --- > arch/arm64/include/asm/perf_event.h | 10 +++++++++ > arch/arm64/kernel/perf_event.c | 35 +++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/arch/arm64/include/asm/perf_event.h b/arch/arm64/include/asm/perf_event.h > index 3eaf462f5752..a038902d6874 100644 > --- a/arch/arm64/include/asm/perf_event.h > +++ b/arch/arm64/include/asm/perf_event.h > @@ -273,4 +273,14 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs); > (regs)->pstate = PSR_MODE_EL1h; \ > } > > +struct pmu_hw_events; > +struct arm_pmu; > +struct perf_event; > + > +static inline void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event) { } > +static inline bool armv8pmu_branch_valid(struct perf_event *event) { return false; } > +static inline void armv8pmu_branch_enable(struct perf_event *event) { } > +static inline void armv8pmu_branch_disable(struct perf_event *event) { } > +static inline void armv8pmu_branch_probe(struct arm_pmu *arm_pmu) { } > +static inline void armv8pmu_branch_reset(void) { } As far as I can tell, these are not supposed to be called when !has_branch_stack(), so it would be good if these had a WARN() or similar to spot buggy usage. > #endif > diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c > index a5193f2146a6..8805b4516088 100644 > --- a/arch/arm64/kernel/perf_event.c > +++ b/arch/arm64/kernel/perf_event.c > @@ -789,10 +789,22 @@ static void armv8pmu_enable_event(struct perf_event *event) > * Enable counter > */ > armv8pmu_enable_event_counter(event); > + > + /* > + * Enable BRBE > + */ > + if (has_branch_stack(event)) > + armv8pmu_branch_enable(event); > } This looks fine, but tbh I think we should delete the existing comments above each function call as they're blindingly obvious and just waste space. > static void armv8pmu_disable_event(struct perf_event *event) > { > + /* > + * Disable BRBE > + */ > + if (has_branch_stack(event)) > + armv8pmu_branch_disable(event); > + Likewise here. > /* > * Disable counter > */ > @@ -878,6 +890,13 @@ static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu) > if (!armpmu_event_set_period(event)) > continue; > > + if (has_branch_stack(event)) { > + WARN_ON(!cpuc->branches); > + armv8pmu_branch_read(cpuc, event); > + data.br_stack = &cpuc->branches->branch_stack; > + data.sample_flags |= PERF_SAMPLE_BRANCH_STACK; > + } How do we ensure the data we're getting isn't changed under our feet? Is BRBE disabled at this point? Is this going to have branches after taking the exception, or does BRBE stop automatically at that point? If so we presumably need to take special care as to when we read this relative to enabling/disabling and/or manipulating the overflow bits. > + > /* > * Perf event overflow will queue the processing of the event as > * an irq_work which will be taken care of in the handling of > @@ -976,6 +995,14 @@ static int armv8pmu_user_event_idx(struct perf_event *event) > return event->hw.idx; > } > > +static void armv8pmu_sched_task(struct perf_event_pmu_context *pmu_ctx, bool sched_in) > +{ > + struct arm_pmu *armpmu = to_arm_pmu(pmu_ctx->pmu); > + > + if (sched_in && arm_pmu_branch_stack_supported(armpmu)) > + armv8pmu_branch_reset(); > +} When scheduling out, shouldn't we save what we have so far? It seems odd that we just throw that away rather than placing it into a FIFO. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel