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

From: Michal Hocko
Date: Fri Jun 19 2015 - 03:13:26 EST


On Thu 18-06-15 12:27:12, David Rientjes wrote:
> On Thu, 18 Jun 2015, Michal Hocko wrote:
>
> > The forced OOM killing is currently wired into out_of_memory() call
> > even though their objective is different which makes the code ugly
> > and harder to follow. Generic out_of_memory path has to deal with
> > configuration settings and heuristics which are completely irrelevant
> > to the forced OOM killer (e.g. sysctl_oom_kill_allocating_task or
> > OOM killer prevention for already dying tasks). All of them are
> > either relying on explicit force_kill check or indirectly by checking
> > current->mm which is always NULL for sysrq+f. This is not nice, hard
> > to follow and error prone.
> >
> > Let's pull forced OOM killer code out into a separate function
> > (force_out_of_memory) which is really trivial now.
> > As a bonus we can clearly state that this is a forced OOM killer
> > in the OOM message which is helpful to distinguish it from the
> > regular OOM killer.
> >
>
> Ok, so this patch reverts _everything_ in the first patch other than the
> documentation. Just start with this patch instead, sheesh.

The ordering is intentional. Clean up on top of the fix. And considering
how much you "loved" the previous attempt of the cleanup I had even
stronger reason to put this on top of the fix.

> > Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
> > ---
> > drivers/tty/sysrq.c | 3 +--
> > include/linux/oom.h | 3 ++-
> > mm/oom_kill.c | 57 ++++++++++++++++++++++++++++++++---------------------
> > mm/page_alloc.c | 2 +-
> > 4 files changed, 39 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 3a42b7187b8e..06a95a8ed701 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -356,8 +356,7 @@ static struct sysrq_key_op sysrq_term_op = {
> > static void moom_callback(struct work_struct *ignored)
> > {
> > mutex_lock(&oom_lock);
> > - if (!out_of_memory(node_zonelist(first_memory_node, GFP_KERNEL),
> > - GFP_KERNEL, 0, NULL, true))
> > + if (!force_out_of_memory())
> > pr_info("OOM request ignored because killer is disabled\n");
> > mutex_unlock(&oom_lock);
> > }
> > diff --git a/include/linux/oom.h b/include/linux/oom.h
> > index 7deecb7bca5e..061e0ffd3493 100644
> > --- a/include/linux/oom.h
> > +++ b/include/linux/oom.h
> > @@ -70,8 +70,9 @@ extern enum oom_scan_t oom_scan_process_thread(struct task_struct *task,
> > unsigned long totalpages, const nodemask_t *nodemask,
> > bool force_kill);
> >
> > +extern bool force_out_of_memory(void);
> > extern bool out_of_memory(struct zonelist *zonelist, gfp_t gfp_mask,
> > - int order, nodemask_t *mask, bool force_kill);
> > + int order, nodemask_t *mask);
> >
> > extern void exit_oom_victim(void);
> >
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 0c312eaac834..050936f35944 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -635,12 +635,38 @@ int unregister_oom_notifier(struct notifier_block *nb)
> > EXPORT_SYMBOL_GPL(unregister_oom_notifier);
> >
> > /**
> > - * __out_of_memory - kill the "best" process when we run out of memory
> > + * force_out_of_memory - forces OOM killer
>
> ... to kill a process.

OK

> > + *
> > + * External trigger for the OOM killer. The system doesn't have to be under
> > + * OOM condition (e.g. sysrq+f).
> > + */
>
> I'm still not sure what you mean by external. I assume you're referring
> to induced by userspace rather than the kernel. I think you should use
> the word "explicit".

OK
---
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 5af8b5e44b27..7783a3760c56 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -635,9 +635,9 @@ int unregister_oom_notifier(struct notifier_block *nb)
EXPORT_SYMBOL_GPL(unregister_oom_notifier);

/**
- * force_out_of_memory - forces OOM killer
+ * force_out_of_memory - forces OOM killer to kill a process
*
- * External trigger for the OOM killer. The system doesn't have to be under
+ * Explicitly trigger the OOM killer. The system doesn't have to be under
* OOM condition (e.g. sysrq+f).
*/
bool force_out_of_memory(void)
--
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/