2013-04-15 17:12:31

by Robin Holt

[permalink] [raw]
Subject: [Patch -v3 0/4] Shutdown from reboot_cpuid without stopping other cpus.

We recently noticed that reboot of a 1024 cpu machine takes approx 16
minutes of just stopping the cpus. The slowdown was tracked to commit
f96972f.

The current implementation does all the work of hot removing the cpus
before halting the system. We are switching to just migrating to the
reboot_cpuid and then continuing with shutdown/reboot.

The patch set is broken into four parts. The first is planned for
the stable release. The others move the halt/shutdown/reboot related
files to their own kernel/reboot.c file and then introduce the kernel
boot parameter.


2013-04-15 17:14:32

by Robin Holt

[permalink] [raw]
Subject: [Patch -v3 1/4] Migrate shutdown/reboot to boot cpu.

We recently noticed that reboot of a 1024 cpu machine takes approx 16
minutes of just stopping the cpus. The slowdown was tracked to commit
f96972f.

The current implementation does all the work of hot removing the cpus
before halting the system. We are switching to just migrating to the
boot cpu and then continuing with shutdown/reboot.

This also has the effect of not breaking x86's command line parameter for
specifying the reboot cpu. Note, this code was shamelessly copied from
arch/x86/kernel/reboot.c with bits removed pertaining to the reboot_cpu
command line parameter.

Signed-off-by: Robin Holt <[email protected]>
To: Shawn Guo <[email protected]>
To: Ingo Molnar <[email protected]>
To: Russ Anderson <[email protected]>
To: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: "[email protected]" <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: <[email protected]>

---

Changes since -v1.
- Set PF_THREAD_BOUND before migrating to eliminate potential race.
- Modified kernel_power_off to also migrate instead of using
disable_nonboot_cpus().
---
kernel/sys.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 0da73cf..5ef7aa2 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -357,6 +357,22 @@ int unregister_reboot_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(unregister_reboot_notifier);

+void migrate_to_reboot_cpu(void)
+{
+ /* The boot cpu is always logical cpu 0 */
+ int reboot_cpu_id = 0;
+
+ /* Make certain the cpu I'm about to reboot on is online */
+ if (!cpu_online(reboot_cpu_id))
+ reboot_cpu_id = smp_processor_id();
+
+ /* Prevent races with other tasks migrating this task. */
+ current->flags |= PF_THREAD_BOUND;
+
+ /* Make certain I only run on the appropriate processor */
+ set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
+}
+
/**
* kernel_restart - reboot the system
* @cmd: pointer to buffer containing command to execute for restart
@@ -368,7 +384,7 @@ EXPORT_SYMBOL(unregister_reboot_notifier);
void kernel_restart(char *cmd)
{
kernel_restart_prepare(cmd);
- disable_nonboot_cpus();
+ migrate_to_reboot_cpu();
syscore_shutdown();
if (!cmd)
printk(KERN_EMERG "Restarting system.\n");
@@ -395,7 +411,7 @@ static void kernel_shutdown_prepare(enum system_states state)
void kernel_halt(void)
{
kernel_shutdown_prepare(SYSTEM_HALT);
- disable_nonboot_cpus();
+ migrate_to_reboot_cpu();
syscore_shutdown();
printk(KERN_EMERG "System halted.\n");
kmsg_dump(KMSG_DUMP_HALT);
@@ -414,7 +430,7 @@ void kernel_power_off(void)
kernel_shutdown_prepare(SYSTEM_POWER_OFF);
if (pm_power_off_prepare)
pm_power_off_prepare();
- disable_nonboot_cpus();
+ migrate_to_reboot_cpu();
syscore_shutdown();
printk(KERN_EMERG "Power down.\n");
kmsg_dump(KMSG_DUMP_POWEROFF);
--
1.8.1.2

2013-04-15 17:15:20

by Robin Holt

[permalink] [raw]
Subject: [Patch -v3 2/4] Move shutdown/reboot related functions to kernel/reboot.c

This patch is preparatory. It moves reboot related syscall, etc
functions from kernel/sys.c to kernel/reboot.c.

Signed-off-by: Robin Holt <[email protected]>
To: Ingo Molnar <[email protected]>
To: Russ Anderson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: "[email protected]" <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Thomas Gleixner <[email protected]>
---
kernel/Makefile | 2 +-
kernel/reboot.c | 328 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
kernel/sys.c | 315 -----------------------------------------------------
3 files changed, 329 insertions(+), 316 deletions(-)
create mode 100644 kernel/reboot.c

diff --git a/kernel/Makefile b/kernel/Makefile
index bbde5f1..5c2cef7 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -9,7 +9,7 @@ obj-y = fork.o exec_domain.o panic.o printk.o \
rcupdate.o extable.o params.o posix-timers.o \
kthread.o wait.o sys_ni.o posix-cpu-timers.o mutex.o \
hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \
- notifier.o ksysfs.o cred.o \
+ notifier.o ksysfs.o cred.o reboot.o \
async.o range.o groups.o lglock.o smpboot.o

ifdef CONFIG_FUNCTION_TRACER
diff --git a/kernel/reboot.c b/kernel/reboot.c
new file mode 100644
index 0000000..00a82ed
--- /dev/null
+++ b/kernel/reboot.c
@@ -0,0 +1,328 @@
+/*
+ * linux/kernel/sys.c
+ *
+ * Copyright (C) 2013 Linus Torvalds
+ */
+
+#include <linux/export.h>
+#include <linux/kexec.h>
+#include <linux/kmod.h>
+#include <linux/kmsg_dump.h>
+#include <linux/reboot.h>
+#include <linux/syscalls.h>
+#include <linux/syscore_ops.h>
+
+/**
+ * emergency_restart - reboot the system
+ *
+ * Without shutting down any hardware or taking any locks
+ * reboot the system. This is called when we know we are in
+ * trouble so this is our best effort to reboot. This is
+ * safe to call in interrupt context.
+ */
+void emergency_restart(void)
+{
+ kmsg_dump(KMSG_DUMP_EMERG);
+ machine_emergency_restart();
+}
+EXPORT_SYMBOL_GPL(emergency_restart);
+
+void kernel_restart_prepare(char *cmd)
+{
+ blocking_notifier_call_chain(&reboot_notifier_list, SYS_RESTART, cmd);
+ system_state = SYSTEM_RESTART;
+ usermodehelper_disable();
+ device_shutdown();
+}
+
+/**
+ * register_reboot_notifier - Register function to be called at reboot time
+ * @nb: Info about notifier function to be called
+ *
+ * Registers a function with the list of functions
+ * to be called at reboot time.
+ *
+ * Currently always returns zero, as blocking_notifier_chain_register()
+ * always returns zero.
+ */
+int register_reboot_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&reboot_notifier_list, nb);
+}
+EXPORT_SYMBOL(register_reboot_notifier);
+
+/**
+ * unregister_reboot_notifier - Unregister previously registered reboot notifier
+ * @nb: Hook to be unregistered
+ *
+ * Unregisters a previously registered reboot
+ * notifier function.
+ *
+ * Returns zero on success, or %-ENOENT on failure.
+ */
+int unregister_reboot_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&reboot_notifier_list, nb);
+}
+EXPORT_SYMBOL(unregister_reboot_notifier);
+
+void migrate_to_reboot_cpu(void)
+{
+ /* The boot cpu is always logical cpu 0 */
+ int reboot_cpu_id = 0;
+
+ /* Make certain the cpu I'm about to reboot on is online */
+ if (!cpu_online(reboot_cpu_id))
+ reboot_cpu_id = smp_processor_id();
+
+ /* Prevent races with other tasks migrating this task. */
+ current->flags |= PF_THREAD_BOUND;
+
+ /* Make certain I only run on the appropriate processor */
+ set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
+}
+
+/**
+ * kernel_restart - reboot the system
+ * @cmd: pointer to buffer containing command to execute for restart
+ * or %NULL
+ *
+ * Shutdown everything and perform a clean reboot.
+ * This is not safe to call in interrupt context.
+ */
+void kernel_restart(char *cmd)
+{
+ kernel_restart_prepare(cmd);
+ migrate_to_reboot_cpu();
+ syscore_shutdown();
+ if (!cmd)
+ printk(KERN_EMERG "Restarting system.\n");
+ else
+ printk(KERN_EMERG "Restarting system with command '%s'.\n", cmd);
+ kmsg_dump(KMSG_DUMP_RESTART);
+ machine_restart(cmd);
+}
+EXPORT_SYMBOL_GPL(kernel_restart);
+
+static void kernel_shutdown_prepare(enum system_states state)
+{
+ blocking_notifier_call_chain(&reboot_notifier_list,
+ (state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
+ system_state = state;
+ usermodehelper_disable();
+ device_shutdown();
+}
+/**
+ * kernel_halt - halt the system
+ *
+ * Shutdown everything and perform a clean system halt.
+ */
+void kernel_halt(void)
+{
+ kernel_shutdown_prepare(SYSTEM_HALT);
+ migrate_to_reboot_cpu();
+ syscore_shutdown();
+ printk(KERN_EMERG "System halted.\n");
+ kmsg_dump(KMSG_DUMP_HALT);
+ machine_halt();
+}
+
+EXPORT_SYMBOL_GPL(kernel_halt);
+
+/**
+ * kernel_power_off - power_off the system
+ *
+ * Shutdown everything and perform a clean system power_off.
+ */
+void kernel_power_off(void)
+{
+ kernel_shutdown_prepare(SYSTEM_POWER_OFF);
+ if (pm_power_off_prepare)
+ pm_power_off_prepare();
+ migrate_to_reboot_cpu();
+ syscore_shutdown();
+ printk(KERN_EMERG "Power down.\n");
+ kmsg_dump(KMSG_DUMP_POWEROFF);
+ machine_power_off();
+}
+EXPORT_SYMBOL_GPL(kernel_power_off);
+
+static DEFINE_MUTEX(reboot_mutex);
+
+/*
+ * Reboot system call: for obvious reasons only root may call it,
+ * and even root needs to set up some magic numbers in the registers
+ * so that some mistake won't make this reboot the whole machine.
+ * You can also set the meaning of the ctrl-alt-del-key here.
+ *
+ * reboot doesn't sync: do that yourself before calling this.
+ */
+SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
+ void __user *, arg)
+{
+ struct pid_namespace *pid_ns = task_active_pid_ns(current);
+ char buffer[256];
+ int ret = 0;
+
+ /* We only trust the superuser with rebooting the system. */
+ if (!ns_capable(pid_ns->user_ns, CAP_SYS_BOOT))
+ return -EPERM;
+
+ /* For safety, we require "magic" arguments. */
+ if (magic1 != LINUX_REBOOT_MAGIC1 ||
+ (magic2 != LINUX_REBOOT_MAGIC2 &&
+ magic2 != LINUX_REBOOT_MAGIC2A &&
+ magic2 != LINUX_REBOOT_MAGIC2B &&
+ magic2 != LINUX_REBOOT_MAGIC2C))
+ return -EINVAL;
+
+ /*
+ * If pid namespaces are enabled and the current task is in a child
+ * pid_namespace, the command is handled by reboot_pid_ns() which will
+ * call do_exit().
+ */
+ ret = reboot_pid_ns(pid_ns, cmd);
+ if (ret)
+ return ret;
+
+ /* Instead of trying to make the power_off code look like
+ * halt when pm_power_off is not set do it the easy way.
+ */
+ if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
+ cmd = LINUX_REBOOT_CMD_HALT;
+
+ mutex_lock(&reboot_mutex);
+ switch (cmd) {
+ case LINUX_REBOOT_CMD_RESTART:
+ kernel_restart(NULL);
+ break;
+
+ case LINUX_REBOOT_CMD_CAD_ON:
+ C_A_D = 1;
+ break;
+
+ case LINUX_REBOOT_CMD_CAD_OFF:
+ C_A_D = 0;
+ break;
+
+ case LINUX_REBOOT_CMD_HALT:
+ kernel_halt();
+ do_exit(0);
+ panic("cannot halt");
+
+ case LINUX_REBOOT_CMD_POWER_OFF:
+ kernel_power_off();
+ do_exit(0);
+ break;
+
+ case LINUX_REBOOT_CMD_RESTART2:
+ if (strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1) < 0) {
+ ret = -EFAULT;
+ break;
+ }
+ buffer[sizeof(buffer) - 1] = '\0';
+
+ kernel_restart(buffer);
+ break;
+
+#ifdef CONFIG_KEXEC
+ case LINUX_REBOOT_CMD_KEXEC:
+ ret = kernel_kexec();
+ break;
+#endif
+
+#ifdef CONFIG_HIBERNATION
+ case LINUX_REBOOT_CMD_SW_SUSPEND:
+ ret = hibernate();
+ break;
+#endif
+
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ mutex_unlock(&reboot_mutex);
+ return ret;
+}
+
+static void deferred_cad(struct work_struct *dummy)
+{
+ kernel_restart(NULL);
+}
+
+/*
+ * This function gets called by ctrl-alt-del - ie the keyboard interrupt.
+ * As it's called within an interrupt, it may NOT sync: the only choice
+ * is whether to reboot at once, or just ignore the ctrl-alt-del.
+ */
+void ctrl_alt_del(void)
+{
+ static DECLARE_WORK(cad_work, deferred_cad);
+
+ if (C_A_D)
+ schedule_work(&cad_work);
+ else
+ kill_cad_pid(SIGINT, 1);
+}
+
+char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff";
+
+static int __orderly_poweroff(bool force)
+{
+ char **argv;
+ static char *envp[] = {
+ "HOME=/",
+ "PATH=/sbin:/bin:/usr/sbin:/usr/bin",
+ NULL
+ };
+ int ret;
+
+ argv = argv_split(GFP_KERNEL, poweroff_cmd, NULL);
+ if (argv) {
+ ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
+ argv_free(argv);
+ } else {
+ printk(KERN_WARNING "%s failed to allocate memory for \"%s\"\n",
+ __func__, poweroff_cmd);
+ ret = -ENOMEM;
+ }
+
+ if (ret && force) {
+ printk(KERN_WARNING "Failed to start orderly shutdown: "
+ "forcing the issue\n");
+ /*
+ * I guess this should try to kick off some daemon to sync and
+ * poweroff asap. Or not even bother syncing if we're doing an
+ * emergency shutdown?
+ */
+ emergency_sync();
+ kernel_power_off();
+ }
+
+ return ret;
+}
+
+static bool poweroff_force;
+
+static void poweroff_work_func(struct work_struct *work)
+{
+ __orderly_poweroff(poweroff_force);
+}
+
+static DECLARE_WORK(poweroff_work, poweroff_work_func);
+
+/**
+ * orderly_poweroff - Trigger an orderly system poweroff
+ * @force: force poweroff if command execution fails
+ *
+ * This may be called from any context to trigger a system shutdown.
+ * If the orderly shutdown fails, it will force an immediate shutdown.
+ */
+int orderly_poweroff(bool force)
+{
+ if (force) /* do not override the pending "true" */
+ poweroff_force = true;
+ schedule_work(&poweroff_work);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(orderly_poweroff);
diff --git a/kernel/sys.c b/kernel/sys.c
index 5ef7aa2..76b9a93 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -303,259 +303,6 @@ out_unlock:
return retval;
}

-/**
- * emergency_restart - reboot the system
- *
- * Without shutting down any hardware or taking any locks
- * reboot the system. This is called when we know we are in
- * trouble so this is our best effort to reboot. This is
- * safe to call in interrupt context.
- */
-void emergency_restart(void)
-{
- kmsg_dump(KMSG_DUMP_EMERG);
- machine_emergency_restart();
-}
-EXPORT_SYMBOL_GPL(emergency_restart);
-
-void kernel_restart_prepare(char *cmd)
-{
- blocking_notifier_call_chain(&reboot_notifier_list, SYS_RESTART, cmd);
- system_state = SYSTEM_RESTART;
- usermodehelper_disable();
- device_shutdown();
-}
-
-/**
- * register_reboot_notifier - Register function to be called at reboot time
- * @nb: Info about notifier function to be called
- *
- * Registers a function with the list of functions
- * to be called at reboot time.
- *
- * Currently always returns zero, as blocking_notifier_chain_register()
- * always returns zero.
- */
-int register_reboot_notifier(struct notifier_block *nb)
-{
- return blocking_notifier_chain_register(&reboot_notifier_list, nb);
-}
-EXPORT_SYMBOL(register_reboot_notifier);
-
-/**
- * unregister_reboot_notifier - Unregister previously registered reboot notifier
- * @nb: Hook to be unregistered
- *
- * Unregisters a previously registered reboot
- * notifier function.
- *
- * Returns zero on success, or %-ENOENT on failure.
- */
-int unregister_reboot_notifier(struct notifier_block *nb)
-{
- return blocking_notifier_chain_unregister(&reboot_notifier_list, nb);
-}
-EXPORT_SYMBOL(unregister_reboot_notifier);
-
-void migrate_to_reboot_cpu(void)
-{
- /* The boot cpu is always logical cpu 0 */
- int reboot_cpu_id = 0;
-
- /* Make certain the cpu I'm about to reboot on is online */
- if (!cpu_online(reboot_cpu_id))
- reboot_cpu_id = smp_processor_id();
-
- /* Prevent races with other tasks migrating this task. */
- current->flags |= PF_THREAD_BOUND;
-
- /* Make certain I only run on the appropriate processor */
- set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
-}
-
-/**
- * kernel_restart - reboot the system
- * @cmd: pointer to buffer containing command to execute for restart
- * or %NULL
- *
- * Shutdown everything and perform a clean reboot.
- * This is not safe to call in interrupt context.
- */
-void kernel_restart(char *cmd)
-{
- kernel_restart_prepare(cmd);
- migrate_to_reboot_cpu();
- syscore_shutdown();
- if (!cmd)
- printk(KERN_EMERG "Restarting system.\n");
- else
- printk(KERN_EMERG "Restarting system with command '%s'.\n", cmd);
- kmsg_dump(KMSG_DUMP_RESTART);
- machine_restart(cmd);
-}
-EXPORT_SYMBOL_GPL(kernel_restart);
-
-static void kernel_shutdown_prepare(enum system_states state)
-{
- blocking_notifier_call_chain(&reboot_notifier_list,
- (state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
- system_state = state;
- usermodehelper_disable();
- device_shutdown();
-}
-/**
- * kernel_halt - halt the system
- *
- * Shutdown everything and perform a clean system halt.
- */
-void kernel_halt(void)
-{
- kernel_shutdown_prepare(SYSTEM_HALT);
- migrate_to_reboot_cpu();
- syscore_shutdown();
- printk(KERN_EMERG "System halted.\n");
- kmsg_dump(KMSG_DUMP_HALT);
- machine_halt();
-}
-
-EXPORT_SYMBOL_GPL(kernel_halt);
-
-/**
- * kernel_power_off - power_off the system
- *
- * Shutdown everything and perform a clean system power_off.
- */
-void kernel_power_off(void)
-{
- kernel_shutdown_prepare(SYSTEM_POWER_OFF);
- if (pm_power_off_prepare)
- pm_power_off_prepare();
- migrate_to_reboot_cpu();
- syscore_shutdown();
- printk(KERN_EMERG "Power down.\n");
- kmsg_dump(KMSG_DUMP_POWEROFF);
- machine_power_off();
-}
-EXPORT_SYMBOL_GPL(kernel_power_off);
-
-static DEFINE_MUTEX(reboot_mutex);
-
-/*
- * Reboot system call: for obvious reasons only root may call it,
- * and even root needs to set up some magic numbers in the registers
- * so that some mistake won't make this reboot the whole machine.
- * You can also set the meaning of the ctrl-alt-del-key here.
- *
- * reboot doesn't sync: do that yourself before calling this.
- */
-SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
- void __user *, arg)
-{
- struct pid_namespace *pid_ns = task_active_pid_ns(current);
- char buffer[256];
- int ret = 0;
-
- /* We only trust the superuser with rebooting the system. */
- if (!ns_capable(pid_ns->user_ns, CAP_SYS_BOOT))
- return -EPERM;
-
- /* For safety, we require "magic" arguments. */
- if (magic1 != LINUX_REBOOT_MAGIC1 ||
- (magic2 != LINUX_REBOOT_MAGIC2 &&
- magic2 != LINUX_REBOOT_MAGIC2A &&
- magic2 != LINUX_REBOOT_MAGIC2B &&
- magic2 != LINUX_REBOOT_MAGIC2C))
- return -EINVAL;
-
- /*
- * If pid namespaces are enabled and the current task is in a child
- * pid_namespace, the command is handled by reboot_pid_ns() which will
- * call do_exit().
- */
- ret = reboot_pid_ns(pid_ns, cmd);
- if (ret)
- return ret;
-
- /* Instead of trying to make the power_off code look like
- * halt when pm_power_off is not set do it the easy way.
- */
- if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
- cmd = LINUX_REBOOT_CMD_HALT;
-
- mutex_lock(&reboot_mutex);
- switch (cmd) {
- case LINUX_REBOOT_CMD_RESTART:
- kernel_restart(NULL);
- break;
-
- case LINUX_REBOOT_CMD_CAD_ON:
- C_A_D = 1;
- break;
-
- case LINUX_REBOOT_CMD_CAD_OFF:
- C_A_D = 0;
- break;
-
- case LINUX_REBOOT_CMD_HALT:
- kernel_halt();
- do_exit(0);
- panic("cannot halt");
-
- case LINUX_REBOOT_CMD_POWER_OFF:
- kernel_power_off();
- do_exit(0);
- break;
-
- case LINUX_REBOOT_CMD_RESTART2:
- if (strncpy_from_user(&buffer[0], arg, sizeof(buffer) - 1) < 0) {
- ret = -EFAULT;
- break;
- }
- buffer[sizeof(buffer) - 1] = '\0';
-
- kernel_restart(buffer);
- break;
-
-#ifdef CONFIG_KEXEC
- case LINUX_REBOOT_CMD_KEXEC:
- ret = kernel_kexec();
- break;
-#endif
-
-#ifdef CONFIG_HIBERNATION
- case LINUX_REBOOT_CMD_SW_SUSPEND:
- ret = hibernate();
- break;
-#endif
-
- default:
- ret = -EINVAL;
- break;
- }
- mutex_unlock(&reboot_mutex);
- return ret;
-}
-
-static void deferred_cad(struct work_struct *dummy)
-{
- kernel_restart(NULL);
-}
-
-/*
- * This function gets called by ctrl-alt-del - ie the keyboard interrupt.
- * As it's called within an interrupt, it may NOT sync: the only choice
- * is whether to reboot at once, or just ignore the ctrl-alt-del.
- */
-void ctrl_alt_del(void)
-{
- static DECLARE_WORK(cad_work, deferred_cad);
-
- if (C_A_D)
- schedule_work(&cad_work);
- else
- kill_cad_pid(SIGINT, 1);
-}
-
/*
* Unprivileged users may change the real gid to the effective gid
* or vice versa. (BSD-style)
@@ -2199,65 +1946,3 @@ SYSCALL_DEFINE3(getcpu, unsigned __user *, cpup, unsigned __user *, nodep,
err |= put_user(cpu_to_node(cpu), nodep);
return err ? -EFAULT : 0;
}
-
-char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff";
-
-static int __orderly_poweroff(bool force)
-{
- char **argv;
- static char *envp[] = {
- "HOME=/",
- "PATH=/sbin:/bin:/usr/sbin:/usr/bin",
- NULL
- };
- int ret;
-
- argv = argv_split(GFP_KERNEL, poweroff_cmd, NULL);
- if (argv) {
- ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
- argv_free(argv);
- } else {
- printk(KERN_WARNING "%s failed to allocate memory for \"%s\"\n",
- __func__, poweroff_cmd);
- ret = -ENOMEM;
- }
-
- if (ret && force) {
- printk(KERN_WARNING "Failed to start orderly shutdown: "
- "forcing the issue\n");
- /*
- * I guess this should try to kick off some daemon to sync and
- * poweroff asap. Or not even bother syncing if we're doing an
- * emergency shutdown?
- */
- emergency_sync();
- kernel_power_off();
- }
-
- return ret;
-}
-
-static bool poweroff_force;
-
-static void poweroff_work_func(struct work_struct *work)
-{
- __orderly_poweroff(poweroff_force);
-}
-
-static DECLARE_WORK(poweroff_work, poweroff_work_func);
-
-/**
- * orderly_poweroff - Trigger an orderly system poweroff
- * @force: force poweroff if command execution fails
- *
- * This may be called from any context to trigger a system shutdown.
- * If the orderly shutdown fails, it will force an immediate shutdown.
- */
-int orderly_poweroff(bool force)
-{
- if (force) /* do not override the pending "true" */
- poweroff_force = true;
- schedule_work(&poweroff_work);
- return 0;
-}
-EXPORT_SYMBOL_GPL(orderly_poweroff);
--
1.8.1.2

2013-04-15 17:16:07

by Robin Holt

[permalink] [raw]
Subject: [Patch -v3 3/4] checkpatch.pl the new kernel/reboot.c file.


I did allow the remaining 81 character line behind. It did not seem
like it was worth changing. Otherwise, it now passes checkpatch.pl.

Signed-off-by: Robin Holt <[email protected]>
To: Ingo Molnar <[email protected]>
To: Russ Anderson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: "[email protected]" <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Thomas Gleixner <[email protected]>
---
kernel/reboot.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 00a82ed..187a0d8 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -96,9 +96,9 @@ void kernel_restart(char *cmd)
migrate_to_reboot_cpu();
syscore_shutdown();
if (!cmd)
- printk(KERN_EMERG "Restarting system.\n");
+ pr_emerg("Restarting system.\n");
else
- printk(KERN_EMERG "Restarting system with command '%s'.\n", cmd);
+ pr_emerg("Restarting system with command '%s'.\n", cmd);
kmsg_dump(KMSG_DUMP_RESTART);
machine_restart(cmd);
}
@@ -107,7 +107,7 @@ EXPORT_SYMBOL_GPL(kernel_restart);
static void kernel_shutdown_prepare(enum system_states state)
{
blocking_notifier_call_chain(&reboot_notifier_list,
- (state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
+ (state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
system_state = state;
usermodehelper_disable();
device_shutdown();
@@ -122,11 +122,10 @@ void kernel_halt(void)
kernel_shutdown_prepare(SYSTEM_HALT);
migrate_to_reboot_cpu();
syscore_shutdown();
- printk(KERN_EMERG "System halted.\n");
+ pr_emerg("System halted.\n");
kmsg_dump(KMSG_DUMP_HALT);
machine_halt();
}
-
EXPORT_SYMBOL_GPL(kernel_halt);

/**
@@ -141,7 +140,7 @@ void kernel_power_off(void)
pm_power_off_prepare();
migrate_to_reboot_cpu();
syscore_shutdown();
- printk(KERN_EMERG "Power down.\n");
+ pr_emerg("Power down.\n");
kmsg_dump(KMSG_DUMP_POWEROFF);
machine_power_off();
}
@@ -170,10 +169,10 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,

/* For safety, we require "magic" arguments. */
if (magic1 != LINUX_REBOOT_MAGIC1 ||
- (magic2 != LINUX_REBOOT_MAGIC2 &&
- magic2 != LINUX_REBOOT_MAGIC2A &&
+ (magic2 != LINUX_REBOOT_MAGIC2 &&
+ magic2 != LINUX_REBOOT_MAGIC2A &&
magic2 != LINUX_REBOOT_MAGIC2B &&
- magic2 != LINUX_REBOOT_MAGIC2C))
+ magic2 != LINUX_REBOOT_MAGIC2C))
return -EINVAL;

/*
@@ -282,14 +281,13 @@ static int __orderly_poweroff(bool force)
ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
argv_free(argv);
} else {
- printk(KERN_WARNING "%s failed to allocate memory for \"%s\"\n",
+ pr_warn("%s failed to allocate memory for \"%s\"\n",
__func__, poweroff_cmd);
ret = -ENOMEM;
}

if (ret && force) {
- printk(KERN_WARNING "Failed to start orderly shutdown: "
- "forcing the issue\n");
+ pr_warn("Failed to start orderly shutdown: forcing the issue\n");
/*
* I guess this should try to kick off some daemon to sync and
* poweroff asap. Or not even bother syncing if we're doing an
--
1.8.1.2

2013-04-15 17:16:38

by Robin Holt

[permalink] [raw]
Subject: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.


Moving the reboot=s<##> parameter for x86 to a kernel parameter
proper. I did not find any other arch that was specifying the
reboot cpu.

Signed-off-by: Robin Holt <[email protected]>
To: Ingo Molnar <[email protected]>
To: Russ Anderson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: "[email protected]" <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Thomas Gleixner <[email protected]>
---
Documentation/kernel-parameters.txt | 2 ++
arch/x86/kernel/reboot.c | 44 ++++---------------------------------
kernel/reboot.c | 8 ++++++-
3 files changed, 13 insertions(+), 41 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4609e81..11ebb48 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2597,6 +2597,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Format: <reboot_mode>[,<reboot_mode2>[,...]]
See arch/*/kernel/reboot.c or arch/*/kernel/process.c

+ reboot_cpuid= [KNL] cpuid to use for reboot/halt, etc operations.
+
relax_domain_level=
[KNL, SMP] Set scheduler's default relax_domain_level.
See Documentation/cgroups/cpusets.txt.
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 76fa1e9..39af130 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -49,10 +49,6 @@ int reboot_force;
*/
static int reboot_default = 1;

-#ifdef CONFIG_SMP
-static int reboot_cpu = -1;
-#endif
-
/*
* This is set if we need to go through the 'emergency' path.
* When machine_emergency_restart() is called, we may be on
@@ -64,11 +60,10 @@ static int reboot_emergency;
bool port_cf9_safe = false;

/*
- * reboot=b[ios] | s[mp] | t[riple] | k[bd] | e[fi] [, [w]arm | [c]old] | p[ci]
+ * reboot=b[ios] | t[riple] | k[bd] | e[fi] [, [w]arm | [c]old] | p[ci]
* warm Don't set the cold reboot flag
* cold Set the cold reboot flag
* bios Reboot by jumping through the BIOS
- * smp Reboot by executing reset on BSP or other CPU
* triple Force a triple fault (init)
* kbd Use the keyboard controller. cold reset (default)
* acpi Use the RESET_REG in the FADT
@@ -95,21 +90,6 @@ static int __init reboot_setup(char *str)
reboot_mode = 0;
break;

-#ifdef CONFIG_SMP
- case 's':
- if (isdigit(*(str+1))) {
- reboot_cpu = (int) (*(str+1) - '0');
- if (isdigit(*(str+2)))
- reboot_cpu = reboot_cpu*10 + (int)(*(str+2) - '0');
- }
- /*
- * We will leave sorting out the final value
- * when we are ready to reboot, since we might not
- * have detected BSP APIC ID or smp_num_cpu
- */
- break;
-#endif /* CONFIG_SMP */
-
case 'b':
case 'a':
case 'k':
@@ -614,26 +594,10 @@ void native_machine_shutdown(void)
{
/* Stop the cpus and apics */
#ifdef CONFIG_SMP
-
- /* The boot cpu is always logical cpu 0 */
- int reboot_cpu_id = 0;
-
- /* See if there has been given a command line override */
- if ((reboot_cpu != -1) && (reboot_cpu < nr_cpu_ids) &&
- cpu_online(reboot_cpu))
- reboot_cpu_id = reboot_cpu;
-
- /* Make certain the cpu I'm about to reboot on is online */
- if (!cpu_online(reboot_cpu_id))
- reboot_cpu_id = smp_processor_id();
-
- /* Make certain I only run on the appropriate processor */
- set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
-
/*
- * O.K Now that I'm on the appropriate processor, stop all of the
- * others. Also disable the local irq to not receive the per-cpu
- * timer interrupt which may trigger scheduler's load balance.
+ * Stop all of the others. Also disable the local irq to
+ * not receive the per-cpu timer interrupt which may trigger
+ * scheduler's load balance.
*/
local_irq_disable();
stop_other_cpus();
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 187a0d8..9fbab07 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -8,6 +8,7 @@
#include <linux/kexec.h>
#include <linux/kmod.h>
#include <linux/kmsg_dump.h>
+#include <linux/moduleparam.h>
#include <linux/reboot.h>
#include <linux/syscalls.h>
#include <linux/syscore_ops.h>
@@ -66,13 +67,18 @@ int unregister_reboot_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(unregister_reboot_notifier);

+int reboot_cpuid;
+core_param(reboot_cpuid, reboot_cpuid, int, 0644);
+
void migrate_to_reboot_cpu(void)
{
/* The boot cpu is always logical cpu 0 */
- int reboot_cpu_id = 0;
+ int reboot_cpu_id = reboot_cpuid;

/* Make certain the cpu I'm about to reboot on is online */
if (!cpu_online(reboot_cpu_id))
+ reboot_cpu_id = 0;
+ if (!cpu_online(reboot_cpu_id))
reboot_cpu_id = smp_processor_id();

/* Prevent races with other tasks migrating this task. */
--
1.8.1.2

2013-04-15 17:20:34

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.

Um... doesn't this break anyone who currently depends on this?

Robin Holt <[email protected]> wrote:

>
>Moving the reboot=s<##> parameter for x86 to a kernel parameter
>proper. I did not find any other arch that was specifying the
>reboot cpu.
>
>Signed-off-by: Robin Holt <[email protected]>
>To: Ingo Molnar <[email protected]>
>To: Russ Anderson <[email protected]>
>Cc: Shawn Guo <[email protected]>
>Cc: Oleg Nesterov <[email protected]>
>Cc: Andrew Morton <[email protected]>
>Cc: "H. Peter Anvin" <[email protected]>
>Cc: Lai Jiangshan <[email protected]>
>Cc: Linus Torvalds <[email protected]>
>Cc: Linux Kernel Mailing List <[email protected]>
>Cc: Michel Lespinasse <[email protected]>
>Cc: Oleg Nesterov <[email protected]>
>Cc: "Paul E. McKenney" <[email protected]>
>Cc: Paul Mackerras <[email protected]>
>Cc: Peter Zijlstra <[email protected]>
>Cc: Robin Holt <[email protected]>
>Cc: "[email protected]" <[email protected]>
>Cc: Tejun Heo <[email protected]>
>Cc: the arch/x86 maintainers <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>---
> Documentation/kernel-parameters.txt | 2 ++
>arch/x86/kernel/reboot.c | 44
>++++---------------------------------
> kernel/reboot.c | 8 ++++++-
> 3 files changed, 13 insertions(+), 41 deletions(-)
>
>diff --git a/Documentation/kernel-parameters.txt
>b/Documentation/kernel-parameters.txt
>index 4609e81..11ebb48 100644
>--- a/Documentation/kernel-parameters.txt
>+++ b/Documentation/kernel-parameters.txt
>@@ -2597,6 +2597,8 @@ bytes respectively. Such letter suffixes can also
>be entirely omitted.
> Format: <reboot_mode>[,<reboot_mode2>[,...]]
> See arch/*/kernel/reboot.c or arch/*/kernel/process.c
>
>+ reboot_cpuid= [KNL] cpuid to use for reboot/halt, etc operations.
>+
> relax_domain_level=
> [KNL, SMP] Set scheduler's default relax_domain_level.
> See Documentation/cgroups/cpusets.txt.
>diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>index 76fa1e9..39af130 100644
>--- a/arch/x86/kernel/reboot.c
>+++ b/arch/x86/kernel/reboot.c
>@@ -49,10 +49,6 @@ int reboot_force;
> */
> static int reboot_default = 1;
>
>-#ifdef CONFIG_SMP
>-static int reboot_cpu = -1;
>-#endif
>-
> /*
> * This is set if we need to go through the 'emergency' path.
> * When machine_emergency_restart() is called, we may be on
>@@ -64,11 +60,10 @@ static int reboot_emergency;
> bool port_cf9_safe = false;
>
> /*
>- * reboot=b[ios] | s[mp] | t[riple] | k[bd] | e[fi] [, [w]arm |
>[c]old] | p[ci]
>+ * reboot=b[ios] | t[riple] | k[bd] | e[fi] [, [w]arm | [c]old] |
>p[ci]
> * warm Don't set the cold reboot flag
> * cold Set the cold reboot flag
> * bios Reboot by jumping through the BIOS
>- * smp Reboot by executing reset on BSP or other CPU
> * triple Force a triple fault (init)
> * kbd Use the keyboard controller. cold reset (default)
> * acpi Use the RESET_REG in the FADT
>@@ -95,21 +90,6 @@ static int __init reboot_setup(char *str)
> reboot_mode = 0;
> break;
>
>-#ifdef CONFIG_SMP
>- case 's':
>- if (isdigit(*(str+1))) {
>- reboot_cpu = (int) (*(str+1) - '0');
>- if (isdigit(*(str+2)))
>- reboot_cpu = reboot_cpu*10 + (int)(*(str+2) - '0');
>- }
>- /*
>- * We will leave sorting out the final value
>- * when we are ready to reboot, since we might not
>- * have detected BSP APIC ID or smp_num_cpu
>- */
>- break;
>-#endif /* CONFIG_SMP */
>-
> case 'b':
> case 'a':
> case 'k':
>@@ -614,26 +594,10 @@ void native_machine_shutdown(void)
> {
> /* Stop the cpus and apics */
> #ifdef CONFIG_SMP
>-
>- /* The boot cpu is always logical cpu 0 */
>- int reboot_cpu_id = 0;
>-
>- /* See if there has been given a command line override */
>- if ((reboot_cpu != -1) && (reboot_cpu < nr_cpu_ids) &&
>- cpu_online(reboot_cpu))
>- reboot_cpu_id = reboot_cpu;
>-
>- /* Make certain the cpu I'm about to reboot on is online */
>- if (!cpu_online(reboot_cpu_id))
>- reboot_cpu_id = smp_processor_id();
>-
>- /* Make certain I only run on the appropriate processor */
>- set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
>-
> /*
>- * O.K Now that I'm on the appropriate processor, stop all of the
>- * others. Also disable the local irq to not receive the per-cpu
>- * timer interrupt which may trigger scheduler's load balance.
>+ * Stop all of the others. Also disable the local irq to
>+ * not receive the per-cpu timer interrupt which may trigger
>+ * scheduler's load balance.
> */
> local_irq_disable();
> stop_other_cpus();
>diff --git a/kernel/reboot.c b/kernel/reboot.c
>index 187a0d8..9fbab07 100644
>--- a/kernel/reboot.c
>+++ b/kernel/reboot.c
>@@ -8,6 +8,7 @@
> #include <linux/kexec.h>
> #include <linux/kmod.h>
> #include <linux/kmsg_dump.h>
>+#include <linux/moduleparam.h>
> #include <linux/reboot.h>
> #include <linux/syscalls.h>
> #include <linux/syscore_ops.h>
>@@ -66,13 +67,18 @@ int unregister_reboot_notifier(struct
>notifier_block *nb)
> }
> EXPORT_SYMBOL(unregister_reboot_notifier);
>
>+int reboot_cpuid;
>+core_param(reboot_cpuid, reboot_cpuid, int, 0644);
>+
> void migrate_to_reboot_cpu(void)
> {
> /* The boot cpu is always logical cpu 0 */
>- int reboot_cpu_id = 0;
>+ int reboot_cpu_id = reboot_cpuid;
>
> /* Make certain the cpu I'm about to reboot on is online */
> if (!cpu_online(reboot_cpu_id))
>+ reboot_cpu_id = 0;
>+ if (!cpu_online(reboot_cpu_id))
> reboot_cpu_id = smp_processor_id();
>
> /* Prevent races with other tasks migrating this task. */

--
Sent from my mobile phone. Please excuse brevity and lack of formatting.

2013-04-15 17:33:27

by Robin Holt

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.

On Mon, Apr 15, 2013 at 10:18:31AM -0700, H. Peter Anvin wrote:
> Um... doesn't this break anyone who currently depends on this?

Yes. That is also why I kept this separate and not tagged for
stable. They would need to start using the more general kernel parameter
to get the same functionality. Should I have done it differently?

Robin

>
> Robin Holt <[email protected]> wrote:
>
> >
> >Moving the reboot=s<##> parameter for x86 to a kernel parameter
> >proper. I did not find any other arch that was specifying the
> >reboot cpu.
> >
> >Signed-off-by: Robin Holt <[email protected]>
> >To: Ingo Molnar <[email protected]>
> >To: Russ Anderson <[email protected]>
> >Cc: Shawn Guo <[email protected]>
> >Cc: Oleg Nesterov <[email protected]>
> >Cc: Andrew Morton <[email protected]>
> >Cc: "H. Peter Anvin" <[email protected]>
> >Cc: Lai Jiangshan <[email protected]>
> >Cc: Linus Torvalds <[email protected]>
> >Cc: Linux Kernel Mailing List <[email protected]>
> >Cc: Michel Lespinasse <[email protected]>
> >Cc: Oleg Nesterov <[email protected]>
> >Cc: "Paul E. McKenney" <[email protected]>
> >Cc: Paul Mackerras <[email protected]>
> >Cc: Peter Zijlstra <[email protected]>
> >Cc: Robin Holt <[email protected]>
> >Cc: "[email protected]" <[email protected]>
> >Cc: Tejun Heo <[email protected]>
> >Cc: the arch/x86 maintainers <[email protected]>
> >Cc: Thomas Gleixner <[email protected]>
> >---
> > Documentation/kernel-parameters.txt | 2 ++
> >arch/x86/kernel/reboot.c | 44
> >++++---------------------------------
> > kernel/reboot.c | 8 ++++++-
> > 3 files changed, 13 insertions(+), 41 deletions(-)
> >
> >diff --git a/Documentation/kernel-parameters.txt
> >b/Documentation/kernel-parameters.txt
> >index 4609e81..11ebb48 100644
> >--- a/Documentation/kernel-parameters.txt
> >+++ b/Documentation/kernel-parameters.txt
> >@@ -2597,6 +2597,8 @@ bytes respectively. Such letter suffixes can also
> >be entirely omitted.
> > Format: <reboot_mode>[,<reboot_mode2>[,...]]
> > See arch/*/kernel/reboot.c or arch/*/kernel/process.c
> >
> >+ reboot_cpuid= [KNL] cpuid to use for reboot/halt, etc operations.
> >+
> > relax_domain_level=
> > [KNL, SMP] Set scheduler's default relax_domain_level.
> > See Documentation/cgroups/cpusets.txt.
> >diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> >index 76fa1e9..39af130 100644
> >--- a/arch/x86/kernel/reboot.c
> >+++ b/arch/x86/kernel/reboot.c
> >@@ -49,10 +49,6 @@ int reboot_force;
> > */
> > static int reboot_default = 1;
> >
> >-#ifdef CONFIG_SMP
> >-static int reboot_cpu = -1;
> >-#endif
> >-
> > /*
> > * This is set if we need to go through the 'emergency' path.
> > * When machine_emergency_restart() is called, we may be on
> >@@ -64,11 +60,10 @@ static int reboot_emergency;
> > bool port_cf9_safe = false;
> >
> > /*
> >- * reboot=b[ios] | s[mp] | t[riple] | k[bd] | e[fi] [, [w]arm |
> >[c]old] | p[ci]
> >+ * reboot=b[ios] | t[riple] | k[bd] | e[fi] [, [w]arm | [c]old] |
> >p[ci]
> > * warm Don't set the cold reboot flag
> > * cold Set the cold reboot flag
> > * bios Reboot by jumping through the BIOS
> >- * smp Reboot by executing reset on BSP or other CPU
> > * triple Force a triple fault (init)
> > * kbd Use the keyboard controller. cold reset (default)
> > * acpi Use the RESET_REG in the FADT
> >@@ -95,21 +90,6 @@ static int __init reboot_setup(char *str)
> > reboot_mode = 0;
> > break;
> >
> >-#ifdef CONFIG_SMP
> >- case 's':
> >- if (isdigit(*(str+1))) {
> >- reboot_cpu = (int) (*(str+1) - '0');
> >- if (isdigit(*(str+2)))
> >- reboot_cpu = reboot_cpu*10 + (int)(*(str+2) - '0');
> >- }
> >- /*
> >- * We will leave sorting out the final value
> >- * when we are ready to reboot, since we might not
> >- * have detected BSP APIC ID or smp_num_cpu
> >- */
> >- break;
> >-#endif /* CONFIG_SMP */
> >-
> > case 'b':
> > case 'a':
> > case 'k':
> >@@ -614,26 +594,10 @@ void native_machine_shutdown(void)
> > {
> > /* Stop the cpus and apics */
> > #ifdef CONFIG_SMP
> >-
> >- /* The boot cpu is always logical cpu 0 */
> >- int reboot_cpu_id = 0;
> >-
> >- /* See if there has been given a command line override */
> >- if ((reboot_cpu != -1) && (reboot_cpu < nr_cpu_ids) &&
> >- cpu_online(reboot_cpu))
> >- reboot_cpu_id = reboot_cpu;
> >-
> >- /* Make certain the cpu I'm about to reboot on is online */
> >- if (!cpu_online(reboot_cpu_id))
> >- reboot_cpu_id = smp_processor_id();
> >-
> >- /* Make certain I only run on the appropriate processor */
> >- set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
> >-
> > /*
> >- * O.K Now that I'm on the appropriate processor, stop all of the
> >- * others. Also disable the local irq to not receive the per-cpu
> >- * timer interrupt which may trigger scheduler's load balance.
> >+ * Stop all of the others. Also disable the local irq to
> >+ * not receive the per-cpu timer interrupt which may trigger
> >+ * scheduler's load balance.
> > */
> > local_irq_disable();
> > stop_other_cpus();
> >diff --git a/kernel/reboot.c b/kernel/reboot.c
> >index 187a0d8..9fbab07 100644
> >--- a/kernel/reboot.c
> >+++ b/kernel/reboot.c
> >@@ -8,6 +8,7 @@
> > #include <linux/kexec.h>
> > #include <linux/kmod.h>
> > #include <linux/kmsg_dump.h>
> >+#include <linux/moduleparam.h>
> > #include <linux/reboot.h>
> > #include <linux/syscalls.h>
> > #include <linux/syscore_ops.h>
> >@@ -66,13 +67,18 @@ int unregister_reboot_notifier(struct
> >notifier_block *nb)
> > }
> > EXPORT_SYMBOL(unregister_reboot_notifier);
> >
> >+int reboot_cpuid;
> >+core_param(reboot_cpuid, reboot_cpuid, int, 0644);
> >+
> > void migrate_to_reboot_cpu(void)
> > {
> > /* The boot cpu is always logical cpu 0 */
> >- int reboot_cpu_id = 0;
> >+ int reboot_cpu_id = reboot_cpuid;
> >
> > /* Make certain the cpu I'm about to reboot on is online */
> > if (!cpu_online(reboot_cpu_id))
> >+ reboot_cpu_id = 0;
> >+ if (!cpu_online(reboot_cpu_id))
> > reboot_cpu_id = smp_processor_id();
> >
> > /* Prevent races with other tasks migrating this task. */
>
> --
> Sent from my mobile phone. Please excuse brevity and lack of formatting.

2013-04-15 17:38:30

by Joe Perches

[permalink] [raw]
Subject: Re: [Patch -v3 2/4] Move shutdown/reboot related functions to kernel/reboot.c

On Mon, 2013-04-15 at 12:15 -0500, Robin Holt wrote:
> This patch is preparatory. It moves reboot related syscall, etc
> functions from kernel/sys.c to kernel/reboot.c.
[]
> diff --git a/kernel/reboot.c b/kernel/reboot.c
[]
> +#ifdef CONFIG_HIBERNATION
> + case LINUX_REBOOT_CMD_SW_SUSPEND:
> + ret = hibernate();
> + break;
> +#endif

Doesn't compile. needs suspend.h


2013-04-15 17:45:41

by Joe Perches

[permalink] [raw]
Subject: Re: [Patch -v3 3/4] checkpatch.pl the new kernel/reboot.c file.

On Mon, 2013-04-15 at 12:16 -0500, Robin Holt wrote:
> I did allow the remaining 81 character line behind. It did not seem
> like it was worth changing. Otherwise, it now passes checkpatch.pl.

trivia:

I'd make these changes on top of your patch:

o Additional OOM messages aren't necessary as a dump_stack is done
o Add pr_fmt to prefix logging messages with "reboot: "
o Remove periods from logging messages
o Argument alignment
o Rename file prefix to "reboot.c"
o Add #include <linux/suspend.h> so it compiles

---
kernel/reboot.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 187a0d8..7adba53 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -1,14 +1,17 @@
/*
- * linux/kernel/sys.c
+ * linux/kernel/reboot.c
*
* Copyright (C) 2013 Linus Torvalds
*/

+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include <linux/export.h>
#include <linux/kexec.h>
#include <linux/kmod.h>
#include <linux/kmsg_dump.h>
#include <linux/reboot.h>
+#include <linux/suspend.h>
#include <linux/syscalls.h>
#include <linux/syscore_ops.h>

@@ -96,9 +99,9 @@ void kernel_restart(char *cmd)
migrate_to_reboot_cpu();
syscore_shutdown();
if (!cmd)
- pr_emerg("Restarting system.\n");
+ pr_emerg("Restarting system\n");
else
- pr_emerg("Restarting system with command '%s'.\n", cmd);
+ pr_emerg("Restarting system with command '%s'\n", cmd);
kmsg_dump(KMSG_DUMP_RESTART);
machine_restart(cmd);
}
@@ -107,7 +110,8 @@ EXPORT_SYMBOL_GPL(kernel_restart);
static void kernel_shutdown_prepare(enum system_states state)
{
blocking_notifier_call_chain(&reboot_notifier_list,
- (state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
+ state == SYSTEM_HALT ? SYS_HALT : SYS_POWER_OFF,
+ NULL);
system_state = state;
usermodehelper_disable();
device_shutdown();
@@ -122,7 +126,7 @@ void kernel_halt(void)
kernel_shutdown_prepare(SYSTEM_HALT);
migrate_to_reboot_cpu();
syscore_shutdown();
- pr_emerg("System halted.\n");
+ pr_emerg("System halted\n");
kmsg_dump(KMSG_DUMP_HALT);
machine_halt();
}
@@ -140,7 +144,7 @@ void kernel_power_off(void)
pm_power_off_prepare();
migrate_to_reboot_cpu();
syscore_shutdown();
- pr_emerg("Power down.\n");
+ pr_emerg("Power down\n");
kmsg_dump(KMSG_DUMP_POWEROFF);
machine_power_off();
}
@@ -169,10 +173,10 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,

/* For safety, we require "magic" arguments. */
if (magic1 != LINUX_REBOOT_MAGIC1 ||
- (magic2 != LINUX_REBOOT_MAGIC2 &&
- magic2 != LINUX_REBOOT_MAGIC2A &&
- magic2 != LINUX_REBOOT_MAGIC2B &&
- magic2 != LINUX_REBOOT_MAGIC2C))
+ (magic2 != LINUX_REBOOT_MAGIC2 &&
+ magic2 != LINUX_REBOOT_MAGIC2A &&
+ magic2 != LINUX_REBOOT_MAGIC2B &&
+ magic2 != LINUX_REBOOT_MAGIC2C))
return -EINVAL;

/*
@@ -281,8 +285,6 @@ static int __orderly_poweroff(bool force)
ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
argv_free(argv);
} else {
- pr_warn("%s failed to allocate memory for \"%s\"\n",
- __func__, poweroff_cmd);
ret = -ENOMEM;
}


2013-04-15 17:48:30

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.

We usually don't break working user setups as a matter if policy. Only if it can't be avoided.

Robin Holt <[email protected]> wrote:

>On Mon, Apr 15, 2013 at 10:18:31AM -0700, H. Peter Anvin wrote:
>> Um... doesn't this break anyone who currently depends on this?
>
>Yes. That is also why I kept this separate and not tagged for
>stable. They would need to start using the more general kernel
>parameter
>to get the same functionality. Should I have done it differently?
>
>Robin
>
>>
>> Robin Holt <[email protected]> wrote:
>>
>> >
>> >Moving the reboot=s<##> parameter for x86 to a kernel parameter
>> >proper. I did not find any other arch that was specifying the
>> >reboot cpu.
>> >
>> >Signed-off-by: Robin Holt <[email protected]>
>> >To: Ingo Molnar <[email protected]>
>> >To: Russ Anderson <[email protected]>
>> >Cc: Shawn Guo <[email protected]>
>> >Cc: Oleg Nesterov <[email protected]>
>> >Cc: Andrew Morton <[email protected]>
>> >Cc: "H. Peter Anvin" <[email protected]>
>> >Cc: Lai Jiangshan <[email protected]>
>> >Cc: Linus Torvalds <[email protected]>
>> >Cc: Linux Kernel Mailing List <[email protected]>
>> >Cc: Michel Lespinasse <[email protected]>
>> >Cc: Oleg Nesterov <[email protected]>
>> >Cc: "Paul E. McKenney" <[email protected]>
>> >Cc: Paul Mackerras <[email protected]>
>> >Cc: Peter Zijlstra <[email protected]>
>> >Cc: Robin Holt <[email protected]>
>> >Cc: "[email protected]" <[email protected]>
>> >Cc: Tejun Heo <[email protected]>
>> >Cc: the arch/x86 maintainers <[email protected]>
>> >Cc: Thomas Gleixner <[email protected]>
>> >---
>> > Documentation/kernel-parameters.txt | 2 ++
>> >arch/x86/kernel/reboot.c | 44
>> >++++---------------------------------
>> > kernel/reboot.c | 8 ++++++-
>> > 3 files changed, 13 insertions(+), 41 deletions(-)
>> >
>> >diff --git a/Documentation/kernel-parameters.txt
>> >b/Documentation/kernel-parameters.txt
>> >index 4609e81..11ebb48 100644
>> >--- a/Documentation/kernel-parameters.txt
>> >+++ b/Documentation/kernel-parameters.txt
>> >@@ -2597,6 +2597,8 @@ bytes respectively. Such letter suffixes can
>also
>> >be entirely omitted.
>> > Format: <reboot_mode>[,<reboot_mode2>[,...]]
>> > See arch/*/kernel/reboot.c or arch/*/kernel/process.c
>> >
>> >+ reboot_cpuid= [KNL] cpuid to use for reboot/halt, etc operations.
>> >+
>> > relax_domain_level=
>> > [KNL, SMP] Set scheduler's default relax_domain_level.
>> > See Documentation/cgroups/cpusets.txt.
>> >diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
>> >index 76fa1e9..39af130 100644
>> >--- a/arch/x86/kernel/reboot.c
>> >+++ b/arch/x86/kernel/reboot.c
>> >@@ -49,10 +49,6 @@ int reboot_force;
>> > */
>> > static int reboot_default = 1;
>> >
>> >-#ifdef CONFIG_SMP
>> >-static int reboot_cpu = -1;
>> >-#endif
>> >-
>> > /*
>> > * This is set if we need to go through the 'emergency' path.
>> > * When machine_emergency_restart() is called, we may be on
>> >@@ -64,11 +60,10 @@ static int reboot_emergency;
>> > bool port_cf9_safe = false;
>> >
>> > /*
>> >- * reboot=b[ios] | s[mp] | t[riple] | k[bd] | e[fi] [, [w]arm |
>> >[c]old] | p[ci]
>> >+ * reboot=b[ios] | t[riple] | k[bd] | e[fi] [, [w]arm | [c]old] |
>> >p[ci]
>> > * warm Don't set the cold reboot flag
>> > * cold Set the cold reboot flag
>> > * bios Reboot by jumping through the BIOS
>> >- * smp Reboot by executing reset on BSP or other CPU
>> > * triple Force a triple fault (init)
>> > * kbd Use the keyboard controller. cold reset (default)
>> > * acpi Use the RESET_REG in the FADT
>> >@@ -95,21 +90,6 @@ static int __init reboot_setup(char *str)
>> > reboot_mode = 0;
>> > break;
>> >
>> >-#ifdef CONFIG_SMP
>> >- case 's':
>> >- if (isdigit(*(str+1))) {
>> >- reboot_cpu = (int) (*(str+1) - '0');
>> >- if (isdigit(*(str+2)))
>> >- reboot_cpu = reboot_cpu*10 + (int)(*(str+2) - '0');
>> >- }
>> >- /*
>> >- * We will leave sorting out the final value
>> >- * when we are ready to reboot, since we might not
>> >- * have detected BSP APIC ID or smp_num_cpu
>> >- */
>> >- break;
>> >-#endif /* CONFIG_SMP */
>> >-
>> > case 'b':
>> > case 'a':
>> > case 'k':
>> >@@ -614,26 +594,10 @@ void native_machine_shutdown(void)
>> > {
>> > /* Stop the cpus and apics */
>> > #ifdef CONFIG_SMP
>> >-
>> >- /* The boot cpu is always logical cpu 0 */
>> >- int reboot_cpu_id = 0;
>> >-
>> >- /* See if there has been given a command line override */
>> >- if ((reboot_cpu != -1) && (reboot_cpu < nr_cpu_ids) &&
>> >- cpu_online(reboot_cpu))
>> >- reboot_cpu_id = reboot_cpu;
>> >-
>> >- /* Make certain the cpu I'm about to reboot on is online */
>> >- if (!cpu_online(reboot_cpu_id))
>> >- reboot_cpu_id = smp_processor_id();
>> >-
>> >- /* Make certain I only run on the appropriate processor */
>> >- set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
>> >-
>> > /*
>> >- * O.K Now that I'm on the appropriate processor, stop all of the
>> >- * others. Also disable the local irq to not receive the per-cpu
>> >- * timer interrupt which may trigger scheduler's load balance.
>> >+ * Stop all of the others. Also disable the local irq to
>> >+ * not receive the per-cpu timer interrupt which may trigger
>> >+ * scheduler's load balance.
>> > */
>> > local_irq_disable();
>> > stop_other_cpus();
>> >diff --git a/kernel/reboot.c b/kernel/reboot.c
>> >index 187a0d8..9fbab07 100644
>> >--- a/kernel/reboot.c
>> >+++ b/kernel/reboot.c
>> >@@ -8,6 +8,7 @@
>> > #include <linux/kexec.h>
>> > #include <linux/kmod.h>
>> > #include <linux/kmsg_dump.h>
>> >+#include <linux/moduleparam.h>
>> > #include <linux/reboot.h>
>> > #include <linux/syscalls.h>
>> > #include <linux/syscore_ops.h>
>> >@@ -66,13 +67,18 @@ int unregister_reboot_notifier(struct
>> >notifier_block *nb)
>> > }
>> > EXPORT_SYMBOL(unregister_reboot_notifier);
>> >
>> >+int reboot_cpuid;
>> >+core_param(reboot_cpuid, reboot_cpuid, int, 0644);
>> >+
>> > void migrate_to_reboot_cpu(void)
>> > {
>> > /* The boot cpu is always logical cpu 0 */
>> >- int reboot_cpu_id = 0;
>> >+ int reboot_cpu_id = reboot_cpuid;
>> >
>> > /* Make certain the cpu I'm about to reboot on is online */
>> > if (!cpu_online(reboot_cpu_id))
>> >+ reboot_cpu_id = 0;
>> >+ if (!cpu_online(reboot_cpu_id))
>> > reboot_cpu_id = smp_processor_id();
>> >
>> > /* Prevent races with other tasks migrating this task. */
>>
>> --
>> Sent from my mobile phone. Please excuse brevity and lack of
>formatting.

--
Sent from my mobile phone. Please excuse brevity and lack of formatting.

2013-04-15 18:23:03

by Robin Holt

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.

On Mon, Apr 15, 2013 at 10:47:49AM -0700, H. Peter Anvin wrote:
> We usually don't break working user setups as a matter if policy. Only if it can't be avoided.
>
> Robin Holt <[email protected]> wrote:
>
> >On Mon, Apr 15, 2013 at 10:18:31AM -0700, H. Peter Anvin wrote:
> >> Um... doesn't this break anyone who currently depends on this?
> >
> >Yes. That is also why I kept this separate and not tagged for
> >stable. They would need to start using the more general kernel
> >parameter
> >to get the same functionality. Should I have done it differently?

How about:
Author: Robin Holt <[email protected]>
Date: Mon Apr 15 11:39:33 2013 -0500

Make reboot_cpuid a kernel parameter.

Moving the reboot=s<##> parameter for x86 to a kernel parameter
proper. I did not find any other arch that was specifying the
reboot cpu.

I left a compatibility mode in there. The new parameter always
takes precedence. I also fixed up the current code to support
up to cpuid's up to the current max of 4096.

Signed-off-by: Robin Holt <[email protected]>
To: Ingo Molnar <[email protected]>
To: Russ Anderson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: "[email protected]" <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Thomas Gleixner <[email protected]>

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4609e81..11ebb48 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2597,6 +2597,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
Format: <reboot_mode>[,<reboot_mode2>[,...]]
See arch/*/kernel/reboot.c or arch/*/kernel/process.c

+ reboot_cpuid= [KNL] cpuid to use for reboot/halt, etc operations.
+
relax_domain_level=
[KNL, SMP] Set scheduler's default relax_domain_level.
See Documentation/cgroups/cpusets.txt.
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 76fa1e9..cdcf28f 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -49,10 +49,6 @@ int reboot_force;
*/
static int reboot_default = 1;

-#ifdef CONFIG_SMP
-static int reboot_cpu = -1;
-#endif
-
/*
* This is set if we need to go through the 'emergency' path.
* When machine_emergency_restart() is called, we may be on
@@ -63,6 +59,8 @@ static int reboot_emergency;
/* This is set by the PCI code if either type 1 or type 2 PCI is detected */
bool port_cf9_safe = false;

+extern int reboot_cpuid;
+
/*
* reboot=b[ios] | s[mp] | t[riple] | k[bd] | e[fi] [, [w]arm | [c]old] | p[ci]
* warm Don't set the cold reboot flag
@@ -97,10 +95,14 @@ static int __init reboot_setup(char *str)

#ifdef CONFIG_SMP
case 's':
- if (isdigit(*(str+1))) {
- reboot_cpu = (int) (*(str+1) - '0');
- if (isdigit(*(str+2)))
- reboot_cpu = reboot_cpu*10 + (int)(*(str+2) - '0');
+ if (reboot_cpuid < 0) {
+ int coffset;
+
+ reboot_cpuid = 0;
+ for (coffset = 1; isdigit(*(str + coffset)); coffset++) {
+ reboot_cpuid *= 10;
+ reboot_cpuid += (int)(*(str+2) - '0');
+ }
}
/*
* We will leave sorting out the final value
@@ -615,25 +617,10 @@ void native_machine_shutdown(void)
/* Stop the cpus and apics */
#ifdef CONFIG_SMP

- /* The boot cpu is always logical cpu 0 */
- int reboot_cpu_id = 0;
-
- /* See if there has been given a command line override */
- if ((reboot_cpu != -1) && (reboot_cpu < nr_cpu_ids) &&
- cpu_online(reboot_cpu))
- reboot_cpu_id = reboot_cpu;
-
- /* Make certain the cpu I'm about to reboot on is online */
- if (!cpu_online(reboot_cpu_id))
- reboot_cpu_id = smp_processor_id();
-
- /* Make certain I only run on the appropriate processor */
- set_cpus_allowed_ptr(current, cpumask_of(reboot_cpu_id));
-
/*
- * O.K Now that I'm on the appropriate processor, stop all of the
- * others. Also disable the local irq to not receive the per-cpu
- * timer interrupt which may trigger scheduler's load balance.
+ * Stop all of the others. Also disable the local irq to
+ * not receive the per-cpu timer interrupt which may trigger
+ * scheduler's load balance.
*/
local_irq_disable();
stop_other_cpus();
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 187a0d8..8d7b7d4 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -8,6 +8,7 @@
#include <linux/kexec.h>
#include <linux/kmod.h>
#include <linux/kmsg_dump.h>
+#include <linux/moduleparam.h>
#include <linux/reboot.h>
#include <linux/syscalls.h>
#include <linux/syscore_ops.h>
@@ -66,13 +67,18 @@ int unregister_reboot_notifier(struct notifier_block *nb)
}
EXPORT_SYMBOL(unregister_reboot_notifier);

+int reboot_cpuid = -1;
+core_param(reboot_cpuid, reboot_cpuid, int, 0644);
+
void migrate_to_reboot_cpu(void)
{
/* The boot cpu is always logical cpu 0 */
- int reboot_cpu_id = 0;
+ int reboot_cpu_id = reboot_cpuid;

/* Make certain the cpu I'm about to reboot on is online */
if (!cpu_online(reboot_cpu_id))
+ reboot_cpu_id = 0;
+ if (!cpu_online(reboot_cpu_id))
reboot_cpu_id = smp_processor_id();

/* Prevent races with other tasks migrating this task. */

2013-04-15 18:40:04

by Robin Holt

[permalink] [raw]
Subject: Re: [Patch -v3 3/4] checkpatch.pl the new kernel/reboot.c file.

On Mon, Apr 15, 2013 at 10:45:38AM -0700, Joe Perches wrote:
> On Mon, 2013-04-15 at 12:16 -0500, Robin Holt wrote:
> > I did allow the remaining 81 character line behind. It did not seem
> > like it was worth changing. Otherwise, it now passes checkpatch.pl.
>
> trivia:
>
> I'd make these changes on top of your patch:
>
> o Additional OOM messages aren't necessary as a dump_stack is done
> o Add pr_fmt to prefix logging messages with "reboot: "
> o Remove periods from logging messages
> o Argument alignment
> o Rename file prefix to "reboot.c"
> o Add #include <linux/suspend.h> so it compiles

I added linux/suspend.h and also turned on HIBERNATE in my .config file.

Robin

>
> ---
> kernel/reboot.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 187a0d8..7adba53 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -1,14 +1,17 @@
> /*
> - * linux/kernel/sys.c
> + * linux/kernel/reboot.c
> *
> * Copyright (C) 2013 Linus Torvalds
> */
>
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> #include <linux/export.h>
> #include <linux/kexec.h>
> #include <linux/kmod.h>
> #include <linux/kmsg_dump.h>
> #include <linux/reboot.h>
> +#include <linux/suspend.h>
> #include <linux/syscalls.h>
> #include <linux/syscore_ops.h>
>
> @@ -96,9 +99,9 @@ void kernel_restart(char *cmd)
> migrate_to_reboot_cpu();
> syscore_shutdown();
> if (!cmd)
> - pr_emerg("Restarting system.\n");
> + pr_emerg("Restarting system\n");
> else
> - pr_emerg("Restarting system with command '%s'.\n", cmd);
> + pr_emerg("Restarting system with command '%s'\n", cmd);
> kmsg_dump(KMSG_DUMP_RESTART);
> machine_restart(cmd);
> }
> @@ -107,7 +110,8 @@ EXPORT_SYMBOL_GPL(kernel_restart);
> static void kernel_shutdown_prepare(enum system_states state)
> {
> blocking_notifier_call_chain(&reboot_notifier_list,
> - (state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
> + state == SYSTEM_HALT ? SYS_HALT : SYS_POWER_OFF,
> + NULL);
> system_state = state;
> usermodehelper_disable();
> device_shutdown();
> @@ -122,7 +126,7 @@ void kernel_halt(void)
> kernel_shutdown_prepare(SYSTEM_HALT);
> migrate_to_reboot_cpu();
> syscore_shutdown();
> - pr_emerg("System halted.\n");
> + pr_emerg("System halted\n");
> kmsg_dump(KMSG_DUMP_HALT);
> machine_halt();
> }
> @@ -140,7 +144,7 @@ void kernel_power_off(void)
> pm_power_off_prepare();
> migrate_to_reboot_cpu();
> syscore_shutdown();
> - pr_emerg("Power down.\n");
> + pr_emerg("Power down\n");
> kmsg_dump(KMSG_DUMP_POWEROFF);
> machine_power_off();
> }
> @@ -169,10 +173,10 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>
> /* For safety, we require "magic" arguments. */
> if (magic1 != LINUX_REBOOT_MAGIC1 ||
> - (magic2 != LINUX_REBOOT_MAGIC2 &&
> - magic2 != LINUX_REBOOT_MAGIC2A &&
> - magic2 != LINUX_REBOOT_MAGIC2B &&
> - magic2 != LINUX_REBOOT_MAGIC2C))
> + (magic2 != LINUX_REBOOT_MAGIC2 &&
> + magic2 != LINUX_REBOOT_MAGIC2A &&
> + magic2 != LINUX_REBOOT_MAGIC2B &&
> + magic2 != LINUX_REBOOT_MAGIC2C))
> return -EINVAL;
>
> /*
> @@ -281,8 +285,6 @@ static int __orderly_poweroff(bool force)
> ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
> argv_free(argv);
> } else {
> - pr_warn("%s failed to allocate memory for \"%s\"\n",
> - __func__, poweroff_cmd);
> ret = -ENOMEM;
> }
>
>

2013-04-16 03:02:30

by Shawn Guo

[permalink] [raw]
Subject: Re: [Patch -v3 1/4] Migrate shutdown/reboot to boot cpu.

On Mon, Apr 15, 2013 at 12:14:29PM -0500, Robin Holt wrote:
> We recently noticed that reboot of a 1024 cpu machine takes approx 16
> minutes of just stopping the cpus. The slowdown was tracked to commit
> f96972f.
>
> The current implementation does all the work of hot removing the cpus
> before halting the system. We are switching to just migrating to the
> boot cpu and then continuing with shutdown/reboot.
>
> This also has the effect of not breaking x86's command line parameter for
> specifying the reboot cpu. Note, this code was shamelessly copied from
> arch/x86/kernel/reboot.c with bits removed pertaining to the reboot_cpu
> command line parameter.
>
> Signed-off-by: Robin Holt <[email protected]>
> To: Shawn Guo <[email protected]>

It works on my hardware - i.MX6 Quad.

Tested-by: Shawn Guo <[email protected]>

2013-04-16 09:01:17

by Ingo Molnar

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.


* Robin Holt <[email protected]> wrote:

> On Mon, Apr 15, 2013 at 10:47:49AM -0700, H. Peter Anvin wrote:
> > We usually don't break working user setups as a matter if policy. Only if it can't be avoided.
> >
> > Robin Holt <[email protected]> wrote:
> >
> > >On Mon, Apr 15, 2013 at 10:18:31AM -0700, H. Peter Anvin wrote:
> > >> Um... doesn't this break anyone who currently depends on this?
> > >
> > >Yes. That is also why I kept this separate and not tagged for
> > >stable. They would need to start using the more general kernel
> > >parameter
> > >to get the same functionality. Should I have done it differently?
>
> How about:
> Author: Robin Holt <[email protected]>
> Date: Mon Apr 15 11:39:33 2013 -0500
>
> Make reboot_cpuid a kernel parameter.
>
> Moving the reboot=s<##> parameter for x86 to a kernel parameter
> proper. I did not find any other arch that was specifying the
> reboot cpu.
>
> I left a compatibility mode in there. The new parameter always
> takes precedence. I also fixed up the current code to support
> up to cpuid's up to the current max of 4096.

Looks pretty nice and clean to me, as the old boot parameter still keeps working.

Mind sending an updated series that includes all the fixes and the Tested-by from
Shawn Guo?

Thanks,

Ingo

2013-04-16 09:24:33

by Robin Holt

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.

On Tue, Apr 16, 2013 at 11:01:09AM +0200, Ingo Molnar wrote:
>
> * Robin Holt <[email protected]> wrote:
>
> > On Mon, Apr 15, 2013 at 10:47:49AM -0700, H. Peter Anvin wrote:
> > > We usually don't break working user setups as a matter if policy. Only if it can't be avoided.
> > >
> > > Robin Holt <[email protected]> wrote:
> > >
> > > >On Mon, Apr 15, 2013 at 10:18:31AM -0700, H. Peter Anvin wrote:
> > > >> Um... doesn't this break anyone who currently depends on this?
> > > >
> > > >Yes. That is also why I kept this separate and not tagged for
> > > >stable. They would need to start using the more general kernel
> > > >parameter
> > > >to get the same functionality. Should I have done it differently?
> >
> > How about:
> > Author: Robin Holt <[email protected]>
> > Date: Mon Apr 15 11:39:33 2013 -0500
> >
> > Make reboot_cpuid a kernel parameter.
> >
> > Moving the reboot=s<##> parameter for x86 to a kernel parameter
> > proper. I did not find any other arch that was specifying the
> > reboot cpu.
> >
> > I left a compatibility mode in there. The new parameter always
> > takes precedence. I also fixed up the current code to support
> > up to cpuid's up to the current max of 4096.
>
> Looks pretty nice and clean to me, as the old boot parameter still keeps working.
>
> Mind sending an updated series that includes all the fixes and the Tested-by from
> Shawn Guo?

Will do. I am still going through the comments from Joe Perches to the
third patch. Already have added the Tested-by to just the first patch.
Otherwise, I am ready to send -v4.

Robin

2013-04-16 09:30:05

by Ingo Molnar

[permalink] [raw]
Subject: Re: [Patch -v3 4/4] Make reboot_cpuid a kernel parameter.


* Robin Holt <[email protected]> wrote:

> On Tue, Apr 16, 2013 at 11:01:09AM +0200, Ingo Molnar wrote:
> >
> > * Robin Holt <[email protected]> wrote:
> >
> > > On Mon, Apr 15, 2013 at 10:47:49AM -0700, H. Peter Anvin wrote:
> > > > We usually don't break working user setups as a matter if policy. Only if it can't be avoided.
> > > >
> > > > Robin Holt <[email protected]> wrote:
> > > >
> > > > >On Mon, Apr 15, 2013 at 10:18:31AM -0700, H. Peter Anvin wrote:
> > > > >> Um... doesn't this break anyone who currently depends on this?
> > > > >
> > > > >Yes. That is also why I kept this separate and not tagged for
> > > > >stable. They would need to start using the more general kernel
> > > > >parameter
> > > > >to get the same functionality. Should I have done it differently?
> > >
> > > How about:
> > > Author: Robin Holt <[email protected]>
> > > Date: Mon Apr 15 11:39:33 2013 -0500
> > >
> > > Make reboot_cpuid a kernel parameter.
> > >
> > > Moving the reboot=s<##> parameter for x86 to a kernel parameter
> > > proper. I did not find any other arch that was specifying the
> > > reboot cpu.
> > >
> > > I left a compatibility mode in there. The new parameter always
> > > takes precedence. I also fixed up the current code to support
> > > up to cpuid's up to the current max of 4096.
> >
> > Looks pretty nice and clean to me, as the old boot parameter still keeps working.
> >
> > Mind sending an updated series that includes all the fixes and the Tested-by from
> > Shawn Guo?
>
> Will do. I am still going through the comments from Joe Perches to the
> third patch. [...]

Those fixes looked sensible to me.

> [...] Already have added the Tested-by to just the first patch. Otherwise, I am
> ready to send -v4.

Cool, thanks.

Assuming there are no objections from others, this looks like v3.10 material, as
we are right before the final v3.9 release and this touches core kernel code
(which is risky in such a late stage). It all needs a few days of testing as well,
since it affects the reboot path of pretty much every Linux box in existence that
runs the updated kernel.

Would you like to see this fix backported? If yes then your simpler v1 patch to
kernel/sys.c from a few days ago should probably be put into the series as patch
#1, partially reverted by patch #2 et al.

Then patch #1 can be sent to -stable in a compact form, not the whole series. The
end result of the series would not change.

Thanks,

Ingo

2013-04-16 09:41:06

by Robin Holt

[permalink] [raw]
Subject: Re: [Patch -v3 3/4] checkpatch.pl the new kernel/reboot.c file.

On Mon, Apr 15, 2013 at 10:45:38AM -0700, Joe Perches wrote:
> On Mon, 2013-04-15 at 12:16 -0500, Robin Holt wrote:
> > I did allow the remaining 81 character line behind. It did not seem
> > like it was worth changing. Otherwise, it now passes checkpatch.pl.
>
> trivia:
>
> I'd make these changes on top of your patch:
>
> o Additional OOM messages aren't necessary as a dump_stack is done

I am not sure what I should be doing with this one. The only message
that seems close is the pr_warn("%s failed to allocate memory for...
but I don't see how dump_stack is getting invoked so I think either
you did not mean this message or you intended me to replace that with a
dump_stack, in which case, the error message would still be informative
and the dump_stack would be relatively useless.

> o Add pr_fmt to prefix logging messages with "reboot: "

Done. I did not see anything in the CodingStyle about this so I hope
I did it correctly. It will change some long-seen messages during
shutdown/reboot. Is that acceptable?

> o Remove periods from logging messages

Done. I read the CodingStyle guide's "Kernel messages do not have to
be terminated with a period." to mean they did not need to be removed.
I would not normally have touched them as I effectively moved them from
one file to another and I followed the guiding principal of do not mess
with somebody else's code style (especially Linus') unless necessary.

> o Argument alignment

Again, not sure what you want here. I kept the formatting fairly
consistent when I resolved the checkpatch.pl bits. I believe I only
changed the formatting on one if() statement. I had used tab followed
by space so I fixed that up in -v4.

> o Rename file prefix to "reboot.c"

I did fix up the comment which said sys.c. Is that what you were
asking for? If not, could you be more specific?

> o Add #include <linux/suspend.h> so it compiles

Done.

Thanks,
Robin


Subject: [Patch -v4 3/4] checkpatch.pl the new kernel/reboot.c file.

I did allow the remaining 81 character line behind. It did not seem
like it was worth changing. Otherwise, it now passes checkpatch.pl.

Signed-off-by: Robin Holt <[email protected]>
To: Ingo Molnar <[email protected]>
To: Russ Anderson <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Lai Jiangshan <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Linux Kernel Mailing List <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Robin Holt <[email protected]>
Cc: "[email protected]" <[email protected]>
Cc: Tejun Heo <[email protected]>
Cc: the arch/x86 maintainers <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Joe Perches <[email protected]>

---

Changes since -v3:
Integrated feedback from Joe Perches.

kernel/reboot.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/kernel/reboot.c b/kernel/reboot.c
index 907ab48..8cfef20 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -1,9 +1,11 @@
/*
- * linux/kernel/sys.c
+ * linux/kernel/reboot.c
*
* Copyright (C) 2013 Linus Torvalds
*/

+#define pr_fmt(fmt) "reboot: " fmt
+
#include <linux/export.h>
#include <linux/kexec.h>
#include <linux/kmod.h>
@@ -97,9 +99,9 @@ void kernel_restart(char *cmd)
migrate_to_reboot_cpu();
syscore_shutdown();
if (!cmd)
- printk(KERN_EMERG "Restarting system.\n");
+ pr_emerg("Restarting system\n");
else
- printk(KERN_EMERG "Restarting system with command '%s'.\n", cmd);
+ pr_emerg("Restarting system with command '%s'\n", cmd);
kmsg_dump(KMSG_DUMP_RESTART);
machine_restart(cmd);
}
@@ -108,7 +110,7 @@ EXPORT_SYMBOL_GPL(kernel_restart);
static void kernel_shutdown_prepare(enum system_states state)
{
blocking_notifier_call_chain(&reboot_notifier_list,
- (state == SYSTEM_HALT)?SYS_HALT:SYS_POWER_OFF, NULL);
+ (state == SYSTEM_HALT) ? SYS_HALT : SYS_POWER_OFF, NULL);
system_state = state;
usermodehelper_disable();
device_shutdown();
@@ -123,11 +125,10 @@ void kernel_halt(void)
kernel_shutdown_prepare(SYSTEM_HALT);
migrate_to_reboot_cpu();
syscore_shutdown();
- printk(KERN_EMERG "System halted.\n");
+ pr_emerg("System halted\n");
kmsg_dump(KMSG_DUMP_HALT);
machine_halt();
}
-
EXPORT_SYMBOL_GPL(kernel_halt);

/**
@@ -142,7 +143,7 @@ void kernel_power_off(void)
pm_power_off_prepare();
migrate_to_reboot_cpu();
syscore_shutdown();
- printk(KERN_EMERG "Power down.\n");
+ pr_emerg("Power down\n");
kmsg_dump(KMSG_DUMP_POWEROFF);
machine_power_off();
}
@@ -171,10 +172,10 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,

/* For safety, we require "magic" arguments. */
if (magic1 != LINUX_REBOOT_MAGIC1 ||
- (magic2 != LINUX_REBOOT_MAGIC2 &&
- magic2 != LINUX_REBOOT_MAGIC2A &&
+ (magic2 != LINUX_REBOOT_MAGIC2 &&
+ magic2 != LINUX_REBOOT_MAGIC2A &&
magic2 != LINUX_REBOOT_MAGIC2B &&
- magic2 != LINUX_REBOOT_MAGIC2C))
+ magic2 != LINUX_REBOOT_MAGIC2C))
return -EINVAL;

/*
@@ -283,14 +284,13 @@ static int __orderly_poweroff(bool force)
ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
argv_free(argv);
} else {
- printk(KERN_WARNING "%s failed to allocate memory for \"%s\"\n",
- __func__, poweroff_cmd);
+ pr_warn("%s failed to allocate memory for \"%s\"\n",
+ __func__, poweroff_cmd);
ret = -ENOMEM;
}

if (ret && force) {
- printk(KERN_WARNING "Failed to start orderly shutdown: "
- "forcing the issue\n");
+ pr_warn("Failed to start orderly shutdown: forcing the issue\n");
/*
* I guess this should try to kick off some daemon to sync and
* poweroff asap. Or not even bother syncing if we're doing an
--
1.8.1.2

2013-04-16 15:10:31

by Joe Perches

[permalink] [raw]
Subject: Re: [Patch -v3 3/4] checkpatch.pl the new kernel/reboot.c file.

(trimmed cc's)

On Tue, 2013-04-16 at 04:41 -0500, Robin Holt wrote:
> On Mon, Apr 15, 2013 at 10:45:38AM -0700, Joe Perches wrote:
> > trivia:
> > I'd make these changes on top of your patch:
> > o Additional OOM messages aren't necessary as a dump_stack is done
> I am not sure what I should be doing with this one.

It could be removed eventually.

> The only message
> that seems close is the pr_warn("%s failed to allocate memory for...
> but I don't see how dump_stack is getting invoked so I think either
> you did not mean this message or you intended me to replace that with a
> dump_stack, in which case, the error message would still be informative
> and the dump_stack would be relatively useless.

All failed mallocs without __GFP_NOWARN get a dump_stack via
mm/page_alloc:warn_alloc_failed() so that pr_warn isn't really
required.