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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id E8BFFC7EE25 for ; Mon, 12 Jun 2023 08:36:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233037AbjFLIgT (ORCPT ); Mon, 12 Jun 2023 04:36:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42814 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232365AbjFLIgO (ORCPT ); Mon, 12 Jun 2023 04:36:14 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id BCD87107; Mon, 12 Jun 2023 01:36:12 -0700 (PDT) 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 B449E1FB; Mon, 12 Jun 2023 01:36:57 -0700 (PDT) Received: from [192.168.0.146] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 70B143F663; Mon, 12 Jun 2023 01:36:00 -0700 (PDT) Message-ID: Date: Mon, 12 Jun 2023 14:05:51 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH V11 06/10] arm64/perf: Enable branch stack events via FEAT_BRBE To: Mark Rutland 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 References: <20230531040428.501523-1-anshuman.khandual@arm.com> <20230531040428.501523-7-anshuman.khandual@arm.com> Content-Language: en-US From: Anshuman Khandual In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 6/9/23 18:45, Mark Rutland wrote: > On Fri, Jun 09, 2023 at 01:47:18PM +0100, Mark Rutland wrote: >> On Fri, Jun 09, 2023 at 10:52:37AM +0530, Anshuman Khandual wrote: >>> On 6/5/23 19:13, Mark Rutland wrote: >>>> Looking at I see: >>>> >>>> | /* >>>> | * branch stack layout: >>>> | * nr: number of taken branches stored in entries[] >>>> | * hw_idx: The low level index of raw branch records >>>> | * for the most recent branch. >>>> | * -1ULL means invalid/unknown. >>>> | * >>>> | * Note that nr can vary from sample to sample >>>> | * branches (to, from) are stored from most recent >>>> | * to least recent, i.e., entries[0] contains the most >>>> | * recent branch. >>>> | * The entries[] is an abstraction of raw branch records, >>>> | * which may not be stored in age order in HW, e.g. Intel LBR. >>>> | * The hw_idx is to expose the low level index of raw >>>> | * branch record for the most recent branch aka entries[0]. >>>> | * The hw_idx index is between -1 (unknown) and max depth, >>>> | * which can be retrieved in /sys/devices/cpu/caps/branches. >>>> | * For the architectures whose raw branch records are >>>> | * already stored in age order, the hw_idx should be 0. >>>> | */ >>>> | struct perf_branch_stack { >>>> | __u64 nr; >>>> | __u64 hw_idx; >>>> | struct perf_branch_entry entries[]; >>>> | }; >>>> >>>> ... which seems to indicate we should be setting hw_idx to 0, since IIUC our >>>> records are in age order. >>> Branch records are indeed in age order, sure will change hw_idx as 0. Earlier >>> figured that there was no need for hw_idx and hence marked it as -1UL similar >>> to other platforms like powerpc. >> >> That's fair enough; looking at power_pmu_bhrb_read() in >> arch/powerpc/perf/core-book3s.c, I see a comment: >> >> Branches are read most recent first (ie. mfbhrb 0 is >> the most recent branch). >> >> ... which suggests that should be 0 also, or that the documentation is wrong. >> >> Do you know how the perf tool consumes this? > > > Thinking about this some more, if what this is saying is that if entries[0] > must be strictly the last branch, and we've lost branches due to interrupt > latency, then we clearly don't meet that requirement and must report -1ULL > here. 'last branch' means relative to the captured records not in absolute terms. Loosing records for interrupt latency too does not change the relative age order for the set. Hence '0' might suggest valid relative not absolute age order for the branch records set. > > So while it'd be nice to figure this out, I'm happy using -1ULL, and would be a > bit concerned using 0. Sounds reasonable. If tools are not going to use this anyway, I guess there is no point in suggesting that each record set has got valid age order with subtle conditions involved.