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

Colton Lewis [off-list ref] writes:

Peter Zijlstra [off-list ref] writes:

quoted

On Fri, Nov 08, 2024 at 08:20:44PM +0100, Peter Zijlstra wrote:

quoted

quoted

Isn't the below more or less what you want?

quoted

quoted

static unsigned long misc_flags(struct pt_regs *regs)
{
	unsigned long flags = 0;

quoted

quoted

	if (regs->flags & PERF_EFLAGS_EXACT)
		flags |= PERF_RECORD_MISC_EXACT_IP;

quoted

quoted

	return flags;
}

quoted

quoted

static unsigned long native_flags(struct pt_regs *regs)
{
	unsigned long flags = 0;

quoted

quoted

	if (user_mode(regs))
		flags |= PERF_RECORD_MISC_USER;
	else
		flags |= PERF_RECORD_MISC_KERNEL;

quoted

quoted

	return flags;
}

quoted

quoted

static unsigned long guest_flags(struct pt_regs *regs)
{
	unsigned long guest_state = perf_guest_state();
	unsigned long flags = 0;

quoted

quoted

	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;
	}

quoted

quoted

	return flags;
}

quoted

quoted

unsigned long perf_arch_guest_misc_flags(struct pt_regs *regs)
{
	unsigned long flags;

quoted

quoted

	flags = misc_flags(regs);
	flags |= guest_flags(regs);

quoted

quoted

	return flags;
}

quoted

quoted

unsigned long perf_arch_misc_flags(struct pt_regs *regs)
{
	unsigned long flags;
	unsigned long guest;

quoted

quoted

	flags = misc_flags(regs);
	guest = guest_flags(regs);
	if (guest)
		flags |= guest;
	else
		flags |= native_flags(regs);

quoted

quoted

	return flags;
}

quoted

This last can be written more concise:

quoted

unsigned long perf_arch_misc_flags(struct pt_regs *regs)
{
	unsigned long flags;

quoted

	flags = guest_flags(regs);
	if (!flags)
		flags |= native_flags(regs);

quoted

	flgs |= misc_flags(regs);

quoted

	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.