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 3B381C433EF for ; Wed, 2 Feb 2022 11:58:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344045AbiBBL6K (ORCPT ); Wed, 2 Feb 2022 06:58:10 -0500 Received: from foss.arm.com ([217.140.110.172]:54042 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232566AbiBBL6J (ORCPT ); Wed, 2 Feb 2022 06:58:09 -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 50FF71FB; Wed, 2 Feb 2022 03:58:09 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.87.240]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 75BD93F718; Wed, 2 Feb 2022 03:58:07 -0800 (PST) Date: Wed, 2 Feb 2022 11:58:04 +0000 From: Mark Rutland To: Anshuman Khandual 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 Subject: Re: [PATCH 1/2] perf: Add more generic branch types Message-ID: References: <1643348653-24367-1-git-send-email-anshuman.khandual@arm.com> <1643348653-24367-2-git-send-email-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1643348653-24367-2-git-send-email-anshuman.khandual@arm.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. >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. * I don't think the debug state entry/exits make sense as generic branch types, since those are somewhat specific to the ARM architecutre, and it might make sense to define generic PERF_BR_ARCH* definitions instead. * 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. > 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. > 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. > + 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*. 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. 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 6FE9EC433EF for ; Wed, 2 Feb 2022 11:59:25 +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=4NsicpqnbtBpf0rlymvE4xzYPaAYHTLq3HNkU4ueypY=; b=lpQHi/LdojGBXb KwbUY6W4FAyuJW1ZUeBX/P29S62pdLq5M40AYTj502bGYrwyEhRtOLWtQZMDdFRa+EvCF9FGOzypp HuMfRnd8S0W/RZrWdOgvTJO56F7dnFKougS8hfizZLjmerVjztNAmvnX1qddfAru9e6E0RujIGMhW 4bQwORfxrk7nmcWsUpWpUwRpaiqjmwJvsd5726ny73cn0RSmT9qk4J7AZWstR74MEt39jTFKU8+3G NumORhLSTPA3OaKOe0tWMNnb6AMWB/0uRLZiquV6hoEzcDTT90shDw/+o/+w6cJDU/KzMGDPQaIBK 7xRvfMqvyA/JcdFX71vg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nFEHA-00F6qS-3f; Wed, 02 Feb 2022 11:58:16 +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 1nFEH4-00F6oZ-LO for linux-arm-kernel@lists.infradead.org; Wed, 02 Feb 2022 11:58:13 +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 50FF71FB; Wed, 2 Feb 2022 03:58:09 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.87.240]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 75BD93F718; Wed, 2 Feb 2022 03:58:07 -0800 (PST) Date: Wed, 2 Feb 2022 11:58:04 +0000 From: Mark Rutland To: Anshuman Khandual 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 Subject: Re: [PATCH 1/2] perf: Add more generic branch types Message-ID: References: <1643348653-24367-1-git-send-email-anshuman.khandual@arm.com> <1643348653-24367-2-git-send-email-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1643348653-24367-2-git-send-email-anshuman.khandual@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220202_035810_842426_EF002E88 X-CRM114-Status: GOOD ( 37.03 ) 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 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. >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. * I don't think the debug state entry/exits make sense as generic branch types, since those are somewhat specific to the ARM architecutre, and it might make sense to define generic PERF_BR_ARCH* definitions instead. * 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. > 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. > 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. > + 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*. 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. 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