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

[Colton Lewis]

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