Message-ID: <20150609093659.GA29057@dhcp22.suse.cz>
Date:	Tue, 9 Jun 2015 11:36:59 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	David Rientjes <rientjes@...gle.com>
Cc:	Austin S Hemmelgarn <ahferroin7@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>, linux-mm@...ck.org,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] oom: split out forced OOM killer

On Mon 08-06-15 16:06:07, David Rientjes wrote:
> 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.

I completely detest yet another check in out_of_memory. And there is
even no reason to do that. Forced kill and genuine oom have different
objectives and combining those two just makes the code harder to read
(one has to go to check the syrq callback to realize that the forced
path is triggered from the workqueue context and that current->mm !=
NULL check will prevent some heuristics. This is just too ugly to
live). So why the heck are you pushing for keeping everything in a
single path?

That being said, I have no problem to do 3 patches, where two of them
would add force check for check_panic_on_oom and panic on no killable
task and only then pull out force_out_of_memory to make it readable
again and drop force checks but I do not see much point in this
juggling.

> > > 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).

I have no problem to be more explicit about the behavior of course. I
can fold it to the original patch.
---
>From b2e611dd5d2589eb82c800e326a9c12da33958d5 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@...e.cz>
Date: Tue, 9 Jun 2015 08:49:09 +0200
Subject: [PATCH] Update the sysrq+f documentnation.

Requested-by: David Rientjes <rientjes@...gle.com>
Signed-off-by: Michal Hocko <mhocko@...e.cz>
---
 Documentation/sysrq.txt | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/sysrq.txt b/Documentation/sysrq.txt
index 0e307c94809a..a5dd88b0aede 100644
--- a/Documentation/sysrq.txt
+++ b/Documentation/sysrq.txt
@@ -75,7 +75,10 @@ On other - If you know of the key combos for other architectures, please
 
 'e'     - Send a SIGTERM to all processes, except for init.
 
-'f'	- Will call oom_kill to kill a memory hog process.
+'f'	- Will call oom_kill to kill a memory hog process. Please note that
+	  an ongoing OOM killer is ignored and a task is killed even though
+	  there was an oom victim selected already. panic_on_oom is ignored
+	  and the system doesn't panic if there are no oom killable tasks.
 
 'g'	- Used by kgdb (kernel debugger)
 
-- 
2.1.4


-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists