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    [W:0.026 / U:180.376 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site