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

Groups > linux.kernel > #1321042 > unrolled thread

[PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-01-28 21:50 +0100
    Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before  out_of_memory Johannes Weiner <hannes@cmpxchg.org> - 2016-01-28 22:40 +0100
      Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before  out_of_memory David Rientjes <rientjes@google.com> - 2016-01-29 00:20 +0100
        Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before  out_of_memory Johannes Weiner <hannes@cmpxchg.org> - 2016-01-29 01:00 +0100
          Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-01-29 11:40 +0100
          Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before  out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-01-29 16:40 +0100
            Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> - 2016-01-30 13:20 +0100
        Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before  out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-01-29 16:30 +0100
      Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before  out_of_memory Michal Hocko <mhocko@kernel.org> - 2016-01-29 16:30 +0100

#1321042 — [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

From: Michal Hocko <mhocko@suse.com>

__alloc_pages_may_oom has been doing get_page_from_freelist with
ALLOC_WMARK_HIGH target before going out_of_memory and invoking the oom
killer. This has two reasons as explained by Andrea:
"
: the reason for the high wmark is to reduce the likelihood of livelocks
: and be sure to invoke the OOM killer, if we're still under pressure
: and reclaim just failed. The high wmark is used to be sure the failure
: of reclaim isn't going to be ignored. If using the min wmark like
: you propose there's risk of livelock or anyway of delayed OOM killer
: invocation.
:
: The reason for doing one last wmark check (regardless of the wmark
: used) before invoking the oom killer, was just to be sure another OOM
: killer invocation hasn't already freed a ton of memory while we were
: stuck in reclaim. A lot of free memory generated by the OOM killer,
: won't make a parallel reclaim more likely to succeed, it just creates
: free memory, but reclaim only succeeds when it finds "freeable" memory
: and it makes progress in converting it to free memory. So for the
: purpose of this last check, the high wmark would work fine as lots of
: free memory would have been generated in such case.
"

This is no longer a concern after "mm, oom: rework oom detection"
because should_reclaim_retry performs the water mark check right before
__alloc_pages_may_oom is invoked. Remove the last moment allocation
request as it just makes the code more confusing and doesn't really
serve any purpose because a success is basically impossible otherwise
should_reclaim_retry would force the reclaim to retry. So this is
merely a code cleanup rather than a functional change.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 268de1654128..f82941c0ac4e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2743,16 +2743,6 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 		return NULL;
 	}
 
-	/*
-	 * Go through the zonelist yet one more time, keep very high watermark
-	 * here, this is only to catch a parallel oom killing, we must fail if
-	 * we're still under heavy pressure.
-	 */
-	page = get_page_from_freelist(gfp_mask | __GFP_HARDWALL, order,
-					ALLOC_WMARK_HIGH|ALLOC_CPUSET, ac);
-	if (page)
-		goto out;
-
 	if (!(gfp_mask & __GFP_NOFAIL)) {
 		/* Coredumps can quickly deplete all memory reserves */
 		if (current->flags & PF_DUMPCORE)
-- 
2.7.0.rc3

[toc] | [next] | [standalone]

#1321081 — Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

On Thu, Jan 28, 2016 at 09:40:03PM +0100, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> __alloc_pages_may_oom has been doing get_page_from_freelist with
> ALLOC_WMARK_HIGH target before going out_of_memory and invoking the oom
> killer. This has two reasons as explained by Andrea:
> "
> : the reason for the high wmark is to reduce the likelihood of livelocks
> : and be sure to invoke the OOM killer, if we're still under pressure
> : and reclaim just failed. The high wmark is used to be sure the failure
> : of reclaim isn't going to be ignored. If using the min wmark like
> : you propose there's risk of livelock or anyway of delayed OOM killer
> : invocation.
> :
> : The reason for doing one last wmark check (regardless of the wmark
> : used) before invoking the oom killer, was just to be sure another OOM
> : killer invocation hasn't already freed a ton of memory while we were
> : stuck in reclaim. A lot of free memory generated by the OOM killer,
> : won't make a parallel reclaim more likely to succeed, it just creates
> : free memory, but reclaim only succeeds when it finds "freeable" memory
> : and it makes progress in converting it to free memory. So for the
> : purpose of this last check, the high wmark would work fine as lots of
> : free memory would have been generated in such case.
> "
> 
> This is no longer a concern after "mm, oom: rework oom detection"
> because should_reclaim_retry performs the water mark check right before
> __alloc_pages_may_oom is invoked. Remove the last moment allocation
> request as it just makes the code more confusing and doesn't really
> serve any purpose because a success is basically impossible otherwise
> should_reclaim_retry would force the reclaim to retry. So this is
> merely a code cleanup rather than a functional change.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

The check has to happen while holding the OOM lock, otherwise we'll
end up killing much more than necessary when there are many racing
allocations.

Please drop this patch.

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

#1321138 — Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

On Thu, 28 Jan 2016, Johannes Weiner wrote:

> The check has to happen while holding the OOM lock, otherwise we'll
> end up killing much more than necessary when there are many racing
> allocations.
> 

Right, we need to try with ALLOC_WMARK_HIGH after oom_lock has been 
acquired.

The situation is still somewhat fragile, however, but I think it's 
tangential to this patch series.  If the ALLOC_WMARK_HIGH allocation fails 
because an oom victim hasn't freed its memory yet, and then the TIF_MEMDIE 
thread isn't visible during the oom killer's tasklist scan because it has 
exited, we still end up killing more than we should.  The likelihood of 
this happening grows with the length of the tasklist.

Perhaps we should try testing watermarks after a victim has been selected 
and immediately before killing?  (Aside: we actually carry an internal 
patch to test mem_cgroup_margin() in the memcg oom path after selecting a 
victim because we have been hit with this before in the memcg path.)

I would think that retrying with ALLOC_WMARK_HIGH would be enough memory 
to deem that we aren't going to immediately reenter an oom condition so 
the deferred killing is a waste of time.

The downside is how sloppy this would be because it's blurring the line 
between oom killer and page allocator.  We'd need the oom killer to return 
the selected victim to the page allocator, try the allocation, and then 
call oom_kill_process() if necessary.

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

#1321157 — Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

On Thu, Jan 28, 2016 at 03:19:08PM -0800, David Rientjes wrote:
> On Thu, 28 Jan 2016, Johannes Weiner wrote:
> 
> > The check has to happen while holding the OOM lock, otherwise we'll
> > end up killing much more than necessary when there are many racing
> > allocations.
> > 
> 
> Right, we need to try with ALLOC_WMARK_HIGH after oom_lock has been 
> acquired.
> 
> The situation is still somewhat fragile, however, but I think it's 
> tangential to this patch series.  If the ALLOC_WMARK_HIGH allocation fails 
> because an oom victim hasn't freed its memory yet, and then the TIF_MEMDIE 
> thread isn't visible during the oom killer's tasklist scan because it has 
> exited, we still end up killing more than we should.  The likelihood of 
> this happening grows with the length of the tasklist.
> 
> Perhaps we should try testing watermarks after a victim has been selected 
> and immediately before killing?  (Aside: we actually carry an internal 
> patch to test mem_cgroup_margin() in the memcg oom path after selecting a 
> victim because we have been hit with this before in the memcg path.)
> 
> I would think that retrying with ALLOC_WMARK_HIGH would be enough memory 
> to deem that we aren't going to immediately reenter an oom condition so 
> the deferred killing is a waste of time.
> 
> The downside is how sloppy this would be because it's blurring the line 
> between oom killer and page allocator.  We'd need the oom killer to return 
> the selected victim to the page allocator, try the allocation, and then 
> call oom_kill_process() if necessary.

https://lkml.org/lkml/2015/3/25/40

We could have out_of_memory() wait until the number of outstanding OOM
victims drops to 0. Then __alloc_pages_may_oom() doesn't relinquish
the lock until its kill has been finalized:

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 914451a..4dc5b9d 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -892,7 +892,9 @@ bool out_of_memory(struct oom_control *oc)
 		 * Give the killed process a good chance to exit before trying
 		 * to allocate memory again.
 		 */
-		schedule_timeout_killable(1);
+		if (!test_thread_flag(TIF_MEMDIE))
+			wait_event_timeout(oom_victims_wait,
+					   !atomic_read(&oom_victims), HZ);
 	}
 	return true;
 }

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

#1321642

Johannes Weiner wrote:
> On Thu, Jan 28, 2016 at 03:19:08PM -0800, David Rientjes wrote:
> > On Thu, 28 Jan 2016, Johannes Weiner wrote:
> >
> > > The check has to happen while holding the OOM lock, otherwise we'll
> > > end up killing much more than necessary when there are many racing
> > > allocations.
> > >
> >
> > Right, we need to try with ALLOC_WMARK_HIGH after oom_lock has been
> > acquired.
> >
> > The situation is still somewhat fragile, however, but I think it's
> > tangential to this patch series.  If the ALLOC_WMARK_HIGH allocation fails
> > because an oom victim hasn't freed its memory yet, and then the TIF_MEMDIE
> > thread isn't visible during the oom killer's tasklist scan because it has
> > exited, we still end up killing more than we should.  The likelihood of
> > this happening grows with the length of the tasklist.
> >
> > Perhaps we should try testing watermarks after a victim has been selected
> > and immediately before killing?  (Aside: we actually carry an internal
> > patch to test mem_cgroup_margin() in the memcg oom path after selecting a
> > victim because we have been hit with this before in the memcg path.)

Yes. Moving final testing to after selecting an OOM victim can reduce the
possibility of killing more OOM victims than we need. But unfortunately, it is
likely that memory becomes available (i.e. get_page_from_freelist() succeeds)
during dump_header() is printing OOM messages using printk(), for printk() is
a slow operation compared to selecting a victim. This happens very much later
counted from the moment the victim cleared TIF_MEMDIE.

We can avoid killing more OOM victims than we need if we move final testing to
after printing OOM messages, but we can't avoid printing OOM messages when we
don't kill a victim. Maybe this is not a problem if we do

  pr_err("But did not kill any process ...")

instead of

  do_send_sig_info(SIGKILL);
  mark_oom_victim();
  pr_err("Killed process %d (%s) ...")

when final testing succeeded.

> >
> > I would think that retrying with ALLOC_WMARK_HIGH would be enough memory
> > to deem that we aren't going to immediately reenter an oom condition so
> > the deferred killing is a waste of time.
> >
> > The downside is how sloppy this would be because it's blurring the line
> > between oom killer and page allocator.  We'd need the oom killer to return
> > the selected victim to the page allocator, try the allocation, and then
> > call oom_kill_process() if necessary.

I assumed that Michal wants to preserve the boundary between the OOM killer
and the page allocator. Therefore, I proposed a patch
( http://lkml.kernel.org/r/201512291559.HGA46749.VFOFSOHLMtFJQO@I-love.SAKURA.ne.jp )
which tries to manage it without returning a victim and without depending on
TIF_MEMDIE or oom_victims.

>
> https://lkml.org/lkml/2015/3/25/40
>
> We could have out_of_memory() wait until the number of outstanding OOM
> victims drops to 0. Then __alloc_pages_may_oom() doesn't relinquish
> the lock until its kill has been finalized:
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 914451a..4dc5b9d 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -892,7 +892,9 @@ bool out_of_memory(struct oom_control *oc)
>  		 * Give the killed process a good chance to exit before trying
>  		 * to allocate memory again.
>  		 */
> -		schedule_timeout_killable(1);
> +		if (!test_thread_flag(TIF_MEMDIE))
> +			wait_event_timeout(oom_victims_wait,
> +					   !atomic_read(&oom_victims), HZ);
>  	}
>  	return true;
>  }
>

oom_victims became 0 does not mean that memory became available (i.e.
get_page_from_freelist() will succeed). I think this patch wants some
effort for trying to reduce possibility of killing more OOM victims
than we need.

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

#1321837 — Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

On Thu 28-01-16 18:51:10, Johannes Weiner wrote:
> On Thu, Jan 28, 2016 at 03:19:08PM -0800, David Rientjes wrote:
> > On Thu, 28 Jan 2016, Johannes Weiner wrote:
> > 
> > > The check has to happen while holding the OOM lock, otherwise we'll
> > > end up killing much more than necessary when there are many racing
> > > allocations.
> > > 
> > 
> > Right, we need to try with ALLOC_WMARK_HIGH after oom_lock has been 
> > acquired.
> > 
> > The situation is still somewhat fragile, however, but I think it's 
> > tangential to this patch series.  If the ALLOC_WMARK_HIGH allocation fails 
> > because an oom victim hasn't freed its memory yet, and then the TIF_MEMDIE 
> > thread isn't visible during the oom killer's tasklist scan because it has 
> > exited, we still end up killing more than we should.  The likelihood of 
> > this happening grows with the length of the tasklist.
> > 
> > Perhaps we should try testing watermarks after a victim has been selected 
> > and immediately before killing?  (Aside: we actually carry an internal 
> > patch to test mem_cgroup_margin() in the memcg oom path after selecting a 
> > victim because we have been hit with this before in the memcg path.)
> > 
> > I would think that retrying with ALLOC_WMARK_HIGH would be enough memory 
> > to deem that we aren't going to immediately reenter an oom condition so 
> > the deferred killing is a waste of time.
> > 
> > The downside is how sloppy this would be because it's blurring the line 
> > between oom killer and page allocator.  We'd need the oom killer to return 
> > the selected victim to the page allocator, try the allocation, and then 
> > call oom_kill_process() if necessary.
> 
> https://lkml.org/lkml/2015/3/25/40
> 
> We could have out_of_memory() wait until the number of outstanding OOM
> victims drops to 0. Then __alloc_pages_may_oom() doesn't relinquish
> the lock until its kill has been finalized:
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 914451a..4dc5b9d 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -892,7 +892,9 @@ bool out_of_memory(struct oom_control *oc)
>  		 * Give the killed process a good chance to exit before trying
>  		 * to allocate memory again.
>  		 */
> -		schedule_timeout_killable(1);
> +		if (!test_thread_flag(TIF_MEMDIE))
> +			wait_event_timeout(oom_victims_wait,
> +					   !atomic_read(&oom_victims), HZ);
>  	}
>  	return true;
>  }

Yes this makes sense to me
-- 
Michal Hocko
SUSE Labs

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

#1322345

Michal Hocko wrote:
> > https://lkml.org/lkml/2015/3/25/40
> > 
> > We could have out_of_memory() wait until the number of outstanding OOM
> > victims drops to 0. Then __alloc_pages_may_oom() doesn't relinquish
> > the lock until its kill has been finalized:
> > 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 914451a..4dc5b9d 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -892,7 +892,9 @@ bool out_of_memory(struct oom_control *oc)
> >  		 * Give the killed process a good chance to exit before trying
> >  		 * to allocate memory again.
> >  		 */
> > -		schedule_timeout_killable(1);
> > +		if (!test_thread_flag(TIF_MEMDIE))
> > +			wait_event_timeout(oom_victims_wait,
> > +					   !atomic_read(&oom_victims), HZ);
> >  	}
> >  	return true;
> >  }
> 
> Yes this makes sense to me

I think schedule_timeout_killable(1) was used for handling cases
where current thread did not get TIF_MEMDIE but got SIGKILL due to
sharing the victim's memory. If current thread is blocking TIF_MEMDIE
thread, this can become a needless delay.

Also, I don't know whether using wait_event_*() helps handling a
problem that schedule_timeout_killable(1) can sleep for many minutes
with oom_lock held when there are a lot of tasks. Detail is explained
in my proposed patch.

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

#1321833 — Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

On Thu 28-01-16 15:19:08, David Rientjes wrote:
> On Thu, 28 Jan 2016, Johannes Weiner wrote:
> 
> > The check has to happen while holding the OOM lock, otherwise we'll
> > end up killing much more than necessary when there are many racing
> > allocations.
> > 
> 
> Right, we need to try with ALLOC_WMARK_HIGH after oom_lock has been 
> acquired.
> 
> The situation is still somewhat fragile, however, but I think it's 
> tangential to this patch series.  If the ALLOC_WMARK_HIGH allocation fails 
> because an oom victim hasn't freed its memory yet, and then the TIF_MEMDIE 
> thread isn't visible during the oom killer's tasklist scan because it has 
> exited, we still end up killing more than we should.  The likelihood of 
> this happening grows with the length of the tasklist.

Yes exactly the point I made in the original thread which brought the
question about ALLOC_WMARK_HIGH originally. The race window after the
last attempt is much larger than between the last wmark check and the
attempt.

> Perhaps we should try testing watermarks after a victim has been selected 
> and immediately before killing?  (Aside: we actually carry an internal 
> patch to test mem_cgroup_margin() in the memcg oom path after selecting a 
> victim because we have been hit with this before in the memcg path.)
> 
> I would think that retrying with ALLOC_WMARK_HIGH would be enough memory 
> to deem that we aren't going to immediately reenter an oom condition so 
> the deferred killing is a waste of time.
> 
> The downside is how sloppy this would be because it's blurring the line 
> between oom killer and page allocator.  We'd need the oom killer to return 
> the selected victim to the page allocator, try the allocation, and then 
> call oom_kill_process() if necessary.

Yes the layer violation is definitely not nice.

-- 
Michal Hocko
SUSE Labs

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

#1321830 — Re: [PATCH 4/3] mm, oom: drop the last allocation attempt before out_of_memory

On Thu 28-01-16 16:36:34, Johannes Weiner wrote:
> On Thu, Jan 28, 2016 at 09:40:03PM +0100, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > __alloc_pages_may_oom has been doing get_page_from_freelist with
> > ALLOC_WMARK_HIGH target before going out_of_memory and invoking the oom
> > killer. This has two reasons as explained by Andrea:
> > "
> > : the reason for the high wmark is to reduce the likelihood of livelocks
> > : and be sure to invoke the OOM killer, if we're still under pressure
> > : and reclaim just failed. The high wmark is used to be sure the failure
> > : of reclaim isn't going to be ignored. If using the min wmark like
> > : you propose there's risk of livelock or anyway of delayed OOM killer
> > : invocation.
> > :
> > : The reason for doing one last wmark check (regardless of the wmark
> > : used) before invoking the oom killer, was just to be sure another OOM
> > : killer invocation hasn't already freed a ton of memory while we were
> > : stuck in reclaim. A lot of free memory generated by the OOM killer,
> > : won't make a parallel reclaim more likely to succeed, it just creates
> > : free memory, but reclaim only succeeds when it finds "freeable" memory
> > : and it makes progress in converting it to free memory. So for the
> > : purpose of this last check, the high wmark would work fine as lots of
> > : free memory would have been generated in such case.
> > "
> > 
> > This is no longer a concern after "mm, oom: rework oom detection"
> > because should_reclaim_retry performs the water mark check right before
> > __alloc_pages_may_oom is invoked. Remove the last moment allocation
> > request as it just makes the code more confusing and doesn't really
> > serve any purpose because a success is basically impossible otherwise
> > should_reclaim_retry would force the reclaim to retry. So this is
> > merely a code cleanup rather than a functional change.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> The check has to happen while holding the OOM lock, otherwise we'll
> end up killing much more than necessary when there are many racing
> allocations.

My testing shows that this doesn't trigger even during oom flood
testing. So I am not really convinced it does anything useful.

> Please drop this patch.

Sure I do not insist...
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web