Messages in this thread | | | From | Thomas Gleixner <> | Subject | Re: [PATCH][RFC] x86: override prefer_mwait_c1_over_halt to avoid loading cpuidle-haltpoll driver | Date | Fri, 02 Dec 2022 19:48:16 +0100 |
| |
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
| |