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.
> > + 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.
>
> > + if (compact_result != COMPACT_SKIPPED &&
> > + compact_result != COMPACT_NOT_SUITABLE_ZONE)
>
> It's undesirable to use COMPACT_NOT_SUITABLE_ZONE here. It is just for
> detailed tracepoint output.
Well this is a compaction code so I considered it acceptable. If you
consider it a big deal I can extract a wrapper and hide this detail.
[...]
> > @@ -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.
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>