lkml.org 
[lkml]   [2013]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch] mm, page_alloc: make __GFP_NOFAIL really not fail
On Tue, 10 Dec 2013 15:20:17 -0800 (PST) David Rientjes <rientjes@google.com> wrote:

> On Mon, 9 Dec 2013, Andrew Morton wrote:
>
> > > __GFP_NOFAIL specifies that the page allocator cannot fail to return
> > > memory. Allocators that call it may not even check for NULL upon
> > > returning.
> > >
> > > It turns out GFP_NOWAIT | __GFP_NOFAIL or GFP_ATOMIC | __GFP_NOFAIL can
> > > actually return NULL. More interestingly, processes that are doing
> > > direct reclaim and have PF_MEMALLOC set may also return NULL for any
> > > __GFP_NOFAIL allocation.
> >
> > __GFP_NOFAIL is a nasty thing and making it pretend to work even better
> > is heading in the wrong direction, surely? It would be saner to just
> > disallow these even-sillier combinations. Can we fix up the current
> > callers then stick a WARN_ON() in there?
> >
>
> Heh, it's difficult to remove __GFP_NOFAIL when new users get added:
> 84235de394d9 ("fs: buffer: move allocation failure loop into the
> allocator") added a new user

That wasn't reeeeealy a new user - it was "convert an existing
open-coded retry-for-ever loop". Which is what __GFP_NOFAIL is for.

I don't think I've ever seen anyone actually fix one of these things
(by teaching the caller to handle ENOMEM), so it obviously isn't
working...

> and a bypass of memcg limits in oom
> conditions so __GFP_NOFAIL just essentially became
> __GFP_BYPASS_MEMCG_LIMIT_ON_OOM.
>
> We can probably ignore the PF_MEMALLOC behavior since it allows full
> access to memory reserves and the only time we would see a __GFP_NOFAIL
> allocation fail in such a context is if every zone's free memory was 0.
> We have bigger problems if memory reserves are completely depleted like
> that, so it's probably sufficient not to address it.
>
> I'd be concerned about new users of __GFP_NOFAIL that are added for
> GFP_NOWAIT or GFP_ATOMIC and never actually trigger such a warning because
> in testing they never trigger the slowpath, but the conditional is
> probably better placed outside of the fastpath:
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2536,8 +2536,15 @@ rebalance:
> }
>
> /* Atomic allocations - we can't balance anything */
> - if (!wait)
> + if (!wait) {
> + /*
> + * All existing users of the deprecated __GFP_NOFAIL are
> + * blockable, so warn of any new users that actually allow this
> + * type of allocation to fail.
> + */
> + WARN_ON_ONCE(gfp_mask & __GFP_NOFAIL);
> goto nopage;
> + }

Seems sensible.

> But perhaps the best way to do this in a preventative way is to add a
> warning to checkpatch.pl that actually warns about adding new users.

yup.


\
 
 \ /
  Last update: 2013-12-11 02:41    [W:0.103 / U:0.608 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site