Unnecessary work and noise from mce code in suspend/resume path
RFC 1/1 Unnecessary work and noise from mce code in suspend/resume path
When we suspend a laptop we offline all but one processor. But the mce code registers on a notify chain so it can clean up some sysfs entries. Part of that code calls device_unregister() which will fire kobject_uevent() which might wake up some user code that is watching for such things.
Patch below from Aiden Park works around the issue by avoiding the device_unregister()/device_register() by just keeping track of the original device registrations.
1) Is there a better way to avoid the kobject_uevent()?
2) What user code actually uses all these sysfs files? Some test code in mcelog(8) uses:
/sys/devices/system/machinecheck/machinecheck0/trigger
Some code in mce-test package adjusts
/sys/devices/system/machinecheck/machinecheck0/tolerant
But neither of these make use of all the per-cpu instances created/destroyed on every suspend/resume (or logical processor offline). Should we just move these out of per-cpu directories and scrap this whole block of code?
3) checkpatch isn't happy about use of NR_CPUS I suspect the usage here warrants for_each_possible_cpu()
-Tony Luck
---
arch/x86/kernel/cpu/mcheck/mce.c +30 -12
@@ -113,6 +113,8 @@ static DEFINE_PER_CPU(struct work_struct, mce_work);static void (*quirk_no_way_out)(int bank,struct mce *m,struct pt_regs *regs); +static struct device *mce_devices[NR_CPUS ]; +/* * CPU/chipset specific EDAC code can register a notifier call here to print * MCE errors in a human-readable form.
@@ -2060,7 +2062,18 @@ static int mce_syscore_suspend(void)static void mce_syscore_shutdown (void ) { +int i =0 ; +struct device *dev; +mce_disable_error_reporting (); + +for (i =0 ; i <NR_CPUS ; i++) { + dev = mce_devices[i]; +if (dev) { +device_unregister (dev); + mce_devices[i] =NULL ; + } + } }/*
@@ -2272,23 +2285,27 @@ static void mce_device_release(struct device *dev)static int mce_device_create (unsigned int cpu) {struct device *dev; -int err; +int err =0 ;int i, j;if (!mce_available (&boot_cpu_data))return -EIO ; - dev =kzalloc (sizeof *dev,GFP_KERNEL ); -if (!dev) -return -ENOMEM ; - dev->id = cpu; - dev->bus = &mce_subsys; - dev->release = &mce_device_release; + dev = mce_devices[cpu]; +if (!dev) { + dev =kzalloc (sizeof *dev,GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + dev->id = cpu; + dev->bus = &mce_subsys; + dev->release = &mce_device_release; - err =device_register (dev); -if (err) { -put_device (dev); -return err; + err =device_register (dev); +if (err) { +put_device (dev); +return err; + } + mce_devices[cpu] = dev; }for (i =0 ; mce_device_attrs[i]; i++) {
@@ -2313,6 +2330,7 @@ error:device_remove_file (dev, mce_device_attrs[i]);device_unregister (dev); + mce_devices[cpu] =NULL ;return err; }
@@ -2331,7 +2349,6 @@ static void mce_device_remove(unsigned int cpu)for (i =0 ; i < mca_cfg.banks; i++)device_remove_file (dev, &mce_banks[i].attr); -device_unregister (dev);cpumask_clear_cpu (cpu, mce_device_initialized);per_cpu (mce_device, cpu) =NULL ; }
@@ -2450,6 +2467,7 @@ static __init int mcheck_init_device(void)cpu_notifier_register_begin ();for_each_online_cpu (i) { + mce_devices[i] =NULL ; err =mce_device_create (i);if (err) {cpu_notifier_register_done ();
Borislav Petkov
maintainerQuoted 5 lines · Tony
When we suspend a laptop we offline all but one processor. But the mce code registers on a notify chain so it can clean up some sysfs entries. Part of that code calls
device_unregister()which will firekobject_uevent()which might wake up some user code that is watching for such things.
The issue being?
It is not clear from the text what actually is the problem.
Quoted 4 lines
Patch below from Aiden Park works around the issue by avoiding the device_unregister()/device_register() by just keeping track of the original device registrations.
Quoted 10 lines
1) Is there a better way to avoid the
kobject_uevent()?
2) What user code actually uses all these sysfs files? Some test code in mcelog(8) uses:
/sys/devices/system/machinecheck/machinecheck0/trigger
Some code in mce-test package adjusts
/sys/devices/system/machinecheck/machinecheck0/tolerant
But neither of these make use of all the per-cpu instances created/destroyed on every suspend/resume (or logical processor offline). Should we just move these out of per-cpu directories and scrap this whole block of code?
Oh, I'd love to.
Especially "trigger" - it wants to be in debugfs anyway. The "tolerant" deal we probably need to discuss a bit more. Exposing MCE tolerance levels even to root is not such a good idea and this tolerant thing is primarily used in testing, AFAICT, so that the box doesn't actually panic when you inject errors and so on...
Quoted 2 lines
3) checkpatch isn't happy about use of
NR_CPUSI suspect the usage here warrantsfor_each_possible_cpu()
Right.
arch/x86/kernel/cpu/mcheck/mce.c
Quoted 6 lines
(quoted diff) +0
-2-Tony Luck ---arch/x86/kernel/cpu/mcheck/mce.c +0
-0
Something decided to make mce.c an executable script :-)
Quoted 13 lines
index
68317c80de7f..e49302f24ce5arch/x86/kernel/cpu/mcheck/mce.c +2
-0@@ -113,6 +113,8 @@ static DEFINE_PER_CPU(struct work_struct, mce_work);static void (*quirk_no_way_out)(int bank,struct mce *m,struct pt_regs *regs); +static struct device *mce_devices[NR_CPUS ]; +/* * CPU/chipset specific EDAC code can register a notifier call here to print * MCE errors in a human-readable form.
arch/x86/kernel/cpu/mcheck/mce.c static int mce_syscore_suspend(void) · line 2070
Quoted 9 lines
arch/x86/kernel/cpu/mcheck/mce.c +5
-0@@ -2060,7 +2062,18 @@ static int mce_syscore_suspend(void)static void mce_syscore_shutdown (void ) { +int i =0 ; +struct device *dev; +mce_disable_error_reporting (); + +for (i =0 ; i <NR_CPUS ; i++) {
WARNING: usage of NR_CPUS is often wrong - consider using cpu_possible(), num_possible_cpus(), for_each_possible_cpu(), etc #64: FILE: arch/x86/kernel/cpu/mcheck/mce.c:2070:
+ for (i = 0; i < NR_CPUS; i++) {
Quoted 19 lines
arch/x86/kernel/cpu/mcheck/mce.c +7
-1+ dev = mce_devices[i]; +if (dev) { +device_unregister (dev); + mce_devices[i] =NULL ; + } + } }/* @@ -2272,23 +2285,27 @@ static void mce_device_release(struct device *dev)static int mce_device_create (unsigned int cpu) {struct device *dev; -int err; +int err =0 ;int i, j;if (!mce_available (&boot_cpu_data))return -EIO ;
arch/x86/kernel/cpu/mcheck/mce.c static void mce_device_release(struct device *dev) · line 2296
Quoted 10 lines
arch/x86/kernel/cpu/mcheck/mce.c +3
-6- dev =kzalloc (sizeof *dev,GFP_KERNEL ); -if (!dev) -return -ENOMEM ; - dev->id = cpu; - dev->bus = &mce_subsys; - dev->release = &mce_device_release; + dev = mce_devices[cpu]; +if (!dev) { + dev =kzalloc (sizeof *dev,GFP_KERNEL );
Please run through checkpatch:
WARNING: sizeof *dev should be sizeof(*dev)
#93: FILE: arch/x86/kernel/cpu/mcheck/mce.c:2296:
+ dev = kzalloc(sizeof *dev, GFP_KERNEL);
Quoted 6 lines
arch/x86/kernel/cpu/mcheck/mce.c +5
-0+if (!dev) +return -ENOMEM ; + dev->id = cpu; + dev->bus = &mce_subsys; + dev->release = &mce_device_release;
arch/x86/kernel/cpu/mcheck/mce.c static void mce_device_release(struct device *dev) · line 2308
Quoted 10 lines
arch/x86/kernel/cpu/mcheck/mce.c +6
-4- err =device_register (dev); -if (err) { -put_device (dev); -return err; + err =device_register (dev); +if (err) { +put_device (dev); +return err; + } + mce_devices[cpu] = dev;
Ok, in any case, AFAIU it is a mechanism to cache those mce_devices and not generate them every time we come from suspend.
And we have those mce_device per cpu vars which we use too. So this one adds another array of mce_devices.
Dumb question: would it be possible to save us all that init/teardown everytime and simply toggle their visibility in sysfs instead? I mean, maybe play with device_create_file/device_remove_file only and hand it down those struct device *dev things we have cached per-cpu.
This will definitely save us the kobject_uevent() calls.
But it could be that I'm missing something obvious...
Signature · 5 lines
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--Quoted 9 lines
Quoted 5 lines
When we suspend a laptop we offline all but one processor. But the mce code registers on a notify chain so it can clean up some sysfs entries. Part of that code calls
device_unregister()which will firekobject_uevent()which might wake up some user code that is watching for such things.The issue being?
It is not clear from the text what actually is the problem.
We are trying to suspend - and yet we are now having udev woken up to run some scripts ... that will burn power ... which is what we are trying to avoid when we shut the lid of the laptop. There might be some Android-isms involved too that I don't understand.
-Tony ���{.n�+�������+%�����ݶ��w��{.n�+����{��G�����{ay�ʇڙ�,j��f���h��������z_�(�階�ݢj"���m������G������?����&���~��iO��z��v�^�m����������?�I�