From: Michal Hocko <mhocko@kernel.org>
To: Joonsoo Kim <js1304@gmail.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Andrew Morton <akpm@linux-foundation.org>,
Linus Torvalds <torvalds@linux-foundation.org>,
Johannes Weiner <hannes@cmpxchg.org>,
Mel Gorman <mgorman@suse.de>,
David Rientjes <rientjes@google.com>,
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
Hillf Danton <hillf.zj@alibaba-inc.com>,
Vlastimil Babka <vbabka@suse.cz>,
Linux Memory Management List <linux-mm@kvack.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders
Date: Wed, 4 May 2016 21:22:54 +0200 [thread overview]
Message-ID: <20160504192254.GD21490@dhcp22.suse.cz> (raw)
In-Reply-To: <CAAmzW4Ohnhrx1RkAFywwQyLW1b1NiHhB9AkvVCp8NVC9vyevtQ@mail.gmail.com>
On Thu 05-05-16 00:14:51, Joonsoo Kim wrote:
> 2016-05-04 18:04 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Wed 04-05-16 15:27:48, Joonsoo Kim wrote:
> >> On Wed, Apr 20, 2016 at 03:47:27PM -0400, Michal Hocko wrote:
> > [...]
> >> > +bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
> >> > + int alloc_flags)
> >> > +{
> >> > + struct zone *zone;
> >> > + struct zoneref *z;
> >> > +
> >> > + /*
> >> > + * Make sure at least one zone would pass __compaction_suitable if we continue
> >> > + * retrying the reclaim.
> >> > + */
> >> > + for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> >> > + ac->nodemask) {
> >> > + unsigned long available;
> >> > + enum compact_result compact_result;
> >> > +
> >> > + /*
> >> > + * Do not consider all the reclaimable memory because we do not
> >> > + * want to trash just for a single high order allocation which
> >> > + * is even not guaranteed to appear even if __compaction_suitable
> >> > + * is happy about the watermark check.
> >> > + */
> >> > + available = zone_reclaimable_pages(zone) / order;
> >>
> >> I can't understand why '/ order' is needed here. Think about specific
> >> example.
> >>
> >> zone_reclaimable_pages = 100 MB
> >> NR_FREE_PAGES = 20 MB
> >> watermark = 40 MB
> >> order = 10
> >>
> >> I think that compaction should run in this situation and your logic
> >> doesn't. We should be conservative when guessing not to do something
> >> prematurely.
> >
> > I do not mind changing this. But pushing really hard on reclaim for
> > order-10 pages doesn't sound like a good idea. So we should somehow
> > reduce the target. I am open for any better suggestions.
>
> If the situation is changed to order-2, it doesn't look good, either.
Why not? If we are not able to get over compaction_suitable watermark
check after we consider half of the reclaimable memory then we are really
close to oom. This will trigger only when the reclaimable LRUs are
really _tiny_. We are (very roughly) talking about:
low_wmark + 2<<order >= NR_FREE_PAGES + reclaimable/order - 1<<order
where low_wmark would be close to NR_FREE_PAGES so in the end we are asking
for order * 3<<order >= reclaimable and that sounds quite conservative
to me. Originally I wanted much more aggressive back off.
> I think that some reduction on zone_reclaimable_page() are needed since
> it's not possible to free all of them in certain case. But, reduction by order
> doesn't make any sense. if we need to consider order to guess probability of
> compaction, it should be considered in __compaction_suitable() instead of
> reduction from here.
I do agree that a more clever algorithm would be better and I also agree
that __compaction_suitable would be a better place for such a better
algorithm. I just wanted to have something simple first and more as a
safety net to stop endless retries (this has proven to work before I
found the real culprit compaction_ready patch). A more rigorous approach
would require a much deeper analysis what the actual compaction capacity
of the reclaimable memory really is. This is a quite hard problem and I
am not really convinced it is really needed.
> I think that following code that is used in should_reclaim_retry() would be
> good for here.
>
> available -= DIV_ROUND_UP(no_progress_loops * available, MAX_RECLAIM_RETRIES)
>
> Any thought?
I would prefer not to mix reclaim retry logic in here. Moreover it can
be argued that this is a kind of arbitrary as well because it has no
relevance to the compaction capacity of the reclaimable memory. If I
have to chose then I would rather go with simpler calculation than
something that is complex and we are even not sure it works any better.
> >> > + available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> >> > + compact_result = __compaction_suitable(zone, order, alloc_flags,
> >> > + ac->classzone_idx, available);
> >>
> >> It misses tracepoint in compaction_suitable().
> >
> > Why do you think the check would be useful. I have considered it more
> > confusing than halpful to I have intentionally not added it.
>
> What confusing do you have in mind?
> If we try to analyze OOM, we need to know why should_compact_retry()
> return false and and tracepoint here could be helpful.
Because then you can easily confuse compaction_suitable from the
compaction decisions and the allocation retries. This code patch
definitely deserves a specific trace point and I plan to prepare one
along with others in the allocation path.
[...]
> >> > @@ -3040,9 +3040,11 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
> >> > /*
> >> > * make sure the compaction wasn't deferred or didn't bail out early
> >> > * due to locks contention before we declare that we should give up.
> >> > + * But do not retry if the given zonelist is not suitable for
> >> > + * compaction.
> >> > */
> >> > if (compaction_withdrawn(compact_result))
> >> > - return true;
> >> > + return compaction_zonelist_suitable(ac, order, alloc_flags);
> >>
> >> I think that compaction_zonelist_suitable() should be checked first.
> >> If compaction_zonelist_suitable() returns false, it's useless to
> >> retry since it means that compaction cannot run if all reclaimable
> >> pages are reclaimed. Logic should be as following.
> >>
> >> if (!compaction_zonelist_suitable())
> >> return false;
> >>
> >> if (compaction_withdrawn())
> >> return true;
> >
> > That is certainly an option as well. The logic above is that I really
> > wanted to have a terminal condition when compaction can return
> > compaction_withdrawn for ever basically. Normally we are bound by a
> > number of successful reclaim rounds. Before we go an change there I
> > would like to see where it makes real change though.
>
> It would not make real change because !compaction_withdrawn() and
> !compaction_zonelist_suitable() case doesn't happen easily.
>
> But, change makes code more understandable so it's worth doing, IMO.
I dunno. I might be really biased here but I consider the current
ordering more appropriate for the reasons described above. Act as a
terminal condition for potentially endless compaction_withdrawn() rather
than a terminal condition on its own. Anyway I am not really sure this
is something crucial or do you consider this particular part really
important? I would prefer to not sneak last minute changes before the
upcoming merge windown just for readability which is even non-trivial.
--
Michal Hocko
SUSE Labs
next prev parent reply other threads:[~2016-05-04 19:22 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-20 19:47 [PATCH 0.14] oom detection rework v6 Michal Hocko
2016-04-20 19:47 ` [PATCH 01/14] vmscan: consider classzone_idx in compaction_ready Michal Hocko
2016-04-21 3:32 ` Hillf Danton
2016-05-04 13:56 ` Michal Hocko
2016-04-20 19:47 ` [PATCH 02/14] mm, compaction: change COMPACT_ constants into enum Michal Hocko
2016-04-20 19:47 ` [PATCH 03/14] mm, compaction: cover all compaction mode in compact_zone Michal Hocko
2016-04-20 19:47 ` [PATCH 04/14] mm, compaction: distinguish COMPACT_DEFERRED from COMPACT_SKIPPED Michal Hocko
2016-04-21 7:08 ` Hillf Danton
2016-04-20 19:47 ` [PATCH 05/14] mm, compaction: distinguish between full and partial COMPACT_COMPLETE Michal Hocko
2016-04-21 6:39 ` Hillf Danton
2016-04-20 19:47 ` [PATCH 06/14] mm, compaction: Update compaction_result ordering Michal Hocko
2016-04-21 6:45 ` Hillf Danton
2016-04-20 19:47 ` [PATCH 07/14] mm, compaction: Simplify __alloc_pages_direct_compact feedback interface Michal Hocko
2016-04-21 6:50 ` Hillf Danton
2016-04-20 19:47 ` [PATCH 08/14] mm, compaction: Abstract compaction feedback to helpers Michal Hocko
2016-04-21 6:57 ` Hillf Danton
2016-04-28 8:47 ` Vlastimil Babka
2016-04-20 19:47 ` [PATCH 09/14] mm: use compaction feedback for thp backoff conditions Michal Hocko
2016-04-21 7:05 ` Hillf Danton
2016-04-28 8:53 ` Vlastimil Babka
2016-04-28 12:35 ` Michal Hocko
2016-04-29 9:16 ` Vlastimil Babka
2016-04-29 9:28 ` Michal Hocko
2016-04-20 19:47 ` [PATCH 10/14] mm, oom: rework oom detection Michal Hocko
2016-04-20 19:47 ` [PATCH 11/14] mm: throttle on IO only when there are too many dirty and writeback pages Michal Hocko
2016-04-20 19:47 ` [PATCH 12/14] mm, oom: protect !costly allocations some more Michal Hocko
2016-04-21 8:03 ` Hillf Danton
2016-05-04 6:01 ` Joonsoo Kim
2016-05-04 6:31 ` Joonsoo Kim
2016-05-04 8:56 ` Michal Hocko
2016-05-04 14:57 ` Joonsoo Kim
2016-05-04 18:19 ` Michal Hocko
2016-05-04 8:53 ` Michal Hocko
2016-05-04 14:39 ` Joonsoo Kim
2016-05-04 18:20 ` Michal Hocko
2016-04-20 19:47 ` [PATCH 13/14] mm: consider compaction feedback also for costly allocation Michal Hocko
2016-04-21 8:13 ` Hillf Danton
2016-04-20 19:47 ` [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Michal Hocko
2016-04-21 8:24 ` Hillf Danton
2016-04-28 8:59 ` Vlastimil Babka
2016-04-28 12:39 ` Michal Hocko
2016-05-04 6:27 ` Joonsoo Kim
2016-05-04 9:04 ` Michal Hocko
2016-05-04 15:14 ` Joonsoo Kim
2016-05-04 19:22 ` Michal Hocko [this message]
2016-05-04 5:45 ` [PATCH 0.14] oom detection rework v6 Joonsoo Kim
2016-05-04 8:12 ` Vlastimil Babka
2016-05-04 8:32 ` Joonsoo Kim
2016-05-04 8:50 ` Michal Hocko
2016-05-04 8:47 ` Michal Hocko
2016-05-04 14:32 ` Joonsoo Kim
2016-05-04 18:16 ` Michal Hocko
2016-05-10 6:41 ` Joonsoo Kim
2016-05-10 7:09 ` Vlastimil Babka
2016-05-10 8:00 ` Joonsoo Kim
2016-05-10 9:44 ` Michal Hocko
2016-05-10 9:43 ` Michal Hocko
2016-05-12 2:23 ` Joonsoo Kim
2016-05-12 5:19 ` Joonsoo Kim
2016-05-12 10:59 ` Michal Hocko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160504192254.GD21490@dhcp22.suse.cz \
--to=mhocko@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=hannes@cmpxchg.org \
--cc=hillf.zj@alibaba-inc.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=js1304@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mgorman@suse.de \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=rientjes@google.com \
--cc=torvalds@linux-foundation.org \
--cc=vbabka@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).