Messages in this thread | | | Date | Tue, 21 Jul 2009 14:46:39 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH] sysrq, kdump: fix regression, revert "simplify sysrq-c handler" |
| |
Neil Horman wrote: > On Mon, Jul 20, 2009 at 12:22:57PM -0700, Eric W. Biederman wrote: >> Lai Jiangshan <laijs@cn.fujitsu.com> writes: >> >>> 1) This fix breaks our tools. >>> This fix changes the ABI. panic_on_oops is default 0, >>> and a lots system do not specify the boot option "panic", >>> thus, Sysrq-c will not cause CrashDump(Kdump) as expected. >> How does it break your tools? >> > Well, upstream doesn't really care about ABI stability, particularly in the > sysrq space.
"simplify sysrq-c handler" sounds like a cleanup patch, why we let a cleanup patch changes the ABI?
> That aside however, it seems like sysrq-c is doing the right thing > with my patch in place, namely, attempting to crash the system. If the > panic_on_oops sysctl isn't set, then a crash fails, as expected
The original name is "crashdump", so crash should be performed as expected.
(unlike the prior behavior, which forced a kexec reboot of the system while ignoring the > sysctl, which seems like it would be labeled the unexpected behavior to me. > Regardless, it seems like the right thing to do if you want sysrq-c to do the > right thing from the start is set panic on the kernel command line. Not sure > what the problem is there. > >>> 2) When CONFIG_KEXEC=n, Sysrq-c should become an invalid >>> command like Sysrq-D(CONFIG_LOCKDEP, show-all-locks). >>> But this fix makes it a valid command and let it do a >>> hazard thing: cause a page fault(NULL dereference) in kernel. >>> >>> So, we revert this fix. >> The idea was to extend sysrq-d to also be a way of testing NULL >> pointer dereferences. How is that a bad idea? >> > Agreed, about the only thing that I see as wrong with my change is that I
also the naming.
> neglected to change the documentation. Prior to my change, the behavior was > completely muddled. Sysrq-c would do one of 3 things: > > 1) If kexec wasn't built into the kernel, it would do nothing > 2) If kexec was built into the kernel but not enabled, it would try and fail to > execute a kdump > 3) if kdump was enabled and configured, it would crash
I don't think it's muddled. 1) If kexec wasn't built into the kernel, IT'S NOT A VALID COMMAND. 2),3) It always try to crashdump. not oops, not normal panic.
> > Under the current implementation, you can always crash the kernel (assuming > you've enabled sysrq, and have permission to use it), which will trigger a kdump > (or just crash the system, which is usefull for development in and of its own > right), or will simply record and oops (if panic_on_oops is clear). The only > case that left open is booting a kdump kernel without handling a bad page fault, > which you can do from user space anyway via the kexec -e command. I fail to see > how the previous implementation is superior.
Even I agreed your fix, I don't agreed your naming, For your fix, the correct naming should be:
.help_msg = "oops(C)", .action_msg = "Trigger an oops"
And document it: Sysrq-c always causes an oops by an indirect way. It'll do one of 4 things: 1) panic_on_oops=0, it is just kill the current task. 2) panic_on_oops=1, but CONFIG_KEXEC=n, just normal panic 3) panic_on_oops=1, CONFIG_KEXEC=y, but not enabled, just normal panic 4) panic_on_oops=1, CONFIG_KEXEC=y, kdump was enabled, CrashDump.
Lai.
| |