linux-kernel - Re: [RFC PATCH 2/4] mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170605064343.GE9248@dhcp22.suse.cz>
Date:   Mon, 5 Jun 2017 08:43:43 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Wei Yang <richard.weiyang@...il.com>
Cc:     linux-mm@...ck.org, Vlastimil Babka <vbabka@...e.cz>,
        Johannes Weiner <hannes@...xchg.org>,
        Mel Gorman <mgorman@...e.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/4] mm, tree wide: replace __GFP_REPEAT by
 __GFP_RETRY_MAYFAIL with more useful semantic

On Sat 03-06-17 10:24:40, Wei Yang wrote:
> Hi, Michal
> 
> Just go through your patch.
> 
> I have one question and one suggestion as below.
> 
> One suggestion:
> 
> This patch does two things to me:
> 1. Replace __GFP_REPEAT with __GFP_RETRY_MAYFAIL
> 2. Adjust the logic in page_alloc to provide the middle semantic
> 
> My suggestion is to split these two task into two patches, so that readers
> could catch your fundamental logic change easily.

Well, the rename and the change is intentionally tight together. My
previous patches have removed all __GFP_REPEAT users for low order
requests which didn't have any implemented semantic. So as of now we
should only have those users which semantic will not change. I do not
add any new low order user in this patch so it in fact doesn't change
any existing semnatic.

> 
> On Tue, Mar 07, 2017 at 04:48:41PM +0100, Michal Hocko wrote:
> >From: Michal Hocko <mhocko@...e.com>
[...]
> >@@ -3776,9 +3784,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > 
> > 	/*
> > 	 * Do not retry costly high order allocations unless they are
> >-	 * __GFP_REPEAT
> >+	 * __GFP_RETRY_MAYFAIL
> > 	 */
> >-	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
> >+	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_RETRY_MAYFAIL))
> > 		goto nopage;
> 
> One question:
> 
> From your change log, it mentions will provide the same semantic for !costly
> allocations. While the logic here is the same as before.
> 
> For a !costly allocation with __GFP_REPEAT flag, the difference after this
> patch is no OOM will be invoked, while it will still continue in the loop.

Not really. There are two things. The above will shortcut retrying if
there is _no_ __GFP_RETRY_MAYFAIL. If the flags _is_ specified we will
back of in __alloc_pages_may_oom.
 
> Maybe I don't catch your point in this message:
> 
>   __GFP_REPEAT was designed to allow retry-but-eventually-fail semantic to
>   the page allocator. This has been true but only for allocations requests
>   larger than PAGE_ALLOC_COSTLY_ORDER. It has been always ignored for
>   smaller sizes. This is a bit unfortunate because there is no way to
>   express the same semantic for those requests and they are considered too
>   important to fail so they might end up looping in the page allocator for
>   ever, similarly to GFP_NOFAIL requests.
> 
> I thought you will provide the same semantic to !costly allocation, or I
> misunderstand?

yes and that is the case. __alloc_pages_may_oom will back off before OOM
killer is invoked and the allocator slow path will fail because
did_some_progress == 0;
-- 
Michal Hocko
SUSE Labs