lkml.org 
[lkml]   [2017]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH 2/4] mm, tree wide: replace __GFP_REPEAT by __GFP_RETRY_MAYFAIL with more useful semantic
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@suse.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.

> 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.
--
Michal Hocko
SUSE Labs

\
 
 \ /
  Last update: 2017-06-12 00:46    [W:0.104 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site