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 11A84C77B73 for ; Mon, 5 Jun 2023 12:06:18 +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=A6ctHgNDcnnkrO8BsgnG1RlKqKvMJjBVlccw9gZ9tVc=; b=PBWYdPqTDTtMk/ ivhhgqc8rSZnbKF4H+w++fhVKcrS3tJafU62K9+lEOd4Bd83TUCL9vyfgL9o0rfENpQkHg38hwNvA EKT/oBGwJVeYHUWNtnNIJjh7MOYXnQnhYGOte9tRudukm7LELFCn+3weXw24TDOCnkw8tAuYyvc7o 6ncO6HghV1tePo5ACS9Uti6iJ4AR4cURFM9cvebby/fcbmt5MYn4faz2bl6JE9rfWwQk99CauAUE3 gaAMz3bSKNeer9O14Cxc0k46Uy7pzFkT+a2s3GuDGFBO8+1IIItWZv+R4c2IlBeHLfobCB5UcVQ71 ZcWK2AA0nYogjwc5gSIg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q68yA-00FNXf-16; Mon, 05 Jun 2023 12:05:54 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q68y7-00FNWq-17 for linux-arm-kernel@lists.infradead.org; Mon, 05 Jun 2023 12:05:52 +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 B9AA2D75; Mon, 5 Jun 2023 05:06:33 -0700 (PDT) Received: from FVFF77S0Q05N (unknown [10.57.25.215]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0275A3F663; Mon, 5 Jun 2023 05:05:45 -0700 (PDT) Date: Mon, 5 Jun 2023 13:05:36 +0100 From: Mark Rutland To: Anshuman Khandual Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, will@kernel.org, catalin.marinas@arm.com, Mark Brown , James Clark , Rob Herring , Marc Zyngier , Suzuki Poulose , Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , linux-perf-users@vger.kernel.org Subject: Re: [PATCH V11 05/10] arm64/perf: Add branch stack support in ARMV8 PMU Message-ID: References: <20230531040428.501523-1-anshuman.khandual@arm.com> <20230531040428.501523-6-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230531040428.501523-6-anshuman.khandual@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230605_050551_482726_F357CFEB X-CRM114-Status: GOOD ( 22.42 ) 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, May 31, 2023 at 09:34:23AM +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. This generally looks good, but I have a few comments below. [...] > +static inline bool armv8pmu_branch_valid(struct perf_event *event) > +{ > + WARN_ON_ONCE(!has_branch_stack(event)); > + return false; > +} IIUC this is for validating the attr, so could we please name this armv8pmu_branch_attr_valid() ? [...] > +static int branch_records_alloc(struct arm_pmu *armpmu) > +{ > + struct pmu_hw_events *events; > + int cpu; > + > + for_each_possible_cpu(cpu) { > + events = per_cpu_ptr(armpmu->hw_events, cpu); > + events->branches = kzalloc(sizeof(struct branch_records), GFP_KERNEL); > + if (!events->branches) > + return -ENOMEM; > + } > + return 0; This leaks memory if any allocation fails, and the next patch replaces this code entirely. Please add this once in a working state. Either use the percpu allocation trick in the next patch from the start, or have this kzalloc() with a corresponding kfree() in an error path. > } > > static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu) > @@ -1145,12 +1162,24 @@ static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu) > }; > int ret; > > + ret = armv8pmu_private_alloc(cpu_pmu); > + if (ret) > + return ret; > + > ret = smp_call_function_any(&cpu_pmu->supported_cpus, > __armv8pmu_probe_pmu, > &probe, 1); > if (ret) > return ret; > > + if (arm_pmu_branch_stack_supported(cpu_pmu)) { > + ret = branch_records_alloc(cpu_pmu); > + if (ret) > + return ret; > + } else { > + armv8pmu_private_free(cpu_pmu); > + } I see from the next patch that "private" is four ints, so please just add that to struct arm_pmu under an ifdef CONFIG_ARM64_BRBE. That'll simplify this, and if we end up needing more space in future we can consider factoring it out. > + > return probe.present ? 0 : -ENODEV; > } It also seems odd to ceck probe.present *after* checking arm_pmu_branch_stack_supported(). With the allocation removed I think this can be written more clearly as: | static int armv8pmu_probe_pmu(struct arm_pmu *cpu_pmu) | { | struct armv8pmu_probe_info probe = { | .pmu = cpu_pmu, | .present = false, | }; | int ret; | | ret = smp_call_function_any(&cpu_pmu->supported_cpus, | __armv8pmu_probe_pmu, | &probe, 1); | if (ret) | return ret; | | if (!probe.present) | return -ENODEV; | | if (arm_pmu_branch_stack_supported(cpu_pmu)) | ret = branch_records_alloc(cpu_pmu); | | return ret; | } Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel