[lkml]   [2015]   [Jun]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH] oom: split out forced OOM killer
On Mon, 8 Jun 2015, Michal Hocko wrote:

> > This patch is not a functional change, so I don't interpret your feedback
> > as any support of it being merged.
> David, have you actually read the patch? The changelog is mentioning this:
> "
> check_panic_on_oom on the other hand will work and that is kind of
> unexpected because sysrq+f should be usable to kill a mem hog whether
> the global OOM policy is to panic or not.
> It also doesn't make much sense to panic the system when no task cannot
> be killed because admin has a separate sysrq for that purpose.
> "
> and the patch exludes panic_on_oom from the sysrq path.

Yes, and that's why I believe we should pursue that direction without the
associated "cleanup" that adds 35 lines of code to supress a panic. In
other words, there's no reason to combine a patch that suppresses the
panic even with panic_on_oom, which I support, and a "cleanup" that I
believe just obfuscates the code.

It's a one-liner change: just test for force_kill and suppress the panic;
we don't need 35 new lines that create even more unique entry paths.

> > That said, you raise an interesting point of whether sysrq+f should ever
> > trigger a panic due to panic_on_oom. The case can be made that it should
> > ignore panic_on_oom and require the use of another sysrq to panic the
> > machine instead. Sysrq+f could then be used to oom kill a process,
> > regardless of panic_on_oom, and the panic only occurs if userspace did not
> > trigger the kill or the kill itself will fail.
> Why would it panic the system if there is no killable task? Shoudln't
> be admin able to do additional steps after the explicit oom killer failed
> and only then panic by sysrq?

Today it panics, I don't think it should panic when there are no killable
processes because it's inherently racy with userspace. It's similar to
suppressing panic_on_oom for sysrq+f, but for a different reason, so it
should probably be a separate patch with its own changelog (and update to
documentation for both patches to make this explicit).

 \ /
  Last update: 2015-06-09 01:21    [W:0.052 / U:13.864 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site