Re: [PATCH v7 4/5] x86: perf: Refactor misc flag assignments

[Colton Lewis]

Peter Zijlstra <pet...@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.