[PATCH] Reduce stack usage in sys.c

[PATCH] Reduce stack usage in sys.c

[Yum Rayan]

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;
 }

Re: [PATCH] Reduce stack usage in sys.c

[Jeff Garzik]

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

Re: [PATCH] Reduce stack usage in sys.c

[Jörn Engel]

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

[PATCH] Cleanup locking in sys_reboot() (was Re: [PATCH] Reduce stack usage in sys.c)

[Yum Rayan]

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;
 }