lkml.org 
[lkml]   [2008]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 02/30] mm: gfp_to_alloc_flags()
    From
    Date
    On Tue, 2008-08-12 at 15:01 +1000, Neil Brown wrote:
    > On Thursday July 24, a.p.zijlstra@chello.nl wrote:
    > > Factor out the gfp to alloc_flags mapping so it can be used in other places.
    > >
    > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
    > > ---
    > > mm/internal.h | 10 +++++
    > > mm/page_alloc.c | 95 +++++++++++++++++++++++++++++++-------------------------
    > > 2 files changed, 64 insertions(+), 41 deletions(-)
    >
    > This patch all looks "obviously correct" and a nice factorisation of
    > code, except the last little bit:
    >
    > > @@ -1618,6 +1627,10 @@ nofail_alloc:
    > > if (!wait)
    > > goto nopage;
    > >
    > > + /* Avoid recursion of direct reclaim */
    > > + if (p->flags & PF_MEMALLOC)
    > > + goto nopage;
    > > +
    > > cond_resched();
    > >
    > > /* We now go into synchronous reclaim */
    > >
    > > --
    >
    > I don't remember seeing it before (though my memory is imperfect) and
    > it doesn't seem to fit with the rest of the patch (except spatially).
    >
    > There is a test above for PF_MEMALLOC which will result in a "goto"
    > somewhere else unless "in_interrupt()".
    > There is immediately above a test for "!wait".
    > So the only way this test can fire is when in_interrupt and wait.
    > But if that happens, then the
    > might_sleep_if(wait)
    > at the top should have thrown a warning... It really shouldn't happen.
    >
    > So it looks like it is useless code: there is already protection
    > against recursion in this case.
    >
    > Did I miss something?
    > If I did, maybe more text in the changelog entry (or the comment)
    > would help.

    Ok, so the old code did:

    if (((p->flags & PF_MEMALLOC) || ...) && !in_interrupt) {
    ....
    goto nopage;
    }

    which avoid anything that has PF_MEMALLOC set from entering into direct
    reclaim, right?

    Now, the new code reads:

    if (alloc_flags & ALLOC_NO_WATERMARK) {
    }

    Which might be false, even though we have PF_MEMALLOC set -
    __GFP_NOMEMALLOC comes to mind.

    So we have to stop that recursion from happening.

    so we add:

    if (p->flags & PF_MEMALLOC)
    goto nopage;

    Now, if it were done before the !wait check, we'd have to consider
    atomic contexts, but as those are - as you rightly pointed out - handled
    by the !wait case, we can plainly do this check.





    \
     
     \ /
      Last update: 2008-08-12 09:35    [W:0.028 / U:0.184 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site