Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1366290 > unrolled thread

[RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-03-29 15:30 +0200
    Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-03-29 15:50 +0200
      Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-03-29 16:30 +0200
        Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-03-29 17:30 +0200
    Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-03-29 16:20 +0200
    Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory David Rientjes <rientjes@google.com> - 2016-03-30 00:20 +0200
      Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-03-30 11:50 +0200
        Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-03-30 13:50 +0200
          Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-03-30 14:20 +0200
            Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-03-31 14:00 +0200
              Re: [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-03-31 17:20 +0200

#1366290 — [RFC PATCH] mm, oom: move GFP_NOFS check to out_of_memory

From: Michal Hocko <mhocko@suse.com>

__alloc_pages_may_oom is the central place to decide when the
out_of_memory should be invoked. This is a good approach for most checks
there because they are page allocator specific and the allocation fails
right after.

The notable exception is GFP_NOFS context which is faking
did_some_progress and keep the page allocator looping even though there
couldn't have been any progress from the OOM killer. This patch doesn't
change this behavior because we are not ready to allow those allocation
requests to fail yet. Instead __GFP_FS check is moved down to
out_of_memory and prevent from OOM victim selection there. There are
two reasons for that
	- OOM notifiers might release some memory even from this context
	  as none of the registered notifier seems to be FS related
	- this might help a dying thread to get an access to memory
          reserves and move on which will make the behavior more
          consistent with the case when the task gets killed from a
          different context.

Keep a comment in __alloc_pages_may_oom to make sure we do not forget
how GFP_NOFS is special and that we really want to do something about
it.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---

Hi,
I am sending this as an RFC now even though I think this makes more
sense than what we have right now. Maybe there are some side effects
I do not see, though. A more tricky part is the OOM notifier part
becasue future notifiers might decide to depend on the FS and we can
lockup. Is this something to worry about, though? Would such a notifier
be correct at all? I would call it broken as it would put OOM killer out
of the way on the contended system which is a plain bug IMHO.

If this looks like a reasonable approach I would go on think about how
we can extend this for the oom_reaper and queue the current thread for
the reaper to free some of the memory.

Any thoughts

 mm/oom_kill.c   |  4 ++++
 mm/page_alloc.c | 24 ++++++++++--------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 86349586eacb..1c2b7a82f0c4 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
 		return true;
 	}
 
+	/* The OOM killer does not compensate for IO-less reclaim. */
+	if (!(oc->gfp_mask & __GFP_FS))
+		return true;
+
 	/*
 	 * Check if there were limitations on the allocation (only relevant for
 	 * NUMA) that may require different handling.
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1b889dba7bd4..736ea28abfcf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2872,22 +2872,18 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 		/* The OOM killer does not needlessly kill tasks for lowmem */
 		if (ac->high_zoneidx < ZONE_NORMAL)
 			goto out;
-		/* The OOM killer does not compensate for IO-less reclaim */
-		if (!(gfp_mask & __GFP_FS)) {
-			/*
-			 * XXX: Page reclaim didn't yield anything,
-			 * and the OOM killer can't be invoked, but
-			 * keep looping as per tradition.
-			 *
-			 * But do not keep looping if oom_killer_disable()
-			 * was already called, for the system is trying to
-			 * enter a quiescent state during suspend.
-			 */
-			*did_some_progress = !oom_killer_disabled;
-			goto out;
-		}
 		if (pm_suspended_storage())
 			goto out;
+		/*
+		 * XXX: GFP_NOFS allocations should rather fail than rely on
+		 * other request to make a forward progress.
+		 * We are in an unfortunate situation where out_of_memory cannot
+		 * do much for this context but let's try it to at least get
+		 * access to memory reserved if the current task is killed (see
+		 * out_of_memory). Once filesystems are ready to handle allocation
+		 * failures more gracefully we should just bail out here.
+		 */
+
 		/* The OOM killer may not free memory on a specific node */
 		if (gfp_mask & __GFP_THISNODE)
 			goto out;
-- 
2.7.0

[toc] | [next] | [standalone]

#1366299

Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> __alloc_pages_may_oom is the central place to decide when the
> out_of_memory should be invoked. This is a good approach for most checks
> there because they are page allocator specific and the allocation fails
> right after.
> 
> The notable exception is GFP_NOFS context which is faking
> did_some_progress and keep the page allocator looping even though there
> couldn't have been any progress from the OOM killer. This patch doesn't
> change this behavior because we are not ready to allow those allocation
> requests to fail yet. Instead __GFP_FS check is moved down to
> out_of_memory and prevent from OOM victim selection there. There are
> two reasons for that
> 	- OOM notifiers might release some memory even from this context
> 	  as none of the registered notifier seems to be FS related
> 	- this might help a dying thread to get an access to memory
>           reserves and move on which will make the behavior more
>           consistent with the case when the task gets killed from a
>           different context.

Allowing !__GFP_FS allocations to get TIF_MEMDIE by calling the shortcuts in
out_of_memory() would be fine. But I don't like the direction you want to go.

I don't like failing !__GFP_FS allocations without selecting OOM victim
( http://lkml.kernel.org/r/201603252054.ADH30264.OJQFFLMOHFSOVt@I-love.SAKURA.ne.jp ).

Also, I suggested removing all shortcuts by setting TIF_MEMDIE from oom_kill_process()
( http://lkml.kernel.org/r/1458529634-5951-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp ).

> 
> Keep a comment in __alloc_pages_may_oom to make sure we do not forget
> how GFP_NOFS is special and that we really want to do something about
> it.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> 
> Hi,
> I am sending this as an RFC now even though I think this makes more
> sense than what we have right now. Maybe there are some side effects
> I do not see, though. A more tricky part is the OOM notifier part
> becasue future notifiers might decide to depend on the FS and we can
> lockup. Is this something to worry about, though? Would such a notifier
> be correct at all? I would call it broken as it would put OOM killer out
> of the way on the contended system which is a plain bug IMHO.
> 
> If this looks like a reasonable approach I would go on think about how
> we can extend this for the oom_reaper and queue the current thread for
> the reaper to free some of the memory.
> 
> Any thoughts

[toc] | [prev] | [next] | [standalone]

#1366336

On Tue 29-03-16 22:45:40, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > __alloc_pages_may_oom is the central place to decide when the
> > out_of_memory should be invoked. This is a good approach for most checks
> > there because they are page allocator specific and the allocation fails
> > right after.
> > 
> > The notable exception is GFP_NOFS context which is faking
> > did_some_progress and keep the page allocator looping even though there
> > couldn't have been any progress from the OOM killer. This patch doesn't
> > change this behavior because we are not ready to allow those allocation
> > requests to fail yet. Instead __GFP_FS check is moved down to
> > out_of_memory and prevent from OOM victim selection there. There are
> > two reasons for that
> > 	- OOM notifiers might release some memory even from this context
> > 	  as none of the registered notifier seems to be FS related
> > 	- this might help a dying thread to get an access to memory
> >           reserves and move on which will make the behavior more
> >           consistent with the case when the task gets killed from a
> >           different context.
> 
> Allowing !__GFP_FS allocations to get TIF_MEMDIE by calling the shortcuts in
> out_of_memory() would be fine. But I don't like the direction you want to go.
> 
> I don't like failing !__GFP_FS allocations without selecting OOM victim
> ( http://lkml.kernel.org/r/201603252054.ADH30264.OJQFFLMOHFSOVt@I-love.SAKURA.ne.jp ).

I didn't get to read and digest that email yet but from a quick glance
it doesn't seem to be directly related to this patch. Even if we decide
that __GFP_FS vs. OOM killer logic is flawed for some reason then would
build on top as granting the access to memory reserves is not against
it.

> Also, I suggested removing all shortcuts by setting TIF_MEMDIE from oom_kill_process()
> ( http://lkml.kernel.org/r/1458529634-5951-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp ).

I personally do not like this much. I believe we have already tried to
explain why we have (some of) those shortcuts. They might be too
optimistic and there is a room for improvements for sure but I am not
convinced we can get rid of them that easily.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1366387

Michal Hocko wrote:
> On Tue 29-03-16 22:45:40, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > From: Michal Hocko <mhocko@suse.com>
> > > 
> > > __alloc_pages_may_oom is the central place to decide when the
> > > out_of_memory should be invoked. This is a good approach for most checks
> > > there because they are page allocator specific and the allocation fails
> > > right after.
> > > 
> > > The notable exception is GFP_NOFS context which is faking
> > > did_some_progress and keep the page allocator looping even though there
> > > couldn't have been any progress from the OOM killer. This patch doesn't
> > > change this behavior because we are not ready to allow those allocation
> > > requests to fail yet. Instead __GFP_FS check is moved down to
> > > out_of_memory and prevent from OOM victim selection there. There are
> > > two reasons for that
> > > 	- OOM notifiers might release some memory even from this context
> > > 	  as none of the registered notifier seems to be FS related
> > > 	- this might help a dying thread to get an access to memory
> > >           reserves and move on which will make the behavior more
> > >           consistent with the case when the task gets killed from a
> > >           different context.
> > 
> > Allowing !__GFP_FS allocations to get TIF_MEMDIE by calling the shortcuts in
> > out_of_memory() would be fine. But I don't like the direction you want to go.
> > 
> > I don't like failing !__GFP_FS allocations without selecting OOM victim
> > ( http://lkml.kernel.org/r/201603252054.ADH30264.OJQFFLMOHFSOVt@I-love.SAKURA.ne.jp ).
> 
> I didn't get to read and digest that email yet but from a quick glance
> it doesn't seem to be directly related to this patch. Even if we decide
> that __GFP_FS vs. OOM killer logic is flawed for some reason then would
> build on top as granting the access to memory reserves is not against
> it.
> 

I think that removing these shortcuts is better.

> > Also, I suggested removing all shortcuts by setting TIF_MEMDIE from oom_kill_process()
> > ( http://lkml.kernel.org/r/1458529634-5951-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp ).
> 
> I personally do not like this much. I believe we have already tried to
> explain why we have (some of) those shortcuts. They might be too
> optimistic and there is a room for improvements for sure but I am not
> convinced we can get rid of them that easily.

These shortcuts are too optimistic. They assume that the target thread can call
exit_oom_victim() but the reality is that the target task can get stuck at
down_read(&mm->mmap_sem) in exit_mm(). If SIGKILL were sent to all thread
groups sharing that mm, the possibility of the target thread getting stuck at
down_read(&mm->mmap_sem) in exit_mm() is significantly reduced.

http://lkml.kernel.org/r/20160329141442.GD4466@dhcp22.suse.cz tried to let
the OOM reaper to call exit_oom_victim() on behalf of the target thread
by waking up the OOM reaper. But the OOM reaper won't call exit_oom_victim()
because the OOM reaper will fail to reap memory because some thread sharing
that mm and holding mm->mmap_sem for write will not receive SIGKILL if we use
these shortcuts. As far as I know, all existing explanations for why we have
these shortcuts are ignoring the possibility of such some thread.

[toc] | [prev] | [next] | [standalone]

#1366325

On Tue 29-03-16 15:27:35, Michal Hocko wrote:
[...]
> If this looks like a reasonable approach I would go on think about how
> we can extend this for the oom_reaper and queue the current thread for
> the reaper to free some of the memory.

And this is what I came up with (untested yet). Doesn't too bad to me:
---
From 1129d802a6feff7fa04b582701d11f556a149f12 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Tue, 29 Mar 2016 16:04:10 +0200
Subject: [PATCH] oom, oom_reaper: Try to reap tasks which skip regular OOM
 killer path

If either the current task is already killed or PF_EXITING or a selected
task is PF_EXITING then the oom killer is suppressed and so is the oom
reaper. This patch adds try_oom_reaper which checks the given task
and queues it for the oom reaper if that is safe to be done meaning
that the task doesn't share the mm with an alive process.

This might help to release the memory pressure while the task tries to
exit.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 18 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 1c2b7a82f0c4..2f637728b12a 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -412,6 +412,25 @@ bool oom_killer_disabled __read_mostly;
 
 #define K(x) ((x) << (PAGE_SHIFT-10))
 
+/*
+ * task->mm can be NULL if the task is the exited group leader.  So to
+ * determine whether the task is using a particular mm, we examine all the
+ * task's threads: if one of those is using this mm then this task was also
+ * using it.
+ */
+static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
+{
+	struct task_struct *t;
+
+	for_each_thread(p, t) {
+		struct mm_struct *t_mm = READ_ONCE(t->mm);
+		if (t_mm)
+			return t_mm == mm;
+	}
+	return false;
+}
+
+
 #ifdef CONFIG_MMU
 /*
  * OOM Reaper kernel thread which tries to reap the memory used by the OOM
@@ -563,6 +582,45 @@ static void wake_oom_reaper(struct task_struct *tsk)
 	wake_up(&oom_reaper_wait);
 }
 
+/* Check if we can reap the given task. This has to be called with stable
+ * tsk->mm
+ */
+static void try_oom_reaper(struct task_struct *tsk)
+{
+	struct mm_struct *mm = tsk->mm;
+	bool can_oom_reap = true;
+	struct task_struct *p;
+
+	if (!mm)
+		return;
+
+	/*
+	 * There might be other threads/processes which are either not
+	 * dying or even not killable.
+	 */
+	if (atomic_read(&mm->mm_users) > 1) {
+		rcu_read_lock();
+		for_each_process(p) {
+			if (!process_shares_mm(p, mm))
+				continue;
+			if (same_thread_group(p, tsk))
+				continue;
+			/*
+			 * other process sharing the mm is not dying so we cannot
+			 * simply reap the address space.
+			 */
+			if (!fatal_signal_pending(p) || !task_will_free_mem(p)) {
+				can_oom_reap = false;
+				break;
+			}
+		}
+		rcu_read_unlock();
+	}
+
+	if (can_oom_reap)
+		wake_oom_reaper(tsk);
+}
+
 static int __init oom_init(void)
 {
 	oom_reaper_th = kthread_run(oom_reaper, NULL, "oom_reaper");
@@ -575,6 +633,10 @@ static int __init oom_init(void)
 }
 subsys_initcall(oom_init)
 #else
+static void try_oom_reaper(struct task_struct *tsk)
+{
+}
+
 static void wake_oom_reaper(struct task_struct *tsk)
 {
 }
@@ -653,24 +715,6 @@ void oom_killer_enable(void)
 }
 
 /*
- * task->mm can be NULL if the task is the exited group leader.  So to
- * determine whether the task is using a particular mm, we examine all the
- * task's threads: if one of those is using this mm then this task was also
- * using it.
- */
-static bool process_shares_mm(struct task_struct *p, struct mm_struct *mm)
-{
-	struct task_struct *t;
-
-	for_each_thread(p, t) {
-		struct mm_struct *t_mm = READ_ONCE(t->mm);
-		if (t_mm)
-			return t_mm == mm;
-	}
-	return false;
-}
-
-/*
  * Must be called while holding a reference to p, which will be released upon
  * returning.
  */
@@ -694,6 +738,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
 	task_lock(p);
 	if (p->mm && task_will_free_mem(p)) {
 		mark_oom_victim(p);
+		try_oom_reaper(p);
 		task_unlock(p);
 		put_task_struct(p);
 		return;
@@ -873,6 +918,7 @@ bool out_of_memory(struct oom_control *oc)
 	if (current->mm &&
 	    (fatal_signal_pending(current) || task_will_free_mem(current))) {
 		mark_oom_victim(current);
+		try_oom_reaper(current);
 		return true;
 	}
 
-- 
2.7.0

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1366751

On Tue, 29 Mar 2016, Michal Hocko wrote:

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 86349586eacb..1c2b7a82f0c4 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
>  		return true;
>  	}
>  
> +	/* The OOM killer does not compensate for IO-less reclaim. */
> +	if (!(oc->gfp_mask & __GFP_FS))
> +		return true;
> +
>  	/*
>  	 * Check if there were limitations on the allocation (only relevant for
>  	 * NUMA) that may require different handling.

I don't object to this necessarily, but I think we need input from those 
that have taken the time to implement their own oom notifier to see if 
they agree.  In the past, they would only be called if reclaim has 
completely failed; now, they can be called in low memory situations when 
reclaim has had very little chance to be successful.  Getting an ack from 
them would be helpful.

I also think we have discussed this before, but I think the oom notifier 
handling should be in done in the page allocator proper, i.e. in 
__alloc_pages_may_oom().  We can leave out_of_memory() for a clear defined 
purpose: to kill a process when all reclaim has failed.

[toc] | [prev] | [next] | [standalone]

#1367051

On Tue 29-03-16 15:13:54, David Rientjes wrote:
> On Tue, 29 Mar 2016, Michal Hocko wrote:
> 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 86349586eacb..1c2b7a82f0c4 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
> >  		return true;
> >  	}
> >  
> > +	/* The OOM killer does not compensate for IO-less reclaim. */
> > +	if (!(oc->gfp_mask & __GFP_FS))
> > +		return true;
> > +
> >  	/*
> >  	 * Check if there were limitations on the allocation (only relevant for
> >  	 * NUMA) that may require different handling.
> 
> I don't object to this necessarily, but I think we need input from those 
> that have taken the time to implement their own oom notifier to see if 
> they agree.  In the past, they would only be called if reclaim has 
> completely failed; now, they can be called in low memory situations when 
> reclaim has had very little chance to be successful.  Getting an ack from 
> them would be helpful.

I will make sure to put them on the CC and mention this in the changelog
when I post this next time. I personally think that this shouldn't make
much difference in the real life because GFP_NOFS only loads are rare
and we should rather help by releasing memory when it is available
rather than rely on something else to do it for us. Waiting for Godot is
never a good strategy.

> I also think we have discussed this before, but I think the oom notifier 
> handling should be in done in the page allocator proper, i.e. in 
> __alloc_pages_may_oom().  We can leave out_of_memory() for a clear defined 
> purpose: to kill a process when all reclaim has failed.

I vaguely remember there was some issue with that the last time we have
discussed that. It was the duplication from the page fault and allocator
paths AFAIR. Nothing that cannot be handled though but the OOM notifier
API is just too ugly to spread outside OOM proper I guess. Why we cannot
move those users to use proper shrinkers interface (after it gets
extended by a priority of some sort and release some objects only after
we are really in troubles)? Something for a separate discussion,
though...

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1367129

Michal Hocko wrote:
> On Tue 29-03-16 15:13:54, David Rientjes wrote:
> > On Tue, 29 Mar 2016, Michal Hocko wrote:
> > 
> > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > index 86349586eacb..1c2b7a82f0c4 100644
> > > --- a/mm/oom_kill.c
> > > +++ b/mm/oom_kill.c
> > > @@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
> > >  		return true;
> > >  	}
> > >  
> > > +	/* The OOM killer does not compensate for IO-less reclaim. */
> > > +	if (!(oc->gfp_mask & __GFP_FS))
> > > +		return true;
> > > +

This patch will disable pagefault_out_of_memory() because currently
pagefault_out_of_memory() is passing oc->gfp_mask == 0.

Because of current behavior, calling oom notifiers from !__GFP_FS seems
to be safe.

> > >  	/*
> > >  	 * Check if there were limitations on the allocation (only relevant for
> > >  	 * NUMA) that may require different handling.
> > 
> > I don't object to this necessarily, but I think we need input from those 
> > that have taken the time to implement their own oom notifier to see if 
> > they agree.  In the past, they would only be called if reclaim has 
> > completely failed; now, they can be called in low memory situations when 
> > reclaim has had very little chance to be successful.  Getting an ack from 
> > them would be helpful.
> 
> I will make sure to put them on the CC and mention this in the changelog
> when I post this next time. I personally think that this shouldn't make
> much difference in the real life because GFP_NOFS only loads are rare

GFP_NOFS only loads are rare. But some GFP_KERNEL load which got TIF_MEMDIE
might be waiting for GFP_NOFS or GFP_NOIO loads to make progress.

I think we are not ready to handle situations where out_of_memory() is called
again after current thread got TIF_MEMDIE due to __GFP_NOFAIL allocation
request when we ran out of memory reserves. We should not assume that the
victim target thread does not have TIF_MEMDIE yet. I think we can handle it
by making mark_oom_victim() return a bool and return via shortcut only if
mark_oom_victim() successfully set TIF_MEMDIE. Though I don't like the
shortcut approach that lacks a guaranteed unlocking mechanism.

> and we should rather help by releasing memory when it is available
> rather than rely on something else to do it for us. Waiting for Godot is
> never a good strategy.
> 
> > I also think we have discussed this before, but I think the oom notifier 
> > handling should be in done in the page allocator proper, i.e. in 
> > __alloc_pages_may_oom().  We can leave out_of_memory() for a clear defined 
> > purpose: to kill a process when all reclaim has failed.
> 
> I vaguely remember there was some issue with that the last time we have
> discussed that. It was the duplication from the page fault and allocator
> paths AFAIR. Nothing that cannot be handled though but the OOM notifier
> API is just too ugly to spread outside OOM proper I guess. Why we cannot
> move those users to use proper shrinkers interface (after it gets
> extended by a priority of some sort and release some objects only after
> we are really in troubles)? Something for a separate discussion,
> though...

Calling oom notifiers from SysRq-f is what we want?

[toc] | [prev] | [next] | [standalone]

#1367159

On Wed 30-03-16 20:46:48, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Tue 29-03-16 15:13:54, David Rientjes wrote:
> > > On Tue, 29 Mar 2016, Michal Hocko wrote:
> > > 
> > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > index 86349586eacb..1c2b7a82f0c4 100644
> > > > --- a/mm/oom_kill.c
> > > > +++ b/mm/oom_kill.c
> > > > @@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
> > > >  		return true;
> > > >  	}
> > > >  
> > > > +	/* The OOM killer does not compensate for IO-less reclaim. */
> > > > +	if (!(oc->gfp_mask & __GFP_FS))
> > > > +		return true;
> > > > +
> 
> This patch will disable pagefault_out_of_memory() because currently
> pagefault_out_of_memory() is passing oc->gfp_mask == 0.
> 
> Because of current behavior, calling oom notifiers from !__GFP_FS seems
> to be safe.

You are right! I have completely missed that and thought we were
providing GFP_KERNEL there. So we have two choices. Either we do
use GFP_KERNEL (same as we do for sysrq+f) or we special case
pagefault_out_of_memory in some way. The second option seems to be safer
because the gfp_mask has to contain at least ___GFP_DIRECT_RECLAIM to
trigger the OOM path.

> > > >  	/*
> > > >  	 * Check if there were limitations on the allocation (only relevant for
> > > >  	 * NUMA) that may require different handling.
> > > 
> > > I don't object to this necessarily, but I think we need input from those 
> > > that have taken the time to implement their own oom notifier to see if 
> > > they agree.  In the past, they would only be called if reclaim has 
> > > completely failed; now, they can be called in low memory situations when 
> > > reclaim has had very little chance to be successful.  Getting an ack from 
> > > them would be helpful.
> > 
> > I will make sure to put them on the CC and mention this in the changelog
> > when I post this next time. I personally think that this shouldn't make
> > much difference in the real life because GFP_NOFS only loads are rare
> 
> GFP_NOFS only loads are rare. But some GFP_KERNEL load which got TIF_MEMDIE
> might be waiting for GFP_NOFS or GFP_NOIO loads to make progress.

How would that matter to oom notifiers?

> I think we are not ready to handle situations where out_of_memory() is called
> again after current thread got TIF_MEMDIE due to __GFP_NOFAIL allocation
> request when we ran out of memory reserves. We should not assume that the
> victim target thread does not have TIF_MEMDIE yet. I think we can handle it
> by making mark_oom_victim() return a bool and return via shortcut only if
> mark_oom_victim() successfully set TIF_MEMDIE. Though I don't like the
> shortcut approach that lacks a guaranteed unlocking mechanism.

That would lead to premature follow up OOM when TIF_MEMDIE makes some
progress just not in time.
 
> > and we should rather help by releasing memory when it is available
> > rather than rely on something else to do it for us. Waiting for Godot is
> > never a good strategy.
> > 
> > > I also think we have discussed this before, but I think the oom notifier 
> > > handling should be in done in the page allocator proper, i.e. in 
> > > __alloc_pages_may_oom().  We can leave out_of_memory() for a clear defined 
> > > purpose: to kill a process when all reclaim has failed.
> > 
> > I vaguely remember there was some issue with that the last time we have
> > discussed that. It was the duplication from the page fault and allocator
> > paths AFAIR. Nothing that cannot be handled though but the OOM notifier
> > API is just too ugly to spread outside OOM proper I guess. Why we cannot
> > move those users to use proper shrinkers interface (after it gets
> > extended by a priority of some sort and release some objects only after
> > we are really in troubles)? Something for a separate discussion,
> > though...
> 
> Calling oom notifiers from SysRq-f is what we want?

I am not really sure about that to be honest. The semantic is really
weak but what would be a downside? This operation shouldn't be fatal
and dropped object can be reconstructed.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1368225

Michal Hocko wrote:
> On Wed 30-03-16 20:46:48, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Tue 29-03-16 15:13:54, David Rientjes wrote:
> > > > On Tue, 29 Mar 2016, Michal Hocko wrote:
> > > > 
> > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > index 86349586eacb..1c2b7a82f0c4 100644
> > > > > --- a/mm/oom_kill.c
> > > > > +++ b/mm/oom_kill.c
> > > > > @@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
> > > > >  		return true;
> > > > >  	}
> > > > >  
> > > > > +	/* The OOM killer does not compensate for IO-less reclaim. */
> > > > > +	if (!(oc->gfp_mask & __GFP_FS))
> > > > > +		return true;
> > > > > +
> > 
> > This patch will disable pagefault_out_of_memory() because currently
> > pagefault_out_of_memory() is passing oc->gfp_mask == 0.
> > 
> > Because of current behavior, calling oom notifiers from !__GFP_FS seems
> > to be safe.
> 
> You are right! I have completely missed that and thought we were
> providing GFP_KERNEL there. So we have two choices. Either we do
> use GFP_KERNEL (same as we do for sysrq+f) or we special case
> pagefault_out_of_memory in some way. The second option seems to be safer
> because the gfp_mask has to contain at least ___GFP_DIRECT_RECLAIM to
> trigger the OOM path.

Oops, I missed that this patch also disables out_of_memory() for !__GFP_FS &&
__GFP_NOFAIL allocation requests.

> > I think we are not ready to handle situations where out_of_memory() is called
> > again after current thread got TIF_MEMDIE due to __GFP_NOFAIL allocation
> > request when we ran out of memory reserves. We should not assume that the
> > victim target thread does not have TIF_MEMDIE yet. I think we can handle it
> > by making mark_oom_victim() return a bool and return via shortcut only if
> > mark_oom_victim() successfully set TIF_MEMDIE. Though I don't like the
> > shortcut approach that lacks a guaranteed unlocking mechanism.
> 
> That would lead to premature follow up OOM when TIF_MEMDIE makes some
> progress just not in time.

We can never know whether the OOM killer prematurely killed a victim.
It is possible that get_page_from_freelist() will succeed even if
select_bad_process() did not find a TIF_MEMDIE thread. You said you don't
want to violate the layer
( http://lkml.kernel.org/r/20160129152307.GF32174@dhcp22.suse.cz ).

What we can do is tolerate possible premature OOM killer invocation using
some threshold. You are proposing such change as OOM detection rework that
might possibly cause premature OOM killer invocation.
Waiting forever unconditionally (e.g.
http://lkml.kernel.org/r/201602092349.ACG81273.OSVtMJQHLOFOFF@I-love.SAKURA.ne.jp )
is no good. Suppressing OOM killer invocation forever unconditionally (e.g.
decide based on only !__GFP_FS, decide based on only TIF_MEMDIE) is no good.

Even if we stop returning via shortcut by making mark_oom_victim() return a
bool, select_bad_process() will work as hold off mechanism. By combining with
timeout (or something finite one) for TIF_MEMDIE, we can tolerate possible
premature OOM killer invocation. It is much better than OOM-livelocked forever.

[toc] | [prev] | [next] | [standalone]

#1368400

On Thu 31-03-16 20:56:23, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > On Wed 30-03-16 20:46:48, Tetsuo Handa wrote:
> > > Michal Hocko wrote:
> > > > On Tue 29-03-16 15:13:54, David Rientjes wrote:
> > > > > On Tue, 29 Mar 2016, Michal Hocko wrote:
> > > > > 
> > > > > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > > > > > index 86349586eacb..1c2b7a82f0c4 100644
> > > > > > --- a/mm/oom_kill.c
> > > > > > +++ b/mm/oom_kill.c
> > > > > > @@ -876,6 +876,10 @@ bool out_of_memory(struct oom_control *oc)
> > > > > >  		return true;
> > > > > >  	}
> > > > > >  
> > > > > > +	/* The OOM killer does not compensate for IO-less reclaim. */
> > > > > > +	if (!(oc->gfp_mask & __GFP_FS))
> > > > > > +		return true;
> > > > > > +
> > > 
> > > This patch will disable pagefault_out_of_memory() because currently
> > > pagefault_out_of_memory() is passing oc->gfp_mask == 0.
> > > 
> > > Because of current behavior, calling oom notifiers from !__GFP_FS seems
> > > to be safe.
> > 
> > You are right! I have completely missed that and thought we were
> > providing GFP_KERNEL there. So we have two choices. Either we do
> > use GFP_KERNEL (same as we do for sysrq+f) or we special case
> > pagefault_out_of_memory in some way. The second option seems to be safer
> > because the gfp_mask has to contain at least ___GFP_DIRECT_RECLAIM to
> > trigger the OOM path.
> 
> Oops, I missed that this patch also disables out_of_memory() for !__GFP_FS &&
> __GFP_NOFAIL allocation requests.

True. The following should take care of that:

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 54aa4ec06889..32d8210b8773 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -882,7 +882,7 @@ bool out_of_memory(struct oom_control *oc)
 	 * make sure exclude 0 mask - all other users should have at least
 	 * ___GFP_DIRECT_RECLAIM to get here.
 	 */
-	if (oc->gfp_mask && !(oc->gfp_mask & __GFP_FS))
+	if (oc->gfp_mask && !(oc->gfp_mask & (__GFP_FS|__GFP_NOFAIL)))
 		return true;
 
 	/*

Thanks for spotting this!

[...]
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web