| Message ID | 1427681733-25488-1-git-send-email-joel@jms.id.au (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michael Ellerman |
| Headers | show |
On Mon, 30 Mar 2015 12:45:32 +1030 Joel Stanley <joel@jms.id.au> wrote: > The kernel has orderly_poweroff which allows the kernel to initiate a > graceful shutdown of userspace, by running /sbin/poweroff. This adds > orderly_reboot that will cause userspace to shut itself down by calling > /sbin/reboot. > > This will be used for shutdown initiated by a system controller on > platforms that do not use ACPI. gee. There are a lot of callers of emergency_restart(). Why is the BMC reboot special, and how many of the emergency_restart() callers really be using orderly_reboot()? We have /proc/sys/kernel/poweroff_cmd. Should we have /proc/sys/kernel/reboot_cmd as well? If not, kernel/reboot.c:reboot_cmd[] can be made static ;)
Hi Andrew, On Wed, Apr 1, 2015 at 9:09 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > On Mon, 30 Mar 2015 12:45:32 +1030 Joel Stanley <joel@jms.id.au> wrote: > >> The kernel has orderly_poweroff which allows the kernel to initiate a >> graceful shutdown of userspace, by running /sbin/poweroff. This adds >> orderly_reboot that will cause userspace to shut itself down by calling >> /sbin/reboot. >> >> This will be used for shutdown initiated by a system controller on >> platforms that do not use ACPI. > > gee. There are a lot of callers of emergency_restart(). Why is the > BMC reboot special, and how many of the emergency_restart() callers > really be using orderly_reboot()? The BMC reboot is intended to be a graceful shutdown - let userspace do it's thing before the system goes down. Userspace may chose to stop and perform some long, slow teardown before it gets around to shutting down. We don't want to move callers over orderly_reboot() if they're shutting the system down due to a critical failure, eg. printer on fire. I had a read of the emergency_restart() callers and I didn't see any obvious cases for moving over to orderly_reboot(). > We have /proc/sys/kernel/poweroff_cmd. Should we have > /proc/sys/kernel/reboot_cmd as well? If not, > kernel/reboot.c:reboot_cmd[] can be made static ;) I don't think we need it. I'll make reboot_cmd[] static. Cheers, Joel
On 03/30/2015 07:45 AM, Joel Stanley wrote: > The kernel has orderly_poweroff which allows the kernel to initiate a > graceful shutdown of userspace, by running /sbin/poweroff. This adds > orderly_reboot that will cause userspace to shut itself down by calling > /sbin/reboot. > > This will be used for shutdown initiated by a system controller on > platforms that do not use ACPI. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > include/linux/reboot.h | 1 + > kernel/reboot.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- > 2 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/include/linux/reboot.h b/include/linux/reboot.h > index 48bf152..a4ffcd9 100644 > --- a/include/linux/reboot.h > +++ b/include/linux/reboot.h > @@ -68,6 +68,7 @@ void ctrl_alt_del(void); > extern char poweroff_cmd[POWEROFF_CMD_PATH_LEN]; > > extern int orderly_poweroff(bool force); > +extern int orderly_reboot(void); > > /* > * Emergency restart, callable from an interrupt handler. > diff --git a/kernel/reboot.c b/kernel/reboot.c > index a3a9e24..d0aa1ec 100644 > --- a/kernel/reboot.c > +++ b/kernel/reboot.c > @@ -306,8 +306,9 @@ void ctrl_alt_del(void) > } > > char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff"; > +char reboot_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/reboot"; Should not we declare one more REBOOT_CMD_PATH_LEN to make it cleaner. > > -static int __orderly_poweroff(bool force) > +static int run_cmd(const char *cmd) > { > char **argv; > static char *envp[] = { > @@ -316,8 +317,7 @@ static int __orderly_poweroff(bool force) > NULL > }; > int ret; > - > - argv = argv_split(GFP_KERNEL, poweroff_cmd, NULL); > + argv = argv_split(GFP_KERNEL, cmd, NULL); > if (argv) { > ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC); > argv_free(argv); > @@ -325,8 +325,33 @@ static int __orderly_poweroff(bool force) > ret = -ENOMEM; > } > > + return ret; > +} > + > +static int __orderly_reboot(void) > +{ > + int ret; > + > + ret = run_cmd(reboot_cmd); > + > + if (ret) { > + pr_warn("Failed to start orderly reboot: forcing the issue\n"); > + emergency_sync(); > + kernel_restart(NULL); > + } > + > + return ret; > +} > + > +static int __orderly_poweroff(bool force) > +{ > + int ret; > + > + ret = run_cmd(reboot_cmd); Would it be poweroff_cmd instead of reboot_cmd ? Dont see poweroff_cmd getting used.
On Wed, 01 Apr 2015 10:22:08 +0530 Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote: > > char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff"; > > +char reboot_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/reboot"; > > Should not we declare one more REBOOT_CMD_PATH_LEN to make it cleaner. It doesn't really seem necessary - they'll have the same value anyway. But if you aren't going to implement the sysctl it isn't needed at all - just do static char reboot_cmd[] = "/sbin/reboot";
On Tue, 31 Mar 2015 22:03:26 -0700 Andrew Morton <akpm@linux-foundation.org> wrote:
> static char reboot_cmd[] = "/sbin/reboot";
static const char, actually.
On 04/01/2015 08:47 AM, Joel Stanley wrote: > Hi Andrew, > > On Wed, Apr 1, 2015 at 9:09 AM, Andrew Morton <akpm@linux-foundation.org> wrote: >> > On Mon, 30 Mar 2015 12:45:32 +1030 Joel Stanley <joel@jms.id.au> wrote: >> > >>> >> The kernel has orderly_poweroff which allows the kernel to initiate a >>> >> graceful shutdown of userspace, by running /sbin/poweroff. This adds >>> >> orderly_reboot that will cause userspace to shut itself down by calling >>> >> /sbin/reboot. >>> >> >>> >> This will be used for shutdown initiated by a system controller on >>> >> platforms that do not use ACPI. >> > >> > gee. There are a lot of callers of emergency_restart(). Why is the >> > BMC reboot special, and how many of the emergency_restart() callers >> > really be using orderly_reboot()? > The BMC reboot is intended to be a graceful shutdown - let userspace > do it's thing before the system goes down. > > Userspace may chose to stop and perform some long, slow teardown > before it gets around to shutting down. We don't want to move callers > over orderly_reboot() if they're shutting the system down due to a > critical failure, eg. printer on fire. > > I had a read of the emergency_restart() callers and I didn't see any > obvious cases for moving over to orderly_reboot(). > >> > We have /proc/sys/kernel/poweroff_cmd. Should we have >> > /proc/sys/kernel/reboot_cmd as well? If not, >> > kernel/reboot.c:reboot_cmd[] can be made static ;) > I don't think we need it. I'll make reboot_cmd[] static. Just to have parity with power off command, /proc/sys/kernel/reboot_cmd would be nice to have.
On Wed, Apr 1, 2015 at 3:22 PM, Anshuman Khandual <khandual@linux.vnet.ibm.com> wrote: >> +static int __orderly_poweroff(bool force) >> +{ >> + int ret; >> + >> + ret = run_cmd(reboot_cmd); > > Would it be poweroff_cmd instead of reboot_cmd ? Dont see poweroff_cmd getting used. Yes, good catch. Thanks. Joel
diff --git a/include/linux/reboot.h b/include/linux/reboot.h index 48bf152..a4ffcd9 100644 --- a/include/linux/reboot.h +++ b/include/linux/reboot.h @@ -68,6 +68,7 @@ void ctrl_alt_del(void); extern char poweroff_cmd[POWEROFF_CMD_PATH_LEN]; extern int orderly_poweroff(bool force); +extern int orderly_reboot(void); /* * Emergency restart, callable from an interrupt handler. diff --git a/kernel/reboot.c b/kernel/reboot.c index a3a9e24..d0aa1ec 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -306,8 +306,9 @@ void ctrl_alt_del(void) } char poweroff_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/poweroff"; +char reboot_cmd[POWEROFF_CMD_PATH_LEN] = "/sbin/reboot"; -static int __orderly_poweroff(bool force) +static int run_cmd(const char *cmd) { char **argv; static char *envp[] = { @@ -316,8 +317,7 @@ static int __orderly_poweroff(bool force) NULL }; int ret; - - argv = argv_split(GFP_KERNEL, poweroff_cmd, NULL); + argv = argv_split(GFP_KERNEL, cmd, NULL); if (argv) { ret = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC); argv_free(argv); @@ -325,8 +325,33 @@ static int __orderly_poweroff(bool force) ret = -ENOMEM; } + return ret; +} + +static int __orderly_reboot(void) +{ + int ret; + + ret = run_cmd(reboot_cmd); + + if (ret) { + pr_warn("Failed to start orderly reboot: forcing the issue\n"); + emergency_sync(); + kernel_restart(NULL); + } + + return ret; +} + +static int __orderly_poweroff(bool force) +{ + int ret; + + ret = run_cmd(reboot_cmd); + if (ret && force) { 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 @@ -364,6 +389,26 @@ int orderly_poweroff(bool force) } EXPORT_SYMBOL_GPL(orderly_poweroff); +static void reboot_work_func(struct work_struct *work) +{ + __orderly_reboot(); +} + +static DECLARE_WORK(reboot_work, reboot_work_func); + +/** + * orderly_reboot - Trigger an orderly system reboot + * + * This may be called from any context to trigger a system reboot. + * If the orderly reboot fails, it will force an immediate reboot. + */ +int orderly_reboot(void) +{ + schedule_work(&reboot_work); + return 0; +} +EXPORT_SYMBOL_GPL(orderly_reboot); + static int __init reboot_setup(char *str) { for (;;) {
The kernel has orderly_poweroff which allows the kernel to initiate a graceful shutdown of userspace, by running /sbin/poweroff. This adds orderly_reboot that will cause userspace to shut itself down by calling /sbin/reboot. This will be used for shutdown initiated by a system controller on platforms that do not use ACPI. Signed-off-by: Joel Stanley <joel@jms.id.au> --- include/linux/reboot.h | 1 + kernel/reboot.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 3 deletions(-)