Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders

[Michal Hocko]

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