Reduce stack usage in sys.c

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.

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

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

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

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

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