[PATCH] Cleanup locking in sys_reboot() (was Re: [PATCH] Reduce stack usage in sys.c)
From: Yum Rayan
Date: Sat Apr 02 2005 - 02:08:14 EST
On Mar 31, 2005 12:29 AM, Jeff Garzik <jgarzik@xxxxxxxxx> wrote:
> Your "cleanup lock usage" increases the number of lock_kernel() calls
> quite a bit, which is not really a cleanup but simply bloat.
Yes, just looking at the patch, seem to indicate so. But let's take a
closer look at the original code from a run time perspective :
346 lock_kernel();
347 switch (cmd) {
...
355 case LINUX_REBOOT_CMD_CAD_ON:
356 C_A_D = 1;
357 break;
....
421 unlock_kernel();
422 return 0;
Why the lock_kernel() and unlock_kernel()? This happens for another
case as follows:
346 lock_kernel();
347 switch (cmd) {
...
358 case LINUX_REBOOT_CMD_CAD_OFF:
359 C_A_D = 0;
360 break;
...
421 unlock_kernel();
422 return 0;
Should'nt we be keeping the lock_kernel() and unlock_kernel() calls to
a minimum?
Again something sloppy happening here:
346 lock_kernel();
347 switch (cmd) {
...
361 case LINUX_REBOOT_CMD_HALT:
362 notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
...
376 unlock_kernel();
377 do_exit(0);
378 break;
...
421 unlock_kernel();
422 return 0;
The previous author shows deligence in having the "break;" after
"do_exit(0)", but why "unlock_kernel()" twice in the same path? What
if down the road, someone changes do_exit() to do something else and
actually return ?
Same style above, shown for this case as well:
370 case LINUX_REBOOT_CMD_POWER_OFF:
Some other kind of mess:
410 case LINUX_REBOOT_CMD_SW_SUSPEND:
411 {
412 int ret = software_suspend();
413 unlock_kernel();
414 return ret;
415 }
... << the switch...case ends at this line
421 unlock_kernel();
422 return 0;
Could'nt we just have a single "unlock_kernel()" above?
Some more:
417 default:
418 unlock_kernel();
419 return -EINVAL;
420 }
421 unlock_kernel();
422 return 0;
It would have been nice to have a single "unlock_kernel()" and single
point of exit. Also note that for "default" case, we are doing
lock_kernel() and unlock_kernel() for nothing?
And finally:
346 lock_kernel();
347 switch (cmd) {
...
379 case LINUX_REBOOT_CMD_RESTART2:
380 if (strncpy_from_user(&buffer[0], arg,
sizeof(buffer) - 1) < 0) {
381 unlock_kernel();
382 return -EFAULT;
383 }
Does the "strncpy_from_user()" really need a lock_kernel()?
My attempt to reduce the stack usage needed to kmalloc buffer and
buffer was being used for the above case (LINUX_REBOOT_CMD_RESTART2)
only. I did not think it was good to have lock_kernel() for the
kmalloc and the subsequent NULL checking of the returned pointer. So I
ended up driving the lock_kernel() and matching unlock_kernel() calls
deeper, IMHO a cleanup. In some cases the unlock_kernel() calls are
provided for sake of completeness, just like the "break;" statements.
You might count the number of "lock_kernel()" to increase in the code,
but actually the patch minimizes the run time calls to
"lock_kernel()".
I assume a call like sys_reboot() is no big deal, but feedback will
always help going forward. I dropped the pick at the stack usage, just
the patch to move the locks around... (cleanup?)
Thanks,
Rayan
Signed-off-by: Yum Rayan <yum.rayan@xxxxxxxxx>
--- linux-2.6.12-rc1-mm4.a/kernel/sys.c 2005-03-31 16:51:30.000000000 -0800
+++ linux-2.6.12-rc1-mm4.b/kernel/sys.c 2005-04-01 22:46:53.000000000 -0800
@@ -385,14 +385,15 @@
magic2 != LINUX_REBOOT_MAGIC2C))
return -EINVAL;
- lock_kernel();
switch (cmd) {
case LINUX_REBOOT_CMD_RESTART:
+ lock_kernel();
notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL);
system_state = SYSTEM_RESTART;
device_shutdown();
printk(KERN_EMERG "Restarting system.\n");
machine_restart(NULL);
+ unlock_kernel();
break;
case LINUX_REBOOT_CMD_CAD_ON:
@@ -404,6 +405,7 @@
break;
case LINUX_REBOOT_CMD_HALT:
+ lock_kernel();
notifier_call_chain(&reboot_notifier_list, SYS_HALT, NULL);
system_state = SYSTEM_HALT;
device_shutdown();
@@ -414,6 +416,7 @@
break;
case LINUX_REBOOT_CMD_POWER_OFF:
+ lock_kernel();
notifier_call_chain(&reboot_notifier_list, SYS_POWER_OFF, NULL);
system_state = SYSTEM_POWER_OFF;
device_shutdown();
@@ -425,22 +428,24 @@
case LINUX_REBOOT_CMD_RESTART2:
if (strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1) < 0) {
- unlock_kernel();
return -EFAULT;
}
buffer[sizeof(buffer) - 1] = '\0';
+ lock_kernel();
notifier_call_chain(&reboot_notifier_list, SYS_RESTART, buffer);
system_state = SYSTEM_RESTART;
device_shutdown();
printk(KERN_EMERG "Restarting system with command '%s'.\n", buffer);
machine_restart(buffer);
+ unlock_kernel();
break;
#ifdef CONFIG_KEXEC
case LINUX_REBOOT_CMD_KEXEC:
{
struct kimage *image;
+ lock_kernel();
image = xchg(&kexec_image, 0);
if (!image) {
unlock_kernel();
@@ -452,23 +457,24 @@
printk(KERN_EMERG "Starting new kernel\n");
machine_shutdown();
machine_kexec(image);
+ unlock_kernel();
break;
}
#endif
#ifdef CONFIG_SOFTWARE_SUSPEND
case LINUX_REBOOT_CMD_SW_SUSPEND:
{
- int ret = software_suspend();
+ int ret;
+ lock_kernel();
+ ret = software_suspend();
unlock_kernel();
return ret;
}
#endif
default:
- unlock_kernel();
return -EINVAL;
}
- unlock_kernel();
return 0;
}
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/