linux-kernel - Re: [PATCH 07/11] Memory compaction core

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: <20100318114302.GM12388@csn.ul.ie>
Date:	Thu, 18 Mar 2010 11:43:03 +0000
From:	Mel Gorman <mel@....ul.ie>
To:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Christoph Lameter <cl@...ux-foundation.org>,
	Adam Litke <agl@...ibm.com>, Avi Kivity <avi@...hat.com>,
	David Rientjes <rientjes@...gle.com>,
	Rik van Riel <riel@...hat.com>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH 07/11] Memory compaction core

On Thu, Mar 18, 2010 at 11:35:46AM +0900, KOSAKI Motohiro wrote:
> > On Wed, Mar 17, 2010 at 07:31:53PM +0900, KOSAKI Motohiro wrote:
> > > nit
> > > 
> > > > +static int compact_zone(struct zone *zone, struct compact_control *cc)
> > > > +{
> > > > +	int ret = COMPACT_INCOMPLETE;
> > > > +
> > > > +	/* Setup to move all movable pages to the end of the zone */
> > > > +	cc->migrate_pfn = zone->zone_start_pfn;
> > > > +	cc->free_pfn = cc->migrate_pfn + zone->spanned_pages;
> > > > +	cc->free_pfn &= ~(pageblock_nr_pages-1);
> > > > +
> > > > +	for (; ret == COMPACT_INCOMPLETE; ret = compact_finished(zone, cc)) {
> > > > +		unsigned long nr_migrate, nr_remaining;
> > > > +		if (!isolate_migratepages(zone, cc))
> > > > +			continue;
> > > > +
> > > > +		nr_migrate = cc->nr_migratepages;
> > > > +		migrate_pages(&cc->migratepages, compaction_alloc,
> > > > +						(unsigned long)cc, 0);
> > > > +		update_nr_listpages(cc);
> > > > +		nr_remaining = cc->nr_migratepages;
> > > > +
> > > > +		count_vm_event(COMPACTBLOCKS);
> > > 
> > > V1 did compaction per pageblock. but current patch doesn't.
> > > so, Is COMPACTBLOCKS still good name?
> > 
> > It's not such a minor nit. I wondered about that myself but it's still a
> > block - just not a pageblock. Would COMPACTCLUSTER be a better name as it's
> > related to COMPACT_CLUSTER_MAX?
> 
> I've looked at this code again. honestly I'm a abit confusing even though both your
> suggestions seems reasonable.  
> 
> now COMPACTBLOCKS is tracking #-of-called-migrate_pages. but I can't imazine
> how to use it. can you please explain this ststics purpose? probably this is only useful
> when conbination other stats, and the name should be consist with such combination one.
> 

It is intended to count how many steps compaction took, the fewer the
better so minimally, the lower this number is the better. Specifically, the
"goodness" is related to the number of pages that were successfully allocated
due to compaction. Assuming the only high-order allocation was huge pages,
one possible calculation for "goodness" is;

hugepage_clusters = (1 << HUGE HUGETLB_PAGE_ORDER) / COMPACT_CLUSTER_MAX
goodness = (compactclusters / hugepage_clusters) / compactsuccess

The value of goodness is undefined if "compactsuccess" is 0.

Otherwise, the closer the "goodness" is to 1, the better. A value of 1
implies that compaction is selecting exactly the right blocks for migration
and the minimum number of pages are being moved around. The greater the value,
the more "useless" work compaction is doing.

If there are a mix of high-orders that are resulting in compaction, calculating
the goodness is a lot harder and compactcluster is just a rule of thumb as
to how much work compaction is doing.

Does that make sense?

> 
> > > > +		count_vm_events(COMPACTPAGES, nr_migrate - nr_remaining);
> > > > +		if (nr_remaining)
> > > > +			count_vm_events(COMPACTPAGEFAILED, nr_remaining);
> > > > +
> > > > +		/* Release LRU pages not migrated */
> > > > +		if (!list_empty(&cc->migratepages)) {
> > > > +			putback_lru_pages(&cc->migratepages);
> > > > +			cc->nr_migratepages = 0;
> > > > +		}
> > > > +
> > > > +		mod_zone_page_state(zone, NR_ISOLATED_ANON, -cc->nr_anon);
> > > > +		mod_zone_page_state(zone, NR_ISOLATED_FILE, -cc->nr_file);
> > > 
> > > I think you don't need decrease this vmstatistics here. migrate_pages() and
> > > putback_lru_pages() alredy does.
> > > 
> > 
> > Hmm, I do need to decrease the vmstats here but not by this much. The
> > pages migrated need to be accounted for but not the ones that failed. I
> > missed this because migration was always succeeding. Thanks. I'll get it
> > fixed for V5
> 
> thanks.
> 
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab
--
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/