Re: [PATCH][RFC] x86: override prefer_mwait_c1_over_halt to avoid loading cpuidle-haltpoll driver

[Thomas Gleixner <tglx@linutronix.de>]

Li!

On Fri, Dec 02 2022 at 11:37, lirongqing@baidu.com wrote:
> From: Li RongQing <lirongqing@baidu.com>
>
> x86 KVM guests with MWAIT can load cpuidle-haltpoll driver, and will
> cause performance degradation, so override prefer_mwait_c1_over_halt
> to a new value, aviod loading cpuidle-haltpoll driver

Neither the subject line nor the above makes any sense to me.

prefer_mwait_c1_over_halt() is a function which is invoked and when it
returns true then the execution ends up in the code path you are
patching.

> @@ -889,6 +889,7 @@ void select_idle_routine(const struct cpuinfo_x86 *c)
>  	} else if (prefer_mwait_c1_over_halt(c)) {
>  		pr_info("using mwait in idle threads\n");
>  		x86_idle = mwait_idle;
> +		boot_option_idle_override = IDLE_PREF_MWAIT;

What you do is setting boot_option_idle_override to a new value, but
that has nothing to do with prefer_mwait_c1_over_halt() at all.

So how are you overriding that function to a new value?

But that's just a word smithing problem.

The real and way worse problem is that you pick a variable, which has
the purpose to capture the idle override on the kernel command line, and
modify it as you see fit, just to prevent that driver from loading.

select_idle_routine() reads it to check whether there was a command line
override or not. But it is not supposed to write it. Why?

Have you checked what else evaluates that variable? Obviously not,
because a simple grep would have told you:

drivers/cpuidle/cpuidle-haltpoll.c:     if (boot_option_idle_override != IDLE_NO_OVERRIDE)
drivers/idle/intel_idle.c:      if (boot_option_idle_override != IDLE_NO_OVERRIDE)

Congratulations!

Your patch breaks the default setup of every recent Intel system on the
planet because it not only prevents the cpuidle-haltpoll, but also the
intel_idle driver from loading.

Seriously. It's not too much asked to do at least a simple grep and look
at all _nine_ places which evaluate boot_option_idle_override.

Thanks,

      tglx