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

[Colton Lewis <coltonlewis@xxxxxxxxxx>]

Colton Lewis <coltonlewis@xxxxxxxxxx> writes:

> 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.

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.