Messages in this thread | | | Date | Thu, 26 Mar 2009 10:01:07 +0100 | From | Andi Kleen <> | Subject | Re: [PATCH -tip 3/3] x86, mce: Add mce=nopoll option to disable timer polling |
| |
Hidetoshi Seto wrote: > This patch adds "mce=nopoll" option to disable timer polling > for corrected errors from boot. Unlike "mce=off", it doesn't > prevent handling for uncorrected errors. > > It is useful if: > - You don't have any interests in corrected errors. You may > use option mce_threshold=0 to disable cmci too. > - You'd like to care banks only which cmci are supported.
These two seem to be conflicting. CMCI errors are corrected errors too. Why would the user care about the reporting mechanism and only shut down one or not another?
Also I'm not sure a boot argument is really needed. Isn't it good enough to do this early at boot through sysfs?
> - You have an application such as hardware monitor that > checks error banks, and that can conflict with OS's polling.
Well then your patch is not enough because it doesn't shut off boot time clearing/logging of corrected errors left over from boot for once. And CMCI.
> - Your system have an intelligent BIOS which can provide > enough health information, so reports from OS is redundant.
It would seem inconvenient then to require the user to set a special boot option. I think it would be better if the BIOS set a flag somewhere that the kernel can check.
> Once booted, we can disable polling by setting check_interval > to 0, but there are no mention about the fact.
That's true, Documentation/x86/x86_64/machinecheck should be fixed to say 0 means no polling. I'm not sure new boot option are the preferred fix for missing documentation though @)
> static int check_interval = 5 * 60; /* 5 minutes */ > @@ -633,11 +635,12 @@ static void mce_init_timer(void) > { > struct timer_list *t = &__get_cpu_var(mce_timer); > > + /* Disable polling if check_interval is 0 */ > + if (!check_interval) > + return;
This check shouldn't be needed, the next two checks already do that.
Also there's a conflicting patch pending which moves next_interval to be per CPU.
-Andi
| |