lkml.org 
[lkml]   [2017]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/2] mm, oom: do not rely on TIF_MEMDIE for memory reserves access
    On Wed 02-08-17 00:30:33, Tetsuo Handa wrote:
    > Michal Hocko wrote:
    > > CONFIG_MMU=n doesn't have oom reaper so let's stick to the original
    > > ALLOC_NO_WATERMARKS approach but be careful because they still might
    > > deplete all the memory reserves so keep the semantic as close to the
    > > original implementation as possible and give them access to memory
    > > reserves only up to exit_mm (when tsk->mm is cleared) rather than while
    > > tsk_is_oom_victim which is until signal struct is gone.
    >
    > Currently memory allocations from __mmput() can use memory reserves but
    > this patch changes __mmput() not to use memory reserves. You say "keep
    > the semantic as close to the original implementation as possible" but
    > this change is not guaranteed to be safe.

    Yeah it cannot. That's why I've said as close as possible rather than
    equivalent. On the other hand I am wondering whether you have anything
    specific in mind or this is just a formalistic nitpicking^Wremark.

    > > @@ -2943,10 +2943,19 @@ bool __zone_watermark_ok(struct zone *z, unsigned int order, unsigned long mark,
    > > * the high-atomic reserves. This will over-estimate the size of the
    > > * atomic reserve but it avoids a search.
    > > */
    > > - if (likely(!alloc_harder))
    > > + if (likely(!alloc_harder)) {
    > > free_pages -= z->nr_reserved_highatomic;
    > > - else
    > > - min -= min / 4;
    > > + } else {
    > > + /*
    > > + * OOM victims can try even harder than normal ALLOC_HARDER
    > > + * users
    > > + */
    > > + if (alloc_flags & ALLOC_OOM)
    >
    > ALLOC_OOM is ALLOC_NO_WATERMARKS if CONFIG_MMU=n.
    > I wonder this test makes sense for ALLOC_NO_WATERMARKS.

    Yeah, it would be pointless because get_page_from_freelist will then
    ignore the result of the watermark check for ALLOC_NO_WATERMARKS. It is
    not harmfull though. I didn't find much better way without making the
    code harder to read. Do you have any suggestion?

    > > + min -= min / 2;
    > > + else
    > > + min -= min / 4;
    > > + }
    > > +
    > >
    > > #ifdef CONFIG_CMA
    > > /* If allocation can't use CMA areas don't use free CMA pages */
    > > @@ -3603,6 +3612,22 @@ gfp_to_alloc_flags(gfp_t gfp_mask)
    > > return alloc_flags;
    > > }
    > >
    > > +static bool oom_reserves_allowed(struct task_struct *tsk)
    > > +{
    > > + if (!tsk_is_oom_victim(tsk))
    > > + return false;
    > > +
    > > + /*
    > > + * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
    > > + * depletion and shouldn't give access to memory reserves passed the
    > > + * exit_mm
    > > + */
    > > + if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
    > > + return false;
    >
    > Branching based on CONFIG_MMU is ugly. I suggest timeout based next OOM
    > victim selection if CONFIG_MMU=n.

    I suggest we do not argue about nommu without actually optimizing for or
    fixing nommu which we are not here. I am even not sure memory reserves
    can ever be depleted for that config.

    Anyway I will go with the following instead
    diff --git a/mm/page_alloc.c b/mm/page_alloc.c
    index 5e5911f40014..3510e06b3bf3 100644
    --- a/mm/page_alloc.c
    +++ b/mm/page_alloc.c
    @@ -3618,11 +3618,10 @@ static bool oom_reserves_allowed(struct task_struct *tsk)
    return false;

    /*
    - * !MMU doesn't have oom reaper so we shouldn't risk the memory reserves
    - * depletion and shouldn't give access to memory reserves passed the
    - * exit_mm
    + * !MMU doesn't have oom reaper so give access to memory reserves
    + * only to the thread with TIF_MEMDIE set
    */
    - if (!IS_ENABLED(CONFIG_MMU) && !tsk->mm)
    + if (!IS_ENABLED(CONFIG_MMU) && !test_thread_flag(TIF_MEMDIE))
    return false;

    return true;
    This should preserve the original semantic. Is that acceptable for you?

    > > @@ -3875,15 +3901,24 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
    > > if (gfp_mask & __GFP_KSWAPD_RECLAIM)
    > > wake_all_kswapds(order, ac);
    > >
    > > - if (gfp_pfmemalloc_allowed(gfp_mask))
    > > - alloc_flags = ALLOC_NO_WATERMARKS;
    > > + /*
    > > + * Distinguish requests which really need access to whole memory
    > > + * reserves from oom victims which can live with their own reserve
    > > + */
    > > + reserves = gfp_pfmemalloc_allowed(gfp_mask);
    > > + if (reserves) {
    > > + if (tsk_is_oom_victim(current))
    > > + alloc_flags = ALLOC_OOM;
    >
    > If reserves == true due to reasons other than tsk_is_oom_victim(current) == true
    > (e.g. __GFP_MEMALLOC), why dare to reduce it?

    Well the comment above tries to explain. I assume that the oom victim is
    special here. a) it is on the way to die and b) we know that something
    will be freeing memory on the background so I assume this is acceptable.

    > > + else
    > > + alloc_flags = ALLOC_NO_WATERMARKS;
    > > + }
    >
    > If CONFIG_MMU=n, doing this test is silly.
    >
    > if (tsk_is_oom_victim(current))
    > alloc_flags = ALLOC_NO_WATERMARKS;
    > else
    > alloc_flags = ALLOC_NO_WATERMARKS;

    I am pretty sure any compiler can see the outcome is the same so the
    check would be dropped in that case. I primarily wanted to prevent from
    an additional ifdefery. I am open to suggestions for a better layout
    though.

    > > @@ -3960,7 +3995,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
    > > goto got_pg;
    > >
    > > /* Avoid allocations with no watermarks from looping endlessly */
    > > - if (test_thread_flag(TIF_MEMDIE) &&
    > > + if (tsk_is_oom_victim(current) &&
    > > (alloc_flags == ALLOC_NO_WATERMARKS ||
    > > (gfp_mask & __GFP_NOMEMALLOC)))
    > > goto nopage;
    >
    > And you are silently changing to "!costly __GFP_DIRECT_RECLAIM allocations never fail
    > (even selected for OOM victims)" (i.e. updating the too small to fail memory allocation
    > rule) by doing alloc_flags == ALLOC_NO_WATERMARKS if CONFIG_MMU=y.

    Ups that is an oversight during the rebase.

    diff --git a/mm/page_alloc.c b/mm/page_alloc.c
    index 5e5911f40014..6593ff9de1d9 100644
    --- a/mm/page_alloc.c
    +++ b/mm/page_alloc.c
    @@ -3996,7 +3996,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,

    /* Avoid allocations with no watermarks from looping endlessly */
    if (tsk_is_oom_victim(current) &&
    - (alloc_flags == ALLOC_NO_WATERMARKS ||
    + (alloc_flags == ALLOC_OOM ||
    (gfp_mask & __GFP_NOMEMALLOC)))
    goto nopage;

    Does this look better?
    > Applying this change might disturb memory allocation behavior. I don't
    > like this patch.

    Do you see anything appart from nommu that would be an unfixable road
    block?
    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2017-08-01 18:53    [W:2.988 / U:1.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site