* [PATCH] Reduce stack usage in sys.c
@ 2005-03-31 7:44 Yum Rayan
2005-03-31 8:29 ` Jeff Garzik
2005-03-31 13:01 ` [PATCH] Reduce stack usage in sys.c Jörn Engel
0 siblings, 2 replies; 4+ messages in thread
From: Yum Rayan @ 2005-03-31 7:44 UTC (permalink / raw)
To: linux-kernel
Attempt to reduce stack usage in sys.c (linux-2.6.12-rc1-mm3). Stack
usage was noted using checkstack.pl. Specifically
Before patch
------------
sys_reboot - 256
After patch
-----------
sys_reboot - none (register usage only)
Along the way, wrap code to 80 column width and cleanup lock usage.
Signed-off-by: Yum Rayan <yum.rayan@gmail.com>
--- a/kernel/sys.c 2005-03-25 22:11:06.000000000 -0800
+++ b/kernel/sys.c 2005-03-30 22:34:03.000000000 -0800
@@ -369,9 +369,9 @@
*
* reboot doesn't sync: do that yourself before calling this.
*/
-asmlinkage long sys_reboot(int magic1, int magic2, unsigned int cmd,
void __user * arg)
+asmlinkage long sys_reboot(int magic1, int magic2,
+ unsigned int cmd, void __user * arg)
{
- char buffer[256];
/* We only trust the superuser with rebooting the system. */
if (!capable(CAP_SYS_BOOT))
@@ -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();
@@ -424,51 +427,60 @@
break;
case LINUX_REBOOT_CMD_RESTART2:
- if (strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1) < 0) {
- unlock_kernel();
+ {
+ char *buffer;
+ buffer = kmalloc(256, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+ if (strncpy_from_user(buffer, arg, 255) < 0) {
+ kfree(buffer);
return -EFAULT;
}
- buffer[sizeof(buffer) - 1] = '\0';
-
+ buffer[255] = '\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);
+ printk(KERN_EMERG "Restarting system with command '%s'.\n",
+ buffer);
machine_restart(buffer);
+ unlock_kernel();
+ kfree(buffer);
break;
-
+ }
#ifdef CONFIG_KEXEC
case LINUX_REBOOT_CMD_KEXEC:
{
struct kimage *image;
image = xchg(&kexec_image, 0);
if (!image) {
- unlock_kernel();
return -EINVAL;
}
+ lock_kernel();
notifier_call_chain(&reboot_notifier_list, SYS_RESTART, NULL);
system_state = SYSTEM_RESTART;
device_shutdown();
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();
- unlock_kernel();
- return ret;
- }
+ {
+ int ret;
+ lock_kernel();
+ ret = software_suspend();
+ unlock_kernel();
+ return ret;
+ }
#endif
default:
- unlock_kernel();
return -EINVAL;
}
- unlock_kernel();
return 0;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] Reduce stack usage in sys.c
2005-03-31 7:44 [PATCH] Reduce stack usage in sys.c Yum Rayan
@ 2005-03-31 8:29 ` Jeff Garzik
2005-04-02 7:05 ` [PATCH] Cleanup locking in sys_reboot() (was Re: [PATCH] Reduce stack usage in sys.c) Yum Rayan
2005-03-31 13:01 ` [PATCH] Reduce stack usage in sys.c Jörn Engel
1 sibling, 1 reply; 4+ messages in thread
From: Jeff Garzik @ 2005-03-31 8:29 UTC (permalink / raw)
To: Yum Rayan; +Cc: linux-kernel
Yum Rayan wrote:
> Attempt to reduce stack usage in sys.c (linux-2.6.12-rc1-mm3). Stack
> usage was noted using checkstack.pl. Specifically
>
> Before patch
> ------------
> sys_reboot - 256
>
> After patch
> -----------
> sys_reboot - none (register usage only)
>
> Along the way, wrap code to 80 column width and cleanup lock usage.
Your "cleanup lock usage" increases the number of lock_kernel() calls
quite a bit, which is not really a cleanup but simply bloat.
Seperate out your patches; don't sneak these supposed-cleanups into
stack uage patches.
Jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] Cleanup locking in sys_reboot() (was Re: [PATCH] Reduce stack usage in sys.c)
2005-03-31 8:29 ` Jeff Garzik
@ 2005-04-02 7:05 ` Yum Rayan
0 siblings, 0 replies; 4+ messages in thread
From: Yum Rayan @ 2005-04-02 7:05 UTC (permalink / raw)
To: Jeff Garzik; +Cc: linux-kernel
On Mar 31, 2005 12:29 AM, Jeff Garzik <jgarzik@pobox.com> 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@gmail.com>
--- 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;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Reduce stack usage in sys.c
2005-03-31 7:44 [PATCH] Reduce stack usage in sys.c Yum Rayan
2005-03-31 8:29 ` Jeff Garzik
@ 2005-03-31 13:01 ` Jörn Engel
1 sibling, 0 replies; 4+ messages in thread
From: Jörn Engel @ 2005-03-31 13:01 UTC (permalink / raw)
To: Yum Rayan; +Cc: linux-kernel
On Wed, 30 March 2005 23:44:38 -0800, Yum Rayan wrote:
> ------------
> sys_reboot - 256
Also not part of any deep stack trace I found.
Jörn
--
There is no worse hell than that provided by the regrets
for wasted opportunities.
-- Andre-Louis Moreau in Scarabouche
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2005-04-02 7:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-03-31 7:44 [PATCH] Reduce stack usage in sys.c Yum Rayan
2005-03-31 8:29 ` Jeff Garzik
2005-04-02 7:05 ` [PATCH] Cleanup locking in sys_reboot() (was Re: [PATCH] Reduce stack usage in sys.c) Yum Rayan
2005-03-31 13:01 ` [PATCH] Reduce stack usage in sys.c Jörn Engel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.