| Message ID | 1487019642-11411-1-git-send-email-gpiccoli@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, 13 Feb 2017 19:00:42 -0200 "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> wrote: > Currently the xmon debugger is set only via kernel boot command-line. > It's disabled by default, and can be enabled with "xmon=on" on the > command-line. Also, xmon may be accessed via sysrq mechanism, but once > we enter xmon via sysrq, it's kept enabled until system is rebooted, > even if we exit the debugger. A kernel crash will then lead to xmon > instance, instead of triggering a kdump procedure (if configured), for > example. > > This patch introduces a debugfs entry for xmon, allowing user to query > its current state and change it if desired. Basically, the "xmon" file > to read from/write to is under the debugfs mount point, on powerpc > directory. Reading this file will provide the current state of the > debugger, one of the following: "on", "off", "early" or "nobt". Writing > one of these states to the file will take immediate effect on the debugger. > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> > --- > * I had this patch partially done for some time, and after a discussion > at the kernel slack channel latest week, I decided to rebase and fix > some remaining bugs. I'd change 'x' option to always disable the debugger, > since with this patch we can always re-enable xmon, but today I noticed > Pan's patch on the mailing list, so perhaps his approach of adding a flag > to 'x' option is preferable. I can change this in a V2, if requested. > Thanks in advance! xmon state changing after the first sysrq+x violates principle of least astonishment, so I think that should be fixed. Then the question is, is it worth making it runtime configurable with xmon command or debugfs tunables? Thanks, Nick
在 2017/2/14 10:35, Nicholas Piggin 写道: > On Mon, 13 Feb 2017 19:00:42 -0200 > "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> wrote: > >> Currently the xmon debugger is set only via kernel boot command-line. >> It's disabled by default, and can be enabled with "xmon=on" on the >> command-line. Also, xmon may be accessed via sysrq mechanism, but once >> we enter xmon via sysrq, it's kept enabled until system is rebooted, >> even if we exit the debugger. A kernel crash will then lead to xmon >> instance, instead of triggering a kdump procedure (if configured), for >> example. >> >> This patch introduces a debugfs entry for xmon, allowing user to query >> its current state and change it if desired. Basically, the "xmon" file >> to read from/write to is under the debugfs mount point, on powerpc >> directory. Reading this file will provide the current state of the >> debugger, one of the following: "on", "off", "early" or "nobt". Writing >> one of these states to the file will take immediate effect on the debugger. >> >> Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> >> --- >> * I had this patch partially done for some time, and after a discussion >> at the kernel slack channel latest week, I decided to rebase and fix >> some remaining bugs. I'd change 'x' option to always disable the debugger, >> since with this patch we can always re-enable xmon, but today I noticed >> Pan's patch on the mailing list, so perhaps his approach of adding a flag >> to 'x' option is preferable. I can change this in a V2, if requested. >> Thanks in advance! > > xmon state changing after the first sysrq+x violates principle of least > astonishment, so I think that should be fixed. > hi, Nick yes, as long as xmon is disabled during boot, it should still be disabled after existing xmon. My patch does not fix that as it need people add one more char 'z' following 'x'. I will provide a new patch to fix that. > Then the question is, is it worth making it runtime configurable with xmon > command or debugfs tunables? > They are options for people to turn xmon features on or off. Maybe people needn't this. However I am not a fan of debugfs this time as I am used to using xmon cmds. :) Hi, Guilherme So in the end, my thought is that: 1) cmd x|X will exit xmon and keep xmon in the original state(indicated by var xmon_off). 2) Then add options to turn some features on/off. And debugfs maybe not fit for this. But I am also wondering at same time, are people needing this? thanks xinhui > Thanks, > Nick >
"Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> writes: > Currently the xmon debugger is set only via kernel boot command-line. > It's disabled by default, and can be enabled with "xmon=on" on the > command-line. Also, xmon may be accessed via sysrq mechanism, but once > we enter xmon via sysrq, it's kept enabled until system is rebooted, > even if we exit the debugger. A kernel crash will then lead to xmon > instance, instead of triggering a kdump procedure (if configured), for > example. > > This patch introduces a debugfs entry for xmon, allowing user to query > its current state and change it if desired. Basically, the "xmon" file > to read from/write to is under the debugfs mount point, on powerpc > directory. Reading this file will provide the current state of the > debugger, one of the following: "on", "off", "early" or "nobt". Writing > one of these states to the file will take immediate effect on the debugger. I like this in general. But I think we can simplify it a bit. I don't think we need the nobt state anymore. As far as I can see it was added as a way to reinstate the old behaviour when the auto backtrace mode was added, but I've never heard of anyone using it. If anyone hits a crash where they really need that feature they can always just hack the code to disable the backtrace. So I think step 1 is a patch to drop the xmon_no_auto_backtrace stuff. Also I'm not sure EARLY needs to be a separate state. It's just a one-off invocation at boot, all we really need is just a single bit of state communicated from early_parse_xmon() to xmon_setup(), ie. a static bool would do. If we agree with that, then there's only two states left, on and off, in which case it can probably just be an int - and we can use a simple attribute file rather than custom parsing. > * I had this patch partially done for some time, and after a discussion > at the kernel slack channel latest week, I decided to rebase and fix > some remaining bugs. I'd change 'x' option to always disable the debugger, Not quite. 'x' should exit and leave xmon in whatever state it was previously in. cheers
Pan Xinhui <xinhui@linux.vnet.ibm.com> writes: > 在 2017/2/14 10:35, Nicholas Piggin 写道: >> On Mon, 13 Feb 2017 19:00:42 -0200 >> "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> wrote: >>> * I had this patch partially done for some time, and after a discussion >>> at the kernel slack channel latest week, I decided to rebase and fix >>> some remaining bugs. I'd change 'x' option to always disable the debugger, >>> since with this patch we can always re-enable xmon, but today I noticed >>> Pan's patch on the mailing list, so perhaps his approach of adding a flag >>> to 'x' option is preferable. I can change this in a V2, if requested. >>> Thanks in advance! >> >> xmon state changing after the first sysrq+x violates principle of least >> astonishment, so I think that should be fixed. > > Hi, Guilherme > So in the end, my thought is that: > 1) cmd x|X will exit xmon and keep xmon in the original state Agreed. > 2) Then add options to turn some features on/off. And debugfs maybe > not fit for this. But I am also wondering at same time, are people > needing this? I think it's useful. Especially seeing as Guilherme has already written the code. And yeah I think debugfs is the right place to do it. It means you can query and/or set the xmon state without crashing the box - which is what entering xmon essentially does, even if you later recover. cheers
On 14/02/2017 01:58, Pan Xinhui wrote: > > > 在 2017/2/14 10:35, Nicholas Piggin 写道: >> On Mon, 13 Feb 2017 19:00:42 -0200 >> >> xmon state changing after the first sysrq+x violates principle of least >> astonishment, so I think that should be fixed. >> > hi, Nick > yes, as long as xmon is disabled during boot, it should still be disabled after existing xmon. > My patch does not fix that as it need people add one more char 'z' following 'x'. > I will provide a new patch to fix that. > >> Then the question is, is it worth making it runtime configurable with xmon >> command or debugfs tunables? >> > They are options for people to turn xmon features on or off. Maybe people needn't this. > However I am not a fan of debugfs this time as I am used to using xmon cmds. :) > > Hi, Guilherme > So in the end, my thought is that: 1) cmd x|X will exit xmon and keep xmon in the original state(indicated by var xmon_off). > 2) Then add options to turn some features on/off. And debugfs maybe not fit for this. But I am also wondering at same time, are people needing this? Hi Nick and Xinhui, thanks very much for the feedback. I agree, we should keep xmon in the state it was firstly set, on boot time - dropping to the debugger using sysrq shouldn't change it. Now, the use case of the debugfs approach is to allow user to enable/disable xmon without need to drop into the debugger itself, or reboot the machine. Imagine a scenario in which we have a production machine, and: i) For some reason, somebody kept xmon enabled on grub.cfg and now, we want to let kdump work in case of crash - how to disable xmon in runtime? ii) The opposite: xmon wasn't enable on boot time in production machine, but we have a super-rare issue and want to drop to xmon next time it happens, so we need to enable it. But we don't want to drop into the debugger to force it gets enabled, so how do we enable it? Regarding the place of the xmon state file, I believe debugfs is the right place - where else could we add it? procfs? configfs? Thanks, Guilherme > > thanks > xinhui > >> Thanks, >> Nick >> >
On 14/02/2017 09:37, Michael Ellerman wrote: > "Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> writes: > >> Currently the xmon debugger is set only via kernel boot command-line. >> It's disabled by default, and can be enabled with "xmon=on" on the >> command-line. Also, xmon may be accessed via sysrq mechanism, but once >> we enter xmon via sysrq, it's kept enabled until system is rebooted, >> even if we exit the debugger. A kernel crash will then lead to xmon >> instance, instead of triggering a kdump procedure (if configured), for >> example. >> >> This patch introduces a debugfs entry for xmon, allowing user to query >> its current state and change it if desired. Basically, the "xmon" file >> to read from/write to is under the debugfs mount point, on powerpc >> directory. Reading this file will provide the current state of the >> debugger, one of the following: "on", "off", "early" or "nobt". Writing >> one of these states to the file will take immediate effect on the debugger. > > I like this in general. > > But I think we can simplify it a bit. > > I don't think we need the nobt state anymore. As far as I can see it was > added as a way to reinstate the old behaviour when the auto backtrace > mode was added, but I've never heard of anyone using it. > > If anyone hits a crash where they really need that feature they can > always just hack the code to disable the backtrace. > > So I think step 1 is a patch to drop the xmon_no_auto_backtrace stuff. > > Also I'm not sure EARLY needs to be a separate state. It's just a > one-off invocation at boot, all we really need is just a single bit of > state communicated from early_parse_xmon() to xmon_setup(), ie. a static > bool would do. > > If we agree with that, then there's only two states left, on and off, in > which case it can probably just be an int - and we can use a simple > attribute file rather than custom parsing. > Thanks for the good suggestions Michael, I'll change the code and submit a new version. >> * I had this patch partially done for some time, and after a discussion >> at the kernel slack channel latest week, I decided to rebase and fix >> some remaining bugs. I'd change 'x' option to always disable the debugger, > > Not quite. > > 'x' should exit and leave xmon in whatever state it was previously in. Agreed! Thanks, Guilherme > > cheers >
"Guilherme G. Piccoli" <gpiccoli@linux.vnet.ibm.com> writes: > On 14/02/2017 01:58, Pan Xinhui wrote: >> 在 2017/2/14 10:35, Nicholas Piggin 写道: >>> On Mon, 13 Feb 2017 19:00:42 -0200 >>> >>> xmon state changing after the first sysrq+x violates principle of least >>> astonishment, so I think that should be fixed. >>> >> hi, Nick >> yes, as long as xmon is disabled during boot, it should still be disabled after existing xmon. >> My patch does not fix that as it need people add one more char 'z' following 'x'. >> I will provide a new patch to fix that. >> >>> Then the question is, is it worth making it runtime configurable with xmon >>> command or debugfs tunables? >>> >> They are options for people to turn xmon features on or off. Maybe people needn't this. >> However I am not a fan of debugfs this time as I am used to using xmon cmds. :) >> >> Hi, Guilherme >> So in the end, my thought is that: 1) cmd x|X will exit xmon and keep xmon in the original state(indicated by var xmon_off). >> 2) Then add options to turn some features on/off. And debugfs maybe not fit for this. But I am also wondering at same time, are people needing this? > > Hi Nick and Xinhui, thanks very much for the feedback. > I agree, we should keep xmon in the state it was firstly set, on boot > time - dropping to the debugger using sysrq shouldn't change it. > > Now, the use case of the debugfs approach is to allow user to > enable/disable xmon without need to drop into the debugger itself, or > reboot the machine. > Imagine a scenario in which we have a production machine, and: > > > i) For some reason, somebody kept xmon enabled on grub.cfg and now, we > want to let kdump work in case of crash - how to disable xmon in runtime? > > ii) The opposite: xmon wasn't enable on boot time in production machine, > but we have a super-rare issue and want to drop to xmon next time it > happens, so we need to enable it. But we don't want to drop into the > debugger to force it gets enabled, so how do we enable it? Yep. I think both of those are good reasons to add a debugfs file. > Regarding the place of the xmon state file, I believe debugfs is the > right place - where else could we add it? procfs? configfs? debugfs, under powerpc_debugfs_root. cheers
在 2017/2/15 上午1:35, Guilherme G. Piccoli 写道: > On 14/02/2017 01:58, Pan Xinhui wrote: >> >> >> 在 2017/2/14 10:35, Nicholas Piggin 写道: >>> On Mon, 13 Feb 2017 19:00:42 -0200 >>> >>> xmon state changing after the first sysrq+x violates principle of least >>> astonishment, so I think that should be fixed. >>> >> hi, Nick >> yes, as long as xmon is disabled during boot, it should still be disabled after existing xmon. >> My patch does not fix that as it need people add one more char 'z' following 'x'. >> I will provide a new patch to fix that. >> >>> Then the question is, is it worth making it runtime configurable with xmon >>> command or debugfs tunables? >>> >> They are options for people to turn xmon features on or off. Maybe people needn't this. >> However I am not a fan of debugfs this time as I am used to using xmon cmds. :) >> >> Hi, Guilherme >> So in the end, my thought is that: 1) cmd x|X will exit xmon and keep xmon in the original state(indicated by var xmon_off). >> 2) Then add options to turn some features on/off. And debugfs maybe not fit for this. But I am also wondering at same time, are people needing this? > > Hi Nick and Xinhui, thanks very much for the feedback. > I agree, we should keep xmon in the state it was firstly set, on boot > time - dropping to the debugger using sysrq shouldn't change it. > Yes, and feel free to include my fix patch "powerpc/xmon: Fix an unexpected xmon onoff state change" :) > Now, the use case of the debugfs approach is to allow user to > enable/disable xmon without need to drop into the debugger itself, or > reboot the machine. Good, got it. We look forward to your new patch. :) thanks xinhui > Imagine a scenario in which we have a production machine, and: > > > i) For some reason, somebody kept xmon enabled on grub.cfg and now, we > want to let kdump work in case of crash - how to disable xmon in runtime? > > ii) The opposite: xmon wasn't enable on boot time in production machine, > but we have a super-rare issue and want to drop to xmon next time it > happens, so we need to enable it. But we don't want to drop into the > debugger to force it gets enabled, so how do we enable it? > > Regarding the place of the xmon state file, I believe debugfs is the > right place - where else could we add it? procfs? configfs? > > Thanks, > > > Guilherme >> >> thanks >> xinhui >> >>> Thanks, >>> Nick >>> >>
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index 9c0e17c..5fb39db 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -29,6 +29,12 @@ #include <linux/nmi.h> #include <linux/ctype.h> +#ifdef CONFIG_DEBUG_FS +#include <linux/debugfs.h> +#include <linux/uaccess.h> +#include <linux/slab.h> +#endif + #include <asm/ptrace.h> #include <asm/string.h> #include <asm/prom.h> @@ -184,7 +190,12 @@ static void dump_tlb_44x(void); static void dump_tlb_book3e(void); #endif -static int xmon_no_auto_backtrace; +/* xmon_state values */ +#define XMON_OFF 0 +#define XMON_ON 1 +#define XMON_EARLY 2 +#define XMON_NOBT 3 +static int xmon_state; #ifdef CONFIG_PPC64 #define REG "%.16lx" @@ -880,8 +891,8 @@ cmds(struct pt_regs *excp) last_cmd = NULL; xmon_regs = excp; - if (!xmon_no_auto_backtrace) { - xmon_no_auto_backtrace = 1; + if (xmon_state != XMON_NOBT) { + xmon_state = XMON_NOBT; xmon_show_stack(excp->gpr[1], excp->link, excp->nip); } @@ -3244,6 +3255,26 @@ static void xmon_init(int enable) } } +static int parse_xmon(char *p) +{ + if (!p || strncmp(p, "early", 5) == 0) { + /* just "xmon" is equivalent to "xmon=early" */ + xmon_init(1); + xmon_state = XMON_EARLY; + } else if (strncmp(p, "on", 2) == 0) { + xmon_init(1); + xmon_state = XMON_ON; + } else if (strncmp(p, "off", 3) == 0) { + xmon_init(0); + xmon_state = XMON_OFF; + } else if (strncmp(p, "nobt", 4) == 0) + xmon_state = XMON_NOBT; + else + return 1; + + return 0; +} + #ifdef CONFIG_MAGIC_SYSRQ static void sysrq_handle_xmon(int key) { @@ -3266,34 +3297,89 @@ static int __init setup_xmon_sysrq(void) __initcall(setup_xmon_sysrq); #endif /* CONFIG_MAGIC_SYSRQ */ -static int __initdata xmon_early, xmon_off; +#ifdef CONFIG_DEBUG_FS +static ssize_t xmon_dbgfs_read(struct file *file, char __user *ubuffer, + size_t len, loff_t *offset) +{ + int buf_len = 0; + char buf[6] = { 0 }; -static int __init early_parse_xmon(char *p) + switch (xmon_state) { + case XMON_OFF: + buf_len = sprintf(buf, "off"); + break; + case XMON_ON: + buf_len = sprintf(buf, "on"); + break; + case XMON_EARLY: + buf_len = sprintf(buf, "early"); + break; + case XMON_NOBT: + buf_len = sprintf(buf, "nobt"); + break; + } + + return simple_read_from_buffer(ubuffer, len, offset, buf, buf_len); +} + +static ssize_t xmon_dbgfs_write(struct file *file, const char __user *ubuffer, + size_t len, loff_t *offset) { - if (!p || strncmp(p, "early", 5) == 0) { - /* just "xmon" is equivalent to "xmon=early" */ - xmon_init(1); - xmon_early = 1; - } else if (strncmp(p, "on", 2) == 0) - xmon_init(1); - else if (strncmp(p, "off", 3) == 0) - xmon_off = 1; - else if (strncmp(p, "nobt", 4) == 0) - xmon_no_auto_backtrace = 1; - else - return 1; + int ret, not_copied; + char *buf; + + /* Valid states are on, off, early and nobt. */ + if ((*offset != 0) || (len <= 0) || (len > 6)) + return -EINVAL; + + buf = kzalloc(len + 1, GFP_KERNEL); + if (!buf) + return -ENOMEM; + + not_copied = copy_from_user(buf, ubuffer, len); + if (not_copied) { + kfree(buf); + return -EFAULT; + } + ret = parse_xmon(buf); + kfree(buf); + + /* parse_xmon returns 0 on success. */ + if (ret) + return -EINVAL; + return len; +} + +static const struct file_operations xmon_dbgfs_ops = { + .owner = THIS_MODULE, + .open = simple_open, + .read = xmon_dbgfs_read, + .write = xmon_dbgfs_write, +}; + +static int __init setup_xmon_dbgfs(void) +{ + debugfs_create_file("xmon", 0600, powerpc_debugfs_root, NULL, + &xmon_dbgfs_ops); return 0; } +__initcall(setup_xmon_dbgfs); +#endif /*CONFIG_DEBUG_FS*/ + +static int __init early_parse_xmon(char *p) +{ + return parse_xmon(p); +} early_param("xmon", early_parse_xmon); void __init xmon_setup(void) { #ifdef CONFIG_XMON_DEFAULT - if (!xmon_off) + if (xmon_state) /* XMON_OFF */ xmon_init(1); #endif - if (xmon_early) + if (xmon_state == XMON_EARLY) debugger(NULL); }
Currently the xmon debugger is set only via kernel boot command-line. It's disabled by default, and can be enabled with "xmon=on" on the command-line. Also, xmon may be accessed via sysrq mechanism, but once we enter xmon via sysrq, it's kept enabled until system is rebooted, even if we exit the debugger. A kernel crash will then lead to xmon instance, instead of triggering a kdump procedure (if configured), for example. This patch introduces a debugfs entry for xmon, allowing user to query its current state and change it if desired. Basically, the "xmon" file to read from/write to is under the debugfs mount point, on powerpc directory. Reading this file will provide the current state of the debugger, one of the following: "on", "off", "early" or "nobt". Writing one of these states to the file will take immediate effect on the debugger. Signed-off-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com> --- * I had this patch partially done for some time, and after a discussion at the kernel slack channel latest week, I decided to rebase and fix some remaining bugs. I'd change 'x' option to always disable the debugger, since with this patch we can always re-enable xmon, but today I noticed Pan's patch on the mailing list, so perhaps his approach of adding a flag to 'x' option is preferable. I can change this in a V2, if requested. Thanks in advance! arch/powerpc/xmon/xmon.c | 124 +++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 105 insertions(+), 19 deletions(-)