Break the assignment logic for misc flags into their own respective
functions to reduce the complexity of the nested logic.
Signed-off-by: Colton Lewis <coltonlewis@google.com>
Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
---
arch/x86/events/core.c | 32 +++++++++++++++++++++++--------
arch/x86/include/asm/perf_event.h | 2 ++
2 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index d19e939f3998..9fdc5fa22c66 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -3011,16 +3011,35 @@ unsigned long perf_arch_instruction_pointer(struct pt_regs *regs)
return regs->ip + code_segment_base(regs);
}
+static unsigned long common_misc_flags(struct pt_regs *regs)
+{
+ if (regs->flags & PERF_EFLAGS_EXACT)
+ return PERF_RECORD_MISC_EXACT_IP;
+
+ return 0;
+}
+
+unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs)
+{
+ unsigned long guest_state = perf_guest_state();
+ unsigned long flags = common_misc_flags(regs);
+
+ if (!(guest_state & PERF_GUEST_ACTIVE))
+ return flags;
+
+ if (guest_state & PERF_GUEST_USER)
+ return flags & PERF_RECORD_MISC_GUEST_USER;
+ else
+ return flags & PERF_RECORD_MISC_GUEST_KERNEL;
+}
+
unsigned long perf_arch_misc_flags(struct pt_regs *regs)
{
unsigned int guest_state = perf_guest_state();
- int misc = 0;
+ unsigned long misc = common_misc_flags(regs);
if (guest_state) {
- if (guest_state & PERF_GUEST_USER)
- misc |= PERF_RECORD_MISC_GUEST_USER;
- else
- misc |= PERF_RECORD_MISC_GUEST_KERNEL;
+ misc |= perf_arch_guest_misc_flags(regs);
} else {
if (user_mode(regs))
misc |= PERF_RECORD_MISC_USER;
@@ -3028,9 +3047,6 @@ unsigned long perf_arch_misc_flags(struct pt_regs *regs)
misc |= PERF_RECORD_MISC_KERNEL;
}
- if (regs->flags & PERF_EFLAGS_EXACT)
- misc |= PERF_RECORD_MISC_EXACT_IP;
-
return misc;
}
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index feb87bf3d2e9..d95f902acc52 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -538,7 +538,9 @@ struct x86_perf_regs {
extern unsigned long perf_arch_instruction_pointer(struct pt_regs *regs);
extern unsigned long perf_arch_misc_flags(struct pt_regs *regs);
+extern unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs);
#define perf_arch_misc_flags(regs) perf_arch_misc_flags(regs)
+#define perf_arch_guest_misc_flags(regs) perf_arch_guest_misc_flags(regs)
#include <asm/stacktrace.h>
--
2.47.0.277.g8800431eea-goog
On Thu, Nov 07, 2024 at 07:03:35PM +0000, Colton Lewis wrote: > Break the assignment logic for misc flags into their own respective > functions to reduce the complexity of the nested logic. > > Signed-off-by: Colton Lewis <coltonlewis@google.com> > Reviewed-by: Oliver Upton <oliver.upton@linux.dev> > --- > arch/x86/events/core.c | 32 +++++++++++++++++++++++-------- > arch/x86/include/asm/perf_event.h | 2 ++ > 2 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > index d19e939f3998..9fdc5fa22c66 100644 > --- a/arch/x86/events/core.c > +++ b/arch/x86/events/core.c > @@ -3011,16 +3011,35 @@ unsigned long perf_arch_instruction_pointer(struct pt_regs *regs) > return regs->ip + code_segment_base(regs); > } > > +static unsigned long common_misc_flags(struct pt_regs *regs) > +{ > + if (regs->flags & PERF_EFLAGS_EXACT) > + return PERF_RECORD_MISC_EXACT_IP; > + > + return 0; > +} > + > +unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) > +{ > + unsigned long guest_state = perf_guest_state(); > + unsigned long flags = common_misc_flags(regs); This is double common_misc and makes no sense > + > + if (!(guest_state & PERF_GUEST_ACTIVE)) > + return flags; > + > + if (guest_state & PERF_GUEST_USER) > + return flags & PERF_RECORD_MISC_GUEST_USER; > + else > + return flags & PERF_RECORD_MISC_GUEST_KERNEL; And this is just broken garbage, right? > +} Did you mean to write: unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) { unsigned long guest_state = perf_guest_state(); unsigned long flags = 0; if (guest_state & PERF_GUEST_ACTIVE) { if (guest_state & PERF_GUEST_USER) flags |= PERF_RECORD_MISC_GUEST_USER; else flags |= PERF_RECORD_MISC_GUEST_KERNEL; } return flags; } > unsigned long perf_arch_misc_flags(struct pt_regs *regs) > { > unsigned int guest_state = perf_guest_state(); > - int misc = 0; > + unsigned long misc = common_misc_flags(regs); Because here you do the common thing.. > > if (guest_state) { > - if (guest_state & PERF_GUEST_USER) > - misc |= PERF_RECORD_MISC_GUEST_USER; > - else > - misc |= PERF_RECORD_MISC_GUEST_KERNEL; > + misc |= perf_arch_guest_misc_flags(regs); And here you mix in the guest things. > } else { > if (user_mode(regs)) > misc |= PERF_RECORD_MISC_USER;
Peter Zijlstra <peterz@infradead.org> writes: > On Thu, Nov 07, 2024 at 07:03:35PM +0000, Colton Lewis wrote: >> Break the assignment logic for misc flags into their own respective >> functions to reduce the complexity of the nested logic. >> Signed-off-by: Colton Lewis <coltonlewis@google.com> >> Reviewed-by: Oliver Upton <oliver.upton@linux.dev> >> --- >> arch/x86/events/core.c | 32 +++++++++++++++++++++++-------- >> arch/x86/include/asm/perf_event.h | 2 ++ >> 2 files changed, 26 insertions(+), 8 deletions(-) >> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c >> index d19e939f3998..9fdc5fa22c66 100644 >> --- a/arch/x86/events/core.c >> +++ b/arch/x86/events/core.c >> @@ -3011,16 +3011,35 @@ unsigned long >> perf_arch_instruction_pointer(struct pt_regs *regs) >> return regs->ip + code_segment_base(regs); >> } >> +static unsigned long common_misc_flags(struct pt_regs *regs) >> +{ >> + if (regs->flags & PERF_EFLAGS_EXACT) >> + return PERF_RECORD_MISC_EXACT_IP; >> + >> + return 0; >> +} >> + >> +unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) >> +{ >> + unsigned long guest_state = perf_guest_state(); >> + unsigned long flags = common_misc_flags(regs); > This is double common_misc and makes no sense I'm confused what you mean. Are you referring to starting with common_misc_flags in both perf_arch_misc_flags and perf_arch_guest_misc_flags so possibly the common_msic_flags are set twice? That seems like a good thing that common flags are set wherever they apply. You can't guarantee where perf_arch_guest_misc_flags may be called in the future. >> + >> + if (!(guest_state & PERF_GUEST_ACTIVE)) >> + return flags; >> + >> + if (guest_state & PERF_GUEST_USER) >> + return flags & PERF_RECORD_MISC_GUEST_USER; >> + else >> + return flags & PERF_RECORD_MISC_GUEST_KERNEL; > And this is just broken garbage, right? >> +} > Did you mean to write: > unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) > { > unsigned long guest_state = perf_guest_state(); > unsigned long flags = 0; > if (guest_state & PERF_GUEST_ACTIVE) { > if (guest_state & PERF_GUEST_USER) > flags |= PERF_RECORD_MISC_GUEST_USER; > else > flags |= PERF_RECORD_MISC_GUEST_KERNEL; > } > return flags; > } Ok, my mistake was using & instead of |, but the branches are functionally the same. I'll use something closer to your suggestion. >> unsigned long perf_arch_misc_flags(struct pt_regs *regs) >> { >> unsigned int guest_state = perf_guest_state(); >> - int misc = 0; >> + unsigned long misc = common_misc_flags(regs); > Because here you do the common thing.. >> if (guest_state) { >> - if (guest_state & PERF_GUEST_USER) >> - misc |= PERF_RECORD_MISC_GUEST_USER; >> - else >> - misc |= PERF_RECORD_MISC_GUEST_KERNEL; >> + misc |= perf_arch_guest_misc_flags(regs); > And here you mix in the guest things. >> } else { >> if (user_mode(regs)) >> misc |= PERF_RECORD_MISC_USER;
On Fri, Nov 08, 2024 at 07:01:16PM +0000, Colton Lewis wrote: > Peter Zijlstra <peterz@infradead.org> writes: > > > On Thu, Nov 07, 2024 at 07:03:35PM +0000, Colton Lewis wrote: > > > Break the assignment logic for misc flags into their own respective > > > functions to reduce the complexity of the nested logic. > > > > Signed-off-by: Colton Lewis <coltonlewis@google.com> > > > Reviewed-by: Oliver Upton <oliver.upton@linux.dev> > > > --- > > > arch/x86/events/core.c | 32 +++++++++++++++++++++++-------- > > > arch/x86/include/asm/perf_event.h | 2 ++ > > > 2 files changed, 26 insertions(+), 8 deletions(-) > > > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > > > index d19e939f3998..9fdc5fa22c66 100644 > > > --- a/arch/x86/events/core.c > > > +++ b/arch/x86/events/core.c > > > @@ -3011,16 +3011,35 @@ unsigned long > > > perf_arch_instruction_pointer(struct pt_regs *regs) > > > return regs->ip + code_segment_base(regs); > > > } > > > > +static unsigned long common_misc_flags(struct pt_regs *regs) > > > +{ > > > + if (regs->flags & PERF_EFLAGS_EXACT) > > > + return PERF_RECORD_MISC_EXACT_IP; > > > + > > > + return 0; > > > +} > > > + > > > +unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) > > > +{ > > > + unsigned long guest_state = perf_guest_state(); > > > + unsigned long flags = common_misc_flags(regs); > > > This is double common_misc and makes no sense > > I'm confused what you mean. Are you referring to starting with > common_misc_flags in both perf_arch_misc_flags and > perf_arch_guest_misc_flags so possibly the common_msic_flags are set > twice? > > That seems like a good thing that common flags are set wherever they > apply. You can't guarantee where perf_arch_guest_misc_flags may be > called in the future. I got confused by perf_arch_misc_flags() calling common_misc_flags() twice. It is in fact worse, because afaict all of perf_arch_guest_misc_flags() is 'common'. Isn't the below more or less what you want? static unsigned long misc_flags(struct pt_regs *regs) { unsigned long flags = 0; if (regs->flags & PERF_EFLAGS_EXACT) flags |= PERF_RECORD_MISC_EXACT_IP; return flags; } static unsigned long native_flags(struct pt_regs *regs) { unsigned long flags = 0; if (user_mode(regs)) flags |= PERF_RECORD_MISC_USER; else flags |= PERF_RECORD_MISC_KERNEL; return flags; } static unsigned long guest_flags(struct pt_regs *regs) { unsigned long guest_state = perf_guest_state(); unsigned long flags = 0; if (guest_state & PERF_GUEST_ACTIVE) { if (guest_state & PERF_GUEST_USER) flags |= PERF_RECORD_MISC_GUEST_USER; else flags |= PERF_RECORD_MISC_GUEST_KERNEL; } return flags; } unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) { unsigned long flags; flags = misc_flags(regs); flags |= guest_flags(regs); return flags; } unsigned long perf_arch_misc_flags(struct pt_regs *regs) { unsigned long flags; unsigned long guest; flags = misc_flags(regs); guest = guest_flags(regs); if (guest) flags |= guest; else flags |= native_flags(regs); return flags; } Note how both perf_arch*() functions end up calling both misc and guest.
On Fri, Nov 08, 2024 at 08:20:44PM +0100, Peter Zijlstra wrote: > Isn't the below more or less what you want? > > static unsigned long misc_flags(struct pt_regs *regs) > { > unsigned long flags = 0; > > if (regs->flags & PERF_EFLAGS_EXACT) > flags |= PERF_RECORD_MISC_EXACT_IP; > > return flags; > } > > static unsigned long native_flags(struct pt_regs *regs) > { > unsigned long flags = 0; > > if (user_mode(regs)) > flags |= PERF_RECORD_MISC_USER; > else > flags |= PERF_RECORD_MISC_KERNEL; > > return flags; > } > > static unsigned long guest_flags(struct pt_regs *regs) > { > unsigned long guest_state = perf_guest_state(); > unsigned long flags = 0; > > if (guest_state & PERF_GUEST_ACTIVE) { > if (guest_state & PERF_GUEST_USER) > flags |= PERF_RECORD_MISC_GUEST_USER; > else > flags |= PERF_RECORD_MISC_GUEST_KERNEL; > } > > return flags; > } > > unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) > { > unsigned long flags; > > flags = misc_flags(regs); > flags |= guest_flags(regs); > > return flags; > } > > unsigned long perf_arch_misc_flags(struct pt_regs *regs) > { > unsigned long flags; > unsigned long guest; > > flags = misc_flags(regs); > guest = guest_flags(regs); > if (guest) > flags |= guest; > else > flags |= native_flags(regs); > > return flags; > } This last can be written more concise: unsigned long perf_arch_misc_flags(struct pt_regs *regs) { unsigned long flags; flags = guest_flags(regs); if (!flags) flags |= native_flags(regs); flgs |= misc_flags(regs); return flags; }
Peter Zijlstra <peterz@infradead.org> writes: > On Fri, Nov 08, 2024 at 08:20:44PM +0100, Peter Zijlstra wrote: >> Isn't the below more or less what you want? >> static unsigned long misc_flags(struct pt_regs *regs) >> { >> unsigned long flags = 0; >> if (regs->flags & PERF_EFLAGS_EXACT) >> flags |= PERF_RECORD_MISC_EXACT_IP; >> return flags; >> } >> static unsigned long native_flags(struct pt_regs *regs) >> { >> unsigned long flags = 0; >> if (user_mode(regs)) >> flags |= PERF_RECORD_MISC_USER; >> else >> flags |= PERF_RECORD_MISC_KERNEL; >> return flags; >> } >> static unsigned long guest_flags(struct pt_regs *regs) >> { >> unsigned long guest_state = perf_guest_state(); >> unsigned long flags = 0; >> if (guest_state & PERF_GUEST_ACTIVE) { >> if (guest_state & PERF_GUEST_USER) >> flags |= PERF_RECORD_MISC_GUEST_USER; >> else >> flags |= PERF_RECORD_MISC_GUEST_KERNEL; >> } >> return flags; >> } >> unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) >> { >> unsigned long flags; >> flags = misc_flags(regs); >> flags |= guest_flags(regs); >> return flags; >> } >> unsigned long perf_arch_misc_flags(struct pt_regs *regs) >> { >> unsigned long flags; >> unsigned long guest; >> flags = misc_flags(regs); >> guest = guest_flags(regs); >> if (guest) >> flags |= guest; >> else >> flags |= native_flags(regs); >> return flags; >> } > This last can be written more concise: > unsigned long perf_arch_misc_flags(struct pt_regs *regs) > { > unsigned long flags; > flags = guest_flags(regs); > if (!flags) > flags |= native_flags(regs); > flgs |= misc_flags(regs); > return flags; > } This isn't right because it is choosing to return guest or native flags depending on the presence of guest flags, but that's not what we want. See perf_misc_flags in kernel/events/core.c which chooses to return perf_arch_guest_misc_flags or perf_arch_misc_flags depending on should_sample_guest which depends on more than current guest state. But I will take some of your suggestions to split the functions out more.
Colton Lewis <coltonlewis@google.com> writes: > Peter Zijlstra <peterz@infradead.org> writes: >> On Fri, Nov 08, 2024 at 08:20:44PM +0100, Peter Zijlstra wrote: >>> Isn't the below more or less what you want? >>> static unsigned long misc_flags(struct pt_regs *regs) >>> { >>> unsigned long flags = 0; >>> if (regs->flags & PERF_EFLAGS_EXACT) >>> flags |= PERF_RECORD_MISC_EXACT_IP; >>> return flags; >>> } >>> static unsigned long native_flags(struct pt_regs *regs) >>> { >>> unsigned long flags = 0; >>> if (user_mode(regs)) >>> flags |= PERF_RECORD_MISC_USER; >>> else >>> flags |= PERF_RECORD_MISC_KERNEL; >>> return flags; >>> } >>> static unsigned long guest_flags(struct pt_regs *regs) >>> { >>> unsigned long guest_state = perf_guest_state(); >>> unsigned long flags = 0; >>> if (guest_state & PERF_GUEST_ACTIVE) { >>> if (guest_state & PERF_GUEST_USER) >>> flags |= PERF_RECORD_MISC_GUEST_USER; >>> else >>> flags |= PERF_RECORD_MISC_GUEST_KERNEL; >>> } >>> return flags; >>> } >>> unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) >>> { >>> unsigned long flags; >>> flags = misc_flags(regs); >>> flags |= guest_flags(regs); >>> return flags; >>> } >>> unsigned long perf_arch_misc_flags(struct pt_regs *regs) >>> { >>> unsigned long flags; >>> unsigned long guest; >>> flags = misc_flags(regs); >>> guest = guest_flags(regs); >>> if (guest) >>> flags |= guest; >>> else >>> flags |= native_flags(regs); >>> return flags; >>> } >> This last can be written more concise: >> unsigned long perf_arch_misc_flags(struct pt_regs *regs) >> { >> unsigned long flags; >> flags = guest_flags(regs); >> if (!flags) >> flags |= native_flags(regs); >> flgs |= misc_flags(regs); >> return flags; >> } > This isn't right because it is choosing to return guest or native > flags depending on the presence of guest flags, but that's not what we > want. > See perf_misc_flags in kernel/events/core.c which chooses to return > perf_arch_guest_misc_flags or perf_arch_misc_flags depending on > should_sample_guest which depends on more than current guest state. This is in the next patch. Excuse me for not clarifying. > But I will take some of your suggestions to split the functions out > more.
On 2024-11-07 2:03 p.m., Colton Lewis wrote: > Break the assignment logic for misc flags into their own respective > functions to reduce the complexity of the nested logic. > > Signed-off-by: Colton Lewis <coltonlewis@google.com> > Reviewed-by: Oliver Upton <oliver.upton@linux.dev> > --- Acked-by: Kan Liang <kan.liang@linux.intel.com> Thanks, Kan > arch/x86/events/core.c | 32 +++++++++++++++++++++++-------- > arch/x86/include/asm/perf_event.h | 2 ++ > 2 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c > index d19e939f3998..9fdc5fa22c66 100644 > --- a/arch/x86/events/core.c > +++ b/arch/x86/events/core.c > @@ -3011,16 +3011,35 @@ unsigned long perf_arch_instruction_pointer(struct pt_regs *regs) > return regs->ip + code_segment_base(regs); > } > > +static unsigned long common_misc_flags(struct pt_regs *regs) > +{ > + if (regs->flags & PERF_EFLAGS_EXACT) > + return PERF_RECORD_MISC_EXACT_IP; > + > + return 0; > +} > + > +unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs) > +{ > + unsigned long guest_state = perf_guest_state(); > + unsigned long flags = common_misc_flags(regs); > + > + if (!(guest_state & PERF_GUEST_ACTIVE)) > + return flags; > + > + if (guest_state & PERF_GUEST_USER) > + return flags & PERF_RECORD_MISC_GUEST_USER; > + else > + return flags & PERF_RECORD_MISC_GUEST_KERNEL; > +} > + > unsigned long perf_arch_misc_flags(struct pt_regs *regs) > { > unsigned int guest_state = perf_guest_state(); > - int misc = 0; > + unsigned long misc = common_misc_flags(regs); > > if (guest_state) { > - if (guest_state & PERF_GUEST_USER) > - misc |= PERF_RECORD_MISC_GUEST_USER; > - else > - misc |= PERF_RECORD_MISC_GUEST_KERNEL; > + misc |= perf_arch_guest_misc_flags(regs); > } else { > if (user_mode(regs)) > misc |= PERF_RECORD_MISC_USER; > @@ -3028,9 +3047,6 @@ unsigned long perf_arch_misc_flags(struct pt_regs *regs) > misc |= PERF_RECORD_MISC_KERNEL; > } > > - if (regs->flags & PERF_EFLAGS_EXACT) > - misc |= PERF_RECORD_MISC_EXACT_IP; > - > return misc; > } > > diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h > index feb87bf3d2e9..d95f902acc52 100644 > --- a/arch/x86/include/asm/perf_event.h > +++ b/arch/x86/include/asm/perf_event.h > @@ -538,7 +538,9 @@ struct x86_perf_regs { > > extern unsigned long perf_arch_instruction_pointer(struct pt_regs *regs); > extern unsigned long perf_arch_misc_flags(struct pt_regs *regs); > +extern unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs); > #define perf_arch_misc_flags(regs) perf_arch_misc_flags(regs) > +#define perf_arch_guest_misc_flags(regs) perf_arch_guest_misc_flags(regs) > > #include <asm/stacktrace.h> >
© 2016 - 2024 Red Hat, Inc.