| Message ID | 1643348653-24367-2-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf: Expand captured branch types | expand |
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 <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Arnaldo Carvalho de Melo <acme@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Will Deacon <will@kernel.org> > 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 <anshuman.khandual@arm.com> > --- > 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 >
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 <peterz@infradead.org> >> Cc: Ingo Molnar <mingo@redhat.com> >> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> >> Cc: Jiri Olsa <jolsa@redhat.com> >> Cc: Namhyung Kim <namhyung@kernel.org> >> Cc: Will Deacon <will@kernel.org> >> 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 <anshuman.khandual@arm.com> >> --- >> 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 >>
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. > >>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: Hi Mark, I have sent a patch adding just PERF_BR_ERET and PERF_BR_IRQ which are generic enough to be included now. The 'type' field still got 3 more places for future use. Hence we should try and include two more branch types, before adding the last entry for ABI expansion (e.g PERF_BR_EXTENDED). https://lore.kernel.org/all/1645681014-3346-1-git-send-email-anshuman.khandual@arm.com/ - Anshuman
On 2/24/22 11:21 AM, Anshuman Khandual wrote: > > > 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. >> >> >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: > > Hi Mark, > > I have sent a patch adding just PERF_BR_ERET and PERF_BR_IRQ which are generic > enough to be included now. The 'type' field still got 3 more places for future > use. Hence we should try and include two more branch types, before adding the The two additional generic branch types could be - PERF_BR_SERROR (possible arm64 equivalent for x86 MCE) - PERF_BR_NO_TX (only missing TX related branch type in perf_branch_entry) perf_branch_entry.[in_tx | abort] are already available. Also PERF_BR_NO_TX could be used right away on x86 platform, via X86_BR_NO_TX. > last entry for ABI expansion (e.g PERF_BR_EXTENDED). PERF_BR_EXTENDED could help expand into another 4 bits 'new_type' field following the existing 4 bits 'type' field. Does this sound feasible ? enum { PERF_BR_UNKNOWN = 0, /* unknown */ PERF_BR_COND = 1, /* conditional */ PERF_BR_UNCOND = 2, /* unconditional */ PERF_BR_IND = 3, /* indirect */ PERF_BR_CALL = 4, /* function call */ PERF_BR_IND_CALL = 5, /* indirect function call */ PERF_BR_RET = 6, /* function return */ PERF_BR_SYSCALL = 7, /* syscall */ PERF_BR_SYSRET = 8, /* syscall return */ PERF_BR_COND_CALL = 9, /* conditional function call */ PERF_BR_COND_RET = 10, /* conditional function return */ PERF_BR_ERET = 11, /* exception return */ PERF_BR_IRQ = 12, /* irq */ PERF_BR_SERROR = 13, /* System error */ PERF_BR_NO_TX = 14, /* No transaction */ PERF_BR_EXTEND_ABI = 15 /* Extended ABI */ PERF_BR_MAX, }; enum { PERF_BR_NEW_FAULT_ALGN = 0, /* Alignment fault */ PERF_BR_NEW_FAULT_DATA = 1, /* Data fault */ PERF_BR_NEW_FAULT_INST = 2, /* Inst fault */ PERF_BR_NEW_ARCH_1 = 3, /* Architecture specific */ PERF_BR_NEW_ARCH_2 = 4, /* Architecture specific */ PERF_BR_NEW_ARCH_3 = 5, /* Architecture specific */ PERF_BR_NEW_ARCH_4 = 6, /* Architecture specific */ PERF_BR_NEW_ARCH_5 = 7, /* Architecture specific */ PERF_BR_NEW_MAX, }; #ifdef CONFIG_ARM64 #define PERF_BR_FIQ PERF_BR_NEW_ARCH_1 #define PERF_BR_DEBUG_HALT PERF_BR_NEW_ARCH_2 #define PERF_BR_DEBUG_EXIT PERF_BR_NEW_ARCH_3 #define PERF_BR_DEBUG_INST PERF_BR_NEW_ARCH_4 #define PERF_BR_DEBUG_DATA PERF_BR_NEW_ARCH_5 #endif User space perf tool will look into perf_branch_entry.new_type, only when (perf_branch_entry.type == PERF_BR_EXTEND_ABI). Older perf tool on a newer kernel will be safe via old PERF_BR_MAX, and will ignore [PERF_BR_ERET - PERF_BR_EXTEND_ABI] values possibly with an warning. - Anshuman
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 */ 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 */ + 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/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)
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. Cc: Peter Zijlstra <peterz@infradead.org> Cc: Ingo Molnar <mingo@redhat.com> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Will Deacon <will@kernel.org> 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 <anshuman.khandual@arm.com> --- 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(-)