On Tue, 10 Dec 2013 15:20:17 -0800 (PST) David Rientjes <rientjes@xxxxxxxxxx> 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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>