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 - 2026 Red Hat, Inc.