Re: [RFC PATCH 2/4] mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY_MAYFAIL with more useful semantic

[Wei Yang]

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@xxxxxxxx>
>[...]
>> >@@ -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
2. !costly allocations will try something more but finally fail without
invoking OOM.

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?

>-- 
>Michal Hocko
>SUSE Labs

-- 
Wei Yang
Help you, Help me

Attachment: signature.asc
Description: PGP signature