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]

Date:	Thu, 18 Mar 2010 11:35:46 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	Mel Gorman <mel@....ul.ie>
Cc:	kosaki.motohiro@...fujitsu.com,
	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 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.


> > > +		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.


--
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/