linux-kernel - Re: [PATCH v2 -mm 3/3] mm: only defer compaction for failed order and higher

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120125162112.GF3901@csn.ul.ie>
Date:	Wed, 25 Jan 2012 16:21:13 +0000
From:	Mel Gorman <mel@....ul.ie>
To:	Rik van Riel <riel@...hat.com>
Cc:	linux-mm@...ck.org, lkml <linux-kernel@...r.kernel.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Minchan Kim <minchan.kim@...il.com>,
	KOSAKI Motohiro <kosaki.motohiro@...il.com>
Subject: Re: [PATCH v2 -mm 3/3] mm: only defer compaction for failed order
 and higher

On Wed, Jan 25, 2012 at 10:55:05AM -0500, Rik van Riel wrote:
> On 01/25/2012 10:41 AM, Mel Gorman wrote:
> 
> >>--- a/mm/compaction.c
> >>+++ b/mm/compaction.c
> >>@@ -673,9 +673,18 @@ static int __compact_pgdat(pg_data_t *pgdat, struct compact_control *cc)
> >>  		INIT_LIST_HEAD(&cc->freepages);
> >>  		INIT_LIST_HEAD(&cc->migratepages);
> >>
> >>-		if (cc->order<  0 || !compaction_deferred(zone))
> >>+		if (cc->order<  0 || !compaction_deferred(zone, cc->order))
> >>  			compact_zone(zone, cc);
> >>
> >>+		if (cc->order>  0) {
> >>+			int ok = zone_watermark_ok(zone, cc->order,
> >>+						low_wmark_pages(zone), 0, 0);
> >>+			if (ok&&  cc->order>  zone->compact_order_failed)
> >>+				zone->compact_order_failed = cc->order + 1;
> >>+			else if (!ok&&  cc->sync)
> >>+				defer_compaction(zone, cc->order);
> >>+		}
> >>+
> >
> >That needs a comment. I think what you're trying to do is reset
> >compat_order_failed once compaction is successful.
> >
> >The "!ok&&  cc->sync" check may be broken. __compact_pgdat() is
> >called from kswapd, not direct compaction so cc->sync will not be true.
> 
> The problem with doing that is that we would be deferring
> synchronous compaction (by allocators), just because
> asynchronous compaction from kswapd failed...
> 

I should have been clear. I was not suggesting that we defer compaction
here for !cc->sync. I was pointing out that the code as-is is dead.

> That is the reason the code is like it is above.  And
> indeed, it will not defer compaction from this code path
> right now.
> 

Ok, that was my understanding, I just wanted to be sure I understood
your intentions.

> Then again, neither does async compaction from page
> allocators defer compaction - only sync compaction does.
> 

Yep, this is on purpose.

> If it turns out we need a separate compaction deferral
> for async compaction, we can always introduce that later,
> and this code will be ready for it.
> 

Ok, I can accept that.

> If you prefer, I can replace the whole "else if" bit with
> a big fat comment explaining why we cannot currently
> defer compaction from this point.
> 

Explaining that it is dead code for kswapd would also do.

If you do replace the code, add a WARN_ON(cc->sync) in case the
assumption changes in the future so defer_compaction() gets thought
about properly.

-- 
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/