Message-ID: <20170607015909.GA6596@WeideMBP.lan>
Date:   Wed, 7 Jun 2017 10:10:36 +0800
From:   Wei Yang <richard.weiyang@...il.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     Wei Yang <richard.weiyang@...il.com>, 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 Tue, Jun 06, 2017 at 02:03:15PM +0200, Michal Hocko wrote:
>On Tue 06-06-17 11:04:01, Wei Yang wrote:
>> On Mon, Jun 05, 2017 at 08:43:43AM +0200, Michal Hocko wrote:
>> >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;
>> 
>> Thanks for your explanation.
>> 
>> So same "semantic" doesn't mean same "behavior".
>> 1. costly allocations will pick up the shut cut
>
>yes and there are no such allocations yet (based on my previous
>cleanups)
>
>> 2. !costly allocations will try something more but finally fail without
>> invoking OOM.
>
>no, the behavior will not change for those.
> 

Hmm... Let me be more specific. With two factors, costly or not, flag set or
not, we have four combinations. Here it is classified into two categories.

1. __GFP_RETRY_MAYFAIL not set

Brief description on behavior:
    costly: pick up the shortcut, so no OOM
    !costly: no shortcut and will OOM I think

Impact from this patch set:
    No.
    
My personal understanding:
    The allocation without __GFP_RETRY_MAYFAIL is not effected by this patch
    set.  Since !costly allocation will trigger OOM, this is the reason why
    "small allocations never fail _practically_", as mentioned in
    https://lwn.net/Articles/723317/.


3. __GFP_RETRY_MAYFAIL set

Brief description on behavior:
    costly/!costly: no shortcut here and no OOM invoked

Impact from this patch set:
    For those allocations with __GFP_RETRY_MAYFAIL, OOM is not invoked for
    both.

My personal understanding:
    This is the semantic you are willing to introduce in this patch set. By
    cutting off the OOM invoke when __GFP_RETRY_MAYFAIL is set, you makes this
    a middle situation between NOFAIL and NORETRY.
    
    page_alloc will try some luck to get some free pages without disturb other
    part of the system. By doing so, the never fail allocation for !costly
    pages will be "fixed". If I understand correctly, you are willing to make
    this the default behavior in the future?

>> Hope this time I catch your point.
>> 
>> BTW, did_some_progress mostly means the OOM works to me. Are there some other
>> important situations when did_some_progress is set to 1?
>
>Yes e.g. for GFP_NOFS when we cannot really invoke the OOM killer yet we
>cannot fail the allocation.


Thanks, currently I have a clear concept on this, while I will remember this :)

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists