lkml.org 
[lkml]   [2009]   [Jun]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: upcoming kerneloops.org item: get_page_from_freelist
On Tue, 30 Jun 2009, Mel Gorman wrote:

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 175a67a..5f4656e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -230,14 +230,21 @@ static struct task_struct *select_bad_process(unsigned long *ppoints,
> /*
> * This task already has access to memory reserves and is
> * being killed. Don't allow any other task access to the
> - * memory reserve.
> + * memory reserve unless the current process is the one
> + * selected for OOM-killing. If the current process has
> + * been OOM-killed and we are OOM again, another process
> + * needs to be considered for OOM-kill
> *
> * Note: this may have a chance of deadlock if it gets
> * blocked waiting for another task which itself is waiting
> * for memory. Is there a better alternative?
> */
> - if (test_tsk_thread_flag(p, TIF_MEMDIE))
> - return ERR_PTR(-1UL);
> + if (test_tsk_thread_flag(p, TIF_MEMDIE)) {
> + if (p == current)
> + continue;
> + else
> + return ERR_PTR(-1UL);
> + }
>
> /*
> * This is in the process of releasing memory so wait for it

This will panic the machine if current is the only user thread running or
eligible for oom kill (all others could have mm's with OOM_DISABLE set).
Currently, such tasks can exit or kthreads can free memory so that the oom
is recoverable.

The problem with this approach is that it increases the liklihood that
memory reserves will be totally depleted when several threads are
competing for them.

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5d714f8..5896469 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1539,6 +1539,14 @@ should_alloc_retry(gfp_t gfp_mask, unsigned int order,
> if (gfp_mask & __GFP_NORETRY)
> return 0;
>
> + /* Do not loop if OOM-killed unless __GFP_NOFAIL is specified */
> + if (test_thread_flag(TIF_MEMDIE)) {
> + if (gfp_mask & __GFP_NOFAIL)
> + WARN(1, "Potential infinite loop with __GFP_NOFAIL");
> + else
> + return 0;
> + }
> +

There's a second bug in the refactored page allocator: when the oom killer
is invoked and it happens to kill current, alloc_flags is never updated
because it loops back to `restart', which is past gfp_to_alloc_flags().

When that is fixed with the patch below, gfp_mask & __GFP_NOFAIL will
never be true here in your scenario, where the oom killer kills current,
because __alloc_pages_high_priority() will infinitely loop.

> /*
> * In this implementation, order <= PAGE_ALLOC_COSTLY_ORDER
> * means __GFP_NOFAIL, but that may not be true in other
>

This is needed for 2.6.31-rc2.


mm: update alloc_flags after oom killer has been called

It is possible for the oom killer to select current as the task to kill.
When this happens, alloc_flags needs to be updated accordingly to set
ALLOC_NO_WATERMARKS so the subsequent allocation attempt may use memory
reserves as the result of its thread having TIF_MEMDIE set if the
allocation is not __GFP_NOMEMALLOC.

Signed-off-by: David Rientjes <rientjes@google.com>
---
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1756,6 +1756,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,

wake_all_kswapd(order, zonelist, high_zoneidx);

+restart:
/*
* OK, we're below the kswapd watermark and have kicked background
* reclaim. Now things get more complex, so set up alloc_flags according
@@ -1763,7 +1764,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
*/
alloc_flags = gfp_to_alloc_flags(gfp_mask);

-restart:
/* This is the last chance, in general, before the goto nopage. */
page = get_page_from_freelist(gfp_mask, nodemask, order, zonelist,
high_zoneidx, alloc_flags & ~ALLOC_NO_WATERMARKS,

\
 
 \ /
  Last update: 2009-06-30 21:39    [from the cache]
©2003-2011 Jasper Spaans