linux-kernel - Re: [PATCH 7/8] mm: compaction: Use the LRU to get a hint on where compaction should start

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]

Date:	Thu, 18 Nov 2010 19:46:59 +0100
From:	Andrea Arcangeli <aarcange@...hat.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Johannes Weiner <hannes@...xchg.org>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/8] mm: compaction: Use the LRU to get a hint on where
 compaction should start

On Wed, Nov 17, 2010 at 04:22:48PM +0000, Mel Gorman wrote:
> +	if (!cc->migrate_pfn)
> +		cc->migrate_pfn = zone->zone_start_pfn;

wouldn't it remove a branch if the caller always set migrate_pfn?

> +	if (!cc->free_pfn) {
> +		cc->free_pfn = zone->zone_start_pfn + zone->spanned_pages;
> +		cc->free_pfn &= ~(pageblock_nr_pages-1);
> +	}

Who sets free_pfn to zero? Previously this was always initialized.

> @@ -523,7 +539,23 @@ unsigned long reclaimcompact_zone_order(struct zone *zone,
>  	INIT_LIST_HEAD(&cc.freepages);
>  	INIT_LIST_HEAD(&cc.migratepages);
>  
> -	return compact_zone(zone, &cc);
> +	/* Get a hint on where to start compacting from the LRU */
> +	anon_page = lru_to_page(&zone->lru[LRU_BASE + LRU_INACTIVE_ANON].list);
> +	file_page = lru_to_page(&zone->lru[LRU_BASE + LRU_INACTIVE_FILE].list);
> +	cc.migrate_pfn = min(page_to_pfn(anon_page), page_to_pfn(file_page));
> +	cc.migrate_pfn = ALIGN(cc.migrate_pfn, pageblock_nr_pages);
> +	start_migrate_pfn = cc.migrate_pfn;
> +
> +	ret = compact_zone(zone, &cc);
> +
> +	/* Restart migration from the start of zone if the hint did not work */
> +	if (!zone_watermark_ok(zone, cc.order, low_wmark_pages(zone), 0, 0)) {
> +		cc.migrate_pfn = 0;
> +		cc.abort_migrate_pfn = start_migrate_pfn;
> +		ret = compact_zone(zone, &cc);
> +	}
> +

I doubt it works ok if the list is empty... Maybe it's safer to
validate the migrate_pfn against the zone pfn start/end before
setting it in the migrate_pfn.

Interesting this heuristic slowed down the benchmark, it should lead
to the exact opposite thanks to saving some cpu. So I guess maybe it's
not worth it. I see it increases the ratio of compaction of a tiny
bit, but if a tiny bit of better compaction comes at the expenses of
an increased runtime I don't like it and I'd drop it... It's not
making enough difference, further we could extend it to check the
"second" page in the list and so on... so we can just go blind. All it
matters is that we use a clock algorithm and I guess this screwes it
and this is why it leads to increased time.
--
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/