Message-ID: <20111021100624.GB4029@csn.ul.ie>
Date:	Fri, 21 Oct 2011 12:06:24 +0200
From:	Mel Gorman <mel@....ul.ie>
To:	Michal Nazarewicz <mina86@...a86.com>
Cc:	Marek Szyprowski <m.szyprowski@...sung.com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-media@...r.kernel.org, linux-mm@...ck.org,
	linaro-mm-sig@...ts.linaro.org,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Russell King <linux@....linux.org.uk>,
	Andrew Morton <akpm@...ux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Ankita Garg <ankita@...ibm.com>,
	Daniel Walker <dwalker@...eaurora.org>,
	Arnd Bergmann <arnd@...db.de>,
	Jesse Barker <jesse.barker@...aro.org>,
	Jonathan Corbet <corbet@....net>,
	Shariq Hasnain <shariq.hasnain@...aro.org>,
	Chunsang Jeong <chunsang.jeong@...aro.org>,
	Dave Hansen <dave@...ux.vnet.ibm.com>
Subject: Re: [PATCH 2/9] mm: alloc_contig_freed_pages() added

On Tue, Oct 18, 2011 at 10:26:37AM -0700, Michal Nazarewicz wrote:
> On Tue, 18 Oct 2011 05:21:09 -0700, Mel Gorman <mel@....ul.ie> wrote:
> 
> >At this point, I'm going to apologise for not reviewing this a long long
> >time ago.
> >
> >On Thu, Oct 06, 2011 at 03:54:42PM +0200, Marek Szyprowski wrote:
> >>From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> >>
> >>This commit introduces alloc_contig_freed_pages() function
> >>which allocates (ie. removes from buddy system) free pages
> >>in range. Caller has to guarantee that all pages in range
> >>are in buddy system.
> >>
> >
> >Straight away, I'm wondering why you didn't use
> >
> >mm/compaction.c#isolate_freepages()
> >
> >It knows how to isolate pages within ranges. All its control information
> >is passed via struct compact_control() which I recognise may be awkward
> >for CMA but compaction.c know how to manage all the isolated pages and
> >pass them to migrate.c appropriately.
> 
> It is something to consider.  At first glance, I see that isolate_freepages
> seem to operate on pageblocks which is not desired for CMA.
> 

isolate_freepages_block operates on a range of pages that happens to be
hard-coded to be a pageblock because that was the requirements. It calculates
end_pfn and it is possible to make that a function parameter.

> >I haven't read all the patches yet but isolate_freepages() does break
> >everything up into order-0 pages. This may not be to your liking but it
> >would not be possible to change.
> 
> Splitting everything into order-0 pages is desired behaviour.
> 

Great.

> >>Along with this function, a free_contig_pages() function is
> >>provided which frees all (or a subset of) pages allocated
> >>with alloc_contig_free_pages().
> 
> >mm/compaction.c#release_freepages()
> 
> It sort of does the same thing but release_freepages() assumes that pages
> that are being freed are not-continuous and they need to be on the lru list.
> With free_contig_pages(), we can assume all pages are continuous.
> 

Ok, I jumped the gun here. release_freepages() may not be a perfect fit.
release_freepages() is also used when finishing compaction where as it
is a real free function that is required here.

> >You can do this in a more general fashion by checking the
> >zone boundaries and resolving the pfn->page every MAX_ORDER_NR_PAGES.
> >That will not be SPARSEMEM specific.
> 
> I've tried doing stuff that way but it ended up with much more code.
> 
> Dave suggested the above function to check if pointer arithmetic is valid.
> 
> Please see also <https://lkml.org/lkml/2011/9/21/220>.
> 

Ok, I'm still not fully convinced but I confess I'm not thinking about this
particular function too deeply because I am expecting the problem would
go away if compaction and CMA shared common code for freeing contiguous
regions via page migration.

> >> <SNIP>
> >>+		if (zone_pfn_same_memmap(pfn - count, pfn))
> >>+			page += count;
> >>+		else
> >>+			page = pfn_to_page(pfn);
> >>+	}
> >>+
> >>+	spin_unlock_irq(&zone->lock);
> >>+
> >>+	/* After this, pages in the range can be freed one be one */
> >>+	count = pfn - start;
> >>+	pfn = start;
> >>+	for (page = pfn_to_page(pfn); count; --count) {
> >>+		prep_new_page(page, 0, flag);
> >>+		++pfn;
> >>+		if (likely(zone_pfn_same_memmap(pfn - 1, pfn)))
> >>+			++page;
> >>+		else
> >>+			page = pfn_to_page(pfn);
> >>+	}
> >>+
> >
> >Here it looks like you have implemented something like split_free_page().
> 
> split_free_page() takes a single page, removes it from buddy system, and finally
> splits it. 

I'm referring to just this chunk.

split_free_page takes a page, checks the watermarks and performs similar
operations to prep_new_page(). There should be no need to introduce a
new similar function. split_free_page() does affect hte pageblock
migratetype and that is undesirable but that part could be taken out and
moved to compaction.c if necessary.

On the watermarks thing, CMA does not pay much attention to them. I have
a strong feeling that it is easy to deadlock a system by using CMA while
under memory pressure. Compaction had the same problem early in its
development FWIW. This is partially why I'd prefer to see CMA and
compaction sharing as much code as possible because compaction gets
continual testing.

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

Powered by blists - more mailing lists