lkml.org 
[lkml]   [2015]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 07/12] mm, page_alloc: Distinguish between being unable to sleep, unwilling to sleep and avoiding waking kswapd
    On Tue, Aug 25, 2015 at 05:37:59PM +0200, Vlastimil Babka wrote:
    > On 08/24/2015 02:09 PM, Mel Gorman wrote:
    > >__GFP_WAIT has been used to identify atomic context in callers that hold
    > >spinlocks or are in interrupts. They are expected to be high priority and
    > >have access one of two watermarks lower than "min" which can be referred
    > >to as the "atomic reserve". __GFP_HIGH users get access to the first lower
    > >watermark and can be called the "high priority reserve".
    > >
    > >Over time, callers had a requirement to not block when fallback options
    > >were available. Some have abused __GFP_WAIT leading to a situation where
    > >an optimisitic allocation with a fallback option can access atomic reserves.
    > >
    > >This patch uses __GFP_ATOMIC to identify callers that are truely atomic,
    > >cannot sleep and have no alternative. High priority users continue to use
    > >__GFP_HIGH. __GFP_DIRECT_RECLAIM identifies callers that can sleep and are
    > >willing to enter direct reclaim. __GFP_KSWAPD_RECLAIM to identify callers
    > >that want to wake kswapd for background reclaim. __GFP_WAIT is redefined
    > >as a caller that is willing to enter direct reclaim and wake kswapd for
    > >background reclaim.
    > >
    > >This patch then converts a number of sites
    > >
    > >o __GFP_ATOMIC is used by callers that are high priority and have memory
    > > pools for those requests. GFP_ATOMIC uses this flag.
    > >
    > >o Callers that have a limited mempool to guarantee forward progress use
    > > __GFP_DIRECT_RECLAIM. bio allocations fall into this category where
    >
    > ^ __GFP_KSWAPD_RECLAIM ? (missed it previously)
    >

    I updated the changelog to make this clearer.

    > > kswapd will still be woken but atomic reserves are not used as there
    > > is a one-entry mempool to guarantee progress.
    > >
    > >o Callers that are checking if they are non-blocking should use the
    > > helper gfpflags_allow_blocking() where possible. This is because
    > > checking for __GFP_WAIT as was done historically now can trigger false
    > > positives. Some exceptions like dm-crypt.c exist where the code intent
    > > is clearer if __GFP_DIRECT_RECLAIM is used instead of the helper due to
    > > flag manipulations.
    > >
    > >o Callers that built their own GFP flags instead of starting with GFP_KERNEL
    > > and friends now also need to specify __GFP_KSWAPD_RECLAIM.
    > >
    > >The first key hazard to watch out for is callers that removed __GFP_WAIT
    > >and was depending on access to atomic reserves for inconspicuous reasons.
    > >In some cases it may be appropriate for them to use __GFP_HIGH.
    > >
    > >The second key hazard is callers that assembled their own combination of
    > >GFP flags instead of starting with something like GFP_KERNEL. They may
    > >now wish to specify __GFP_KSWAPD_RECLAIM. It's almost certainly harmless
    > >if it's missed in most cases as other activity will wake kswapd.
    > >
    > >Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
    >
    > Thanks for the effort!
    >
    > Acked-by: Vlastimil Babka <vbabka@suse.cz>
    >
    > Just last few bits:
    >
    > >@@ -2158,7 +2158,7 @@ static bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
    > > return false;
    > > if (fail_page_alloc.ignore_gfp_highmem && (gfp_mask & __GFP_HIGHMEM))
    > > return false;
    > >- if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & __GFP_WAIT))
    > >+ if (fail_page_alloc.ignore_gfp_wait && (gfp_mask & (__GFP_ATOMIC|__GFP_DIRECT_RECLAIM)))
    > > return false;
    > >
    > > return should_fail(&fail_page_alloc.attr, 1 << order);
    >
    > IIUC ignore_gfp_wait tells it to assume that reclaimers will eventually
    > succeed (for some reason?), so they shouldn't fail. Probably to focus the
    > testing on atomic allocations. But your change makes atomic allocation never
    > fail, so that goes against the knob IMHO?
    >

    Fair point, I'll remove the __GFP_ATOMIC check. I felt this was a sensible
    but then again deliberately failing allocations makes my brain twitch a
    bit. In retrospect, someone who cared should add a ignore_gfp_atomic knob.

    > >@@ -2660,7 +2660,7 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
    > > if (test_thread_flag(TIF_MEMDIE) ||
    > > (current->flags & (PF_MEMALLOC | PF_EXITING)))
    > > filter &= ~SHOW_MEM_FILTER_NODES;
    > >- if (in_interrupt() || !(gfp_mask & __GFP_WAIT))
    > >+ if (in_interrupt() || !(gfp_mask & __GFP_WAIT) || (gfp_mask & __GFP_ATOMIC))
    > > filter &= ~SHOW_MEM_FILTER_NODES;
    > >
    > > if (fmt) {
    >
    > This caught me previously and I convinced myself that it's OK, but now I'm
    > not anymore. IIUC this is to not filter nodes by mems_allowed during
    > printing, if the allocation itself wasn't limited? In that case it should
    > probably only look at __GFP_ATOMIC after this patch? As that's the only
    > thing that determines ALLOC_CPUSET.
    > I don't know where in_interrupt() comes from, but it was probably considered
    > in the past, as can be seen in zlc_setup()?
    >

    I assumed the in_interrupt() thing was simply because cpusets were the
    primary means of limiting allocations of interest to the author at the
    time.

    I guess now that I think about it more that a more sensible check would
    be against __GFP_DIRECT_RECLAIM because that covers the interesting
    cases.

    --
    Mel Gorman
    SUSE Labs


    \
     
     \ /
      Last update: 2015-08-26 17:01    [W:3.413 / U:0.532 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site