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 A01E4C433FE for ; Fri, 4 Feb 2022 04:57:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1357008AbiBDE5D (ORCPT ); Thu, 3 Feb 2022 23:57:03 -0500 Received: from foss.arm.com ([217.140.110.172]:42680 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235508AbiBDE5C (ORCPT ); Thu, 3 Feb 2022 23:57:02 -0500 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 203EE1435; Thu, 3 Feb 2022 20:57:02 -0800 (PST) Received: from [10.163.45.195] (unknown [10.163.45.195]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B5BA53F774; Thu, 3 Feb 2022 20:56:57 -0800 (PST) From: Anshuman Khandual Subject: Re: [PATCH 1/2] perf: Add more generic branch types To: Mark Rutland Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, robh@kernel.org, Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Will Deacon References: <1643348653-24367-1-git-send-email-anshuman.khandual@arm.com> <1643348653-24367-2-git-send-email-anshuman.khandual@arm.com> Message-ID: Date: Fri, 4 Feb 2022 10:26:54 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2/2/22 5:28 PM, Mark Rutland wrote: > Hi Anshuman, > > On Fri, Jan 28, 2022 at 11:14:12AM +0530, Anshuman Khandual wrote: >> This expands generic branch type classification by adding some more entries >> , that can still be represented with the existing 4 bit 'type' field. While ^ >> here this also updates the x86 implementation with these new branch types. > > It looks like there's some whitespace damage here. Are you referring the above ? I will have a look. > >>>From a quick scan of the below, I think the "exception return" and "IRQ > exception" types are somewhat generic, and could be added now (aside from any > bikeshedding over names), but: > > * For IRQ vs FIQ, we might just want to have a top-level "asynchronous > exception" type, and then further divide that with a separate field. That way > it's easier to extend in future if new exceptions are added. Okay. But that might lead to a hierarchical bit fields design where as the current one is just linear. > > * I don't think the debug state entry/exits make sense as generic branch types, >From BRBE perspective, a branch is any control flow change including exception level change, debug enter, debug exit etc. If exception and its return can be classified as 'branches' why not debug state change ? Are there no similar debug states transition on other platforms ? > since those are somewhat specific to the ARM architecutre, and it might make > sense to define generic PERF_BR_ARCH* definitions instead. Makes sense but corresponding bit field layout change in branch_sample_type will remain a challenge. > > * Given the next patch extends the field, and therei are potential ABI problems > with that, we might need to reserve a value for ABI extensibility purposes, > and I suspect we need to do that *first*. More comments on the subsequent > patch. Sure, understood. > >> Cc: Peter Zijlstra >> Cc: Ingo Molnar >> Cc: Arnaldo Carvalho de Melo >> Cc: Mark Rutland >> Cc: Alexander Shishkin >> Cc: Jiri Olsa >> Cc: Namhyung Kim >> Cc: Will Deacon >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-perf-users@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual >> --- >> arch/x86/events/intel/lbr.c | 4 ++-- >> include/uapi/linux/perf_event.h | 5 +++++ >> tools/include/uapi/linux/perf_event.h | 5 +++++ >> tools/perf/util/branch.c | 7 ++++++- >> 4 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c >> index 8043213b75a5..9f86fac8c6a5 100644 >> --- a/arch/x86/events/intel/lbr.c >> +++ b/arch/x86/events/intel/lbr.c >> @@ -1336,10 +1336,10 @@ static int branch_map[X86_BR_TYPE_MAP_MAX] = { >> PERF_BR_SYSCALL, /* X86_BR_SYSCALL */ >> PERF_BR_SYSRET, /* X86_BR_SYSRET */ >> PERF_BR_UNKNOWN, /* X86_BR_INT */ >> - PERF_BR_UNKNOWN, /* X86_BR_IRET */ >> + PERF_BR_EXPT_RET, /* X86_BR_IRET */ >> PERF_BR_COND, /* X86_BR_JCC */ >> PERF_BR_UNCOND, /* X86_BR_JMP */ >> - PERF_BR_UNKNOWN, /* X86_BR_IRQ */ >> + PERF_BR_IRQ, /* X86_BR_IRQ */ >> PERF_BR_IND_CALL, /* X86_BR_IND_CALL */ >> PERF_BR_UNKNOWN, /* X86_BR_ABORT */ >> PERF_BR_UNKNOWN, /* X86_BR_IN_TX */ > > This presumably changes the values reported to userspace, so the commit message > should mention that and explain why that is not a problem. Okay. > >> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h >> index 1b65042ab1db..b91d0f575d0c 100644 >> --- a/include/uapi/linux/perf_event.h >> +++ b/include/uapi/linux/perf_event.h >> @@ -251,6 +251,11 @@ enum { >> PERF_BR_SYSRET = 8, /* syscall return */ >> PERF_BR_COND_CALL = 9, /* conditional function call */ >> PERF_BR_COND_RET = 10, /* conditional function return */ >> + PERF_BR_EXPT_RET = 11, /* exception return */ > > We don't use 'EXPT' anywhere else, so it might be better to just use 'ERET'. > IIUC that's the naming the x86 FRED stuff is going to use anyhow. Sure, will change. > >> + PERF_BR_IRQ = 12, /* irq */ > > This looks somewhat generic, so adding it now makes sense to me, but ... > >> + PERF_BR_FIQ = 13, /* fiq */ > > ... this is arguably just a idfferent class of interrupt from the PoV of Linux, > and the naming is ARM-specific, so I don't think this makes sense to add *now*. I assume 'now' --> without ABI extension. > As above, maybe it would be better to have a generic "aynchronous exception" or > "interrupt" type, and a separate field to distinguish specific types of those. > >> + PERF_BR_DEBUG_HALT = 14, /* debug halt */ >> + PERF_BR_DEBUG_EXIT = 15, /* debug exit */ > > For the benefit of those not familiar with the ARM architecture, "debug halt" > and "debug exit" usually refer to "debug state", which is what an external > (e.g. JTAG) debugger uses rather than the usual self-hosted debug stuff that > Linux uses. > > Given that, I'm not sure these are very generic, and I suspect it would be > better to have more generic PERF_BR_ARCH_* entries for things like this. Sure, will try and come up with something similar. > > Thanks, > Mark. > >> PERF_BR_MAX, >> }; >> >> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h >> index 4cd39aaccbe7..1882054e8684 100644 >> --- a/tools/include/uapi/linux/perf_event.h >> +++ b/tools/include/uapi/linux/perf_event.h >> @@ -251,6 +251,11 @@ enum { >> PERF_BR_SYSRET = 8, /* syscall return */ >> PERF_BR_COND_CALL = 9, /* conditional function call */ >> PERF_BR_COND_RET = 10, /* conditional function return */ >> + PERF_BR_EXPT_RET = 11, /* exception return */ >> + PERF_BR_IRQ = 12, /* irq */ >> + PERF_BR_FIQ = 13, /* fiq */ >> + PERF_BR_DEBUG_HALT = 14, /* debug halt */ >> + PERF_BR_DEBUG_EXIT = 15, /* debug exit */ >> PERF_BR_MAX, >> }; >> >> diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c >> index 2285b1eb3128..74e5e67b1779 100644 >> --- a/tools/perf/util/branch.c >> +++ b/tools/perf/util/branch.c >> @@ -49,7 +49,12 @@ const char *branch_type_name(int type) >> "SYSCALL", >> "SYSRET", >> "COND_CALL", >> - "COND_RET" >> + "COND_RET", >> + "EXPT_RET", >> + "IRQ", >> + "FIQ", >> + "DEBUG_HALT", >> + "DEBUG_EXIT" >> }; >> >> if (type >= 0 && type < PERF_BR_MAX) >> -- >> 2.25.1 >> 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 0E679C433EF for ; Fri, 4 Feb 2022 04:58: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:Date: Message-ID:References:Cc:To:Subject:From:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Owner; bh=gE+vImJtS4HRzJCHwogVJLLU/sLj/ZYY9RRhIyckR1Q=; b=tXUD9nHB+Wuj1T/6iqz06KwJzS 4MctV8zzBAbdFXB+c6fovMgWiqg1cVTP0FgEK0Czvza7Ex5+fusAFw67XjXhKAtK53NFaqHEyXy7I moL1VOnBVCX0DovJkrci4+tTH2O5ztgutXw8LE2nm48AddcnXSo2bDd0zzSMnt9B77PHAhsdrDQH7 MqLzJgRNHSwLAAYODMjdSvge7qWhe2WUChaWUGXaOSLpPgtVkNG3tZJQd2GPP5Y5aeBIUyN9VGgpM RL3/X/VQAzLNQxZHGWUSDalpSSYUY+wXvJ4xUZim9vr4R9RNvDIRrKNZlQjwyuwZfmTniBacsiBUd t7B6LraA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nFqeh-003UG8-Ac; Fri, 04 Feb 2022 04:57:07 +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 1nFqed-003UE4-Db for linux-arm-kernel@lists.infradead.org; Fri, 04 Feb 2022 04:57:05 +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 203EE1435; Thu, 3 Feb 2022 20:57:02 -0800 (PST) Received: from [10.163.45.195] (unknown [10.163.45.195]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B5BA53F774; Thu, 3 Feb 2022 20:56:57 -0800 (PST) From: Anshuman Khandual Subject: Re: [PATCH 1/2] perf: Add more generic branch types To: Mark Rutland Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, robh@kernel.org, Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Alexander Shishkin , Jiri Olsa , Namhyung Kim , Will Deacon References: <1643348653-24367-1-git-send-email-anshuman.khandual@arm.com> <1643348653-24367-2-git-send-email-anshuman.khandual@arm.com> Message-ID: Date: Fri, 4 Feb 2022 10:26:54 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220203_205703_613039_5E699223 X-CRM114-Status: GOOD ( 44.15 ) 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 2/2/22 5:28 PM, Mark Rutland wrote: > Hi Anshuman, > > On Fri, Jan 28, 2022 at 11:14:12AM +0530, Anshuman Khandual wrote: >> This expands generic branch type classification by adding some more entries >> , that can still be represented with the existing 4 bit 'type' field. While ^ >> here this also updates the x86 implementation with these new branch types. > > It looks like there's some whitespace damage here. Are you referring the above ? I will have a look. > >>>From a quick scan of the below, I think the "exception return" and "IRQ > exception" types are somewhat generic, and could be added now (aside from any > bikeshedding over names), but: > > * For IRQ vs FIQ, we might just want to have a top-level "asynchronous > exception" type, and then further divide that with a separate field. That way > it's easier to extend in future if new exceptions are added. Okay. But that might lead to a hierarchical bit fields design where as the current one is just linear. > > * I don't think the debug state entry/exits make sense as generic branch types, >From BRBE perspective, a branch is any control flow change including exception level change, debug enter, debug exit etc. If exception and its return can be classified as 'branches' why not debug state change ? Are there no similar debug states transition on other platforms ? > since those are somewhat specific to the ARM architecutre, and it might make > sense to define generic PERF_BR_ARCH* definitions instead. Makes sense but corresponding bit field layout change in branch_sample_type will remain a challenge. > > * Given the next patch extends the field, and therei are potential ABI problems > with that, we might need to reserve a value for ABI extensibility purposes, > and I suspect we need to do that *first*. More comments on the subsequent > patch. Sure, understood. > >> Cc: Peter Zijlstra >> Cc: Ingo Molnar >> Cc: Arnaldo Carvalho de Melo >> Cc: Mark Rutland >> Cc: Alexander Shishkin >> Cc: Jiri Olsa >> Cc: Namhyung Kim >> Cc: Will Deacon >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-perf-users@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual >> --- >> arch/x86/events/intel/lbr.c | 4 ++-- >> include/uapi/linux/perf_event.h | 5 +++++ >> tools/include/uapi/linux/perf_event.h | 5 +++++ >> tools/perf/util/branch.c | 7 ++++++- >> 4 files changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c >> index 8043213b75a5..9f86fac8c6a5 100644 >> --- a/arch/x86/events/intel/lbr.c >> +++ b/arch/x86/events/intel/lbr.c >> @@ -1336,10 +1336,10 @@ static int branch_map[X86_BR_TYPE_MAP_MAX] = { >> PERF_BR_SYSCALL, /* X86_BR_SYSCALL */ >> PERF_BR_SYSRET, /* X86_BR_SYSRET */ >> PERF_BR_UNKNOWN, /* X86_BR_INT */ >> - PERF_BR_UNKNOWN, /* X86_BR_IRET */ >> + PERF_BR_EXPT_RET, /* X86_BR_IRET */ >> PERF_BR_COND, /* X86_BR_JCC */ >> PERF_BR_UNCOND, /* X86_BR_JMP */ >> - PERF_BR_UNKNOWN, /* X86_BR_IRQ */ >> + PERF_BR_IRQ, /* X86_BR_IRQ */ >> PERF_BR_IND_CALL, /* X86_BR_IND_CALL */ >> PERF_BR_UNKNOWN, /* X86_BR_ABORT */ >> PERF_BR_UNKNOWN, /* X86_BR_IN_TX */ > > This presumably changes the values reported to userspace, so the commit message > should mention that and explain why that is not a problem. Okay. > >> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h >> index 1b65042ab1db..b91d0f575d0c 100644 >> --- a/include/uapi/linux/perf_event.h >> +++ b/include/uapi/linux/perf_event.h >> @@ -251,6 +251,11 @@ enum { >> PERF_BR_SYSRET = 8, /* syscall return */ >> PERF_BR_COND_CALL = 9, /* conditional function call */ >> PERF_BR_COND_RET = 10, /* conditional function return */ >> + PERF_BR_EXPT_RET = 11, /* exception return */ > > We don't use 'EXPT' anywhere else, so it might be better to just use 'ERET'. > IIUC that's the naming the x86 FRED stuff is going to use anyhow. Sure, will change. > >> + PERF_BR_IRQ = 12, /* irq */ > > This looks somewhat generic, so adding it now makes sense to me, but ... > >> + PERF_BR_FIQ = 13, /* fiq */ > > ... this is arguably just a idfferent class of interrupt from the PoV of Linux, > and the naming is ARM-specific, so I don't think this makes sense to add *now*. I assume 'now' --> without ABI extension. > As above, maybe it would be better to have a generic "aynchronous exception" or > "interrupt" type, and a separate field to distinguish specific types of those. > >> + PERF_BR_DEBUG_HALT = 14, /* debug halt */ >> + PERF_BR_DEBUG_EXIT = 15, /* debug exit */ > > For the benefit of those not familiar with the ARM architecture, "debug halt" > and "debug exit" usually refer to "debug state", which is what an external > (e.g. JTAG) debugger uses rather than the usual self-hosted debug stuff that > Linux uses. > > Given that, I'm not sure these are very generic, and I suspect it would be > better to have more generic PERF_BR_ARCH_* entries for things like this. Sure, will try and come up with something similar. > > Thanks, > Mark. > >> PERF_BR_MAX, >> }; >> >> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h >> index 4cd39aaccbe7..1882054e8684 100644 >> --- a/tools/include/uapi/linux/perf_event.h >> +++ b/tools/include/uapi/linux/perf_event.h >> @@ -251,6 +251,11 @@ enum { >> PERF_BR_SYSRET = 8, /* syscall return */ >> PERF_BR_COND_CALL = 9, /* conditional function call */ >> PERF_BR_COND_RET = 10, /* conditional function return */ >> + PERF_BR_EXPT_RET = 11, /* exception return */ >> + PERF_BR_IRQ = 12, /* irq */ >> + PERF_BR_FIQ = 13, /* fiq */ >> + PERF_BR_DEBUG_HALT = 14, /* debug halt */ >> + PERF_BR_DEBUG_EXIT = 15, /* debug exit */ >> PERF_BR_MAX, >> }; >> >> diff --git a/tools/perf/util/branch.c b/tools/perf/util/branch.c >> index 2285b1eb3128..74e5e67b1779 100644 >> --- a/tools/perf/util/branch.c >> +++ b/tools/perf/util/branch.c >> @@ -49,7 +49,12 @@ const char *branch_type_name(int type) >> "SYSCALL", >> "SYSRET", >> "COND_CALL", >> - "COND_RET" >> + "COND_RET", >> + "EXPT_RET", >> + "IRQ", >> + "FIQ", >> + "DEBUG_HALT", >> + "DEBUG_EXIT" >> }; >> >> if (type >= 0 && type < PERF_BR_MAX) >> -- >> 2.25.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel