Re: [PATCH] oom: split out forced OOM killer

[Michal Hocko]

On Tue 09-06-15 15:45:35, David Rientjes wrote:
> On Tue, 9 Jun 2015, Michal Hocko wrote:
> 
> > > 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?
> > 
> 
> Perhaps if you renamed "force_kill" to "sysrq" it would make more sense to 
> you?

The naming is not _the_ problem.

> I don't think the oom killer needs multiple entry points that duplicates 
> code and adds more than twice the lines it removes.  It would make sense 
> if that was an optimization in a hot path, or a warm path, or even a 
> luke-warm path, but not an icy cold path like the oom killer.  

This is not trying to optimize for speed. It is a clean up for
_readability_ and _maintainability_ which is considerably better after
the patch because responsibilities of both paths are clear and sysrq
path doesn't have to care about whatever special handling the oom path
wants to care. It is _that_ simple.

> check_panic_on_oom() can simply do

> 
> 	if (sysrq)
> 		return;

and then do the same thing for panic on no killable task and then for
all other cases which are of no relevance for the sysrq path which we
come up later potentially.

This level of argumentation is just ridiculous. You are blocking a
useful cleanup which also fixes a really non-intuitive behavior. I admit
that nobody was complaining about this behavior so this is nothing
urgent but if we go with panic_on_oom_timeout proposal posted in other
email thread then I expect panic_on_oom would be usable much more and
then it would matter much more.

> It's not hard and it's very clear.  We don't need 35 more lines of code to 
> do this.

Sure we do not _need_ it and we definitely can _clutter_ the code even
more.

I do not think your objections are justified. It is natural and a good
practice to split code paths which have different requirements rather
than differentiate them with multiple checks in the common path (some of
them even very subtle). It is a common practice to split up common
infrastructure in helper functions and reuse them when needed. But I
guess I do not have teach you this trivial things...

</bunfight> from my side

Andrew do whatever you like with the patch but I find the level of
argumentation in this thread as not reasonable (I would even consider it
trolling at some parts) and not sufficient for a nack.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/