RE: [PATCH 01/11] mm: page_alloc: set_migratetype_isolate: drain PCP prior to isolating

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On Thursday, January 05, 2012 4:40 PM Michał Nazarewicz wrote:

> On Thu, 29 Dec 2011 13:39:02 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > From: Michal Nazarewicz <mina86@mina86.com>
> >
> > When set_migratetype_isolate() sets pageblock's migrate type, it does
> > not change each page_private data.  This makes sense, as the function
> > has no way of knowing what kind of information page_private stores.
> >
> > Unfortunately, if a page is on PCP list, it's page_private indicates
> > its migrate type.  This means, that if a page on PCP list gets
> > isolated, a call to free_pcppages_bulk() will assume it has the old
> > migrate type rather than MIGRATE_ISOLATE.  This means, that a page
> > which should be isolated, will end up on a free list of it's old
> > migrate type.
> >
> > Coincidentally, at the very end, set_migratetype_isolate() calls
> > drain_all_pages() which leads to calling free_pcppages_bulk(), which
> > does the wrong thing.
> >
> > To avoid this situation, this commit moves the draining prior to
> > setting pageblock's migratetype and moving pages from old free list to
> > MIGRATETYPE_ISOLATE's free list.
> >
> > Because of spin locks this is a non-trivial change however as both
> > set_migratetype_isolate() and free_pcppages_bulk() grab zone->lock.
> > To solve this problem, this commit renames free_pcppages_bulk() to
> > __free_pcppages_bulk() and changes it so that it no longer grabs
> > zone->lock instead requiring caller to hold it.  This commit later
> > adds a __zone_drain_all_pages() function which works just like
> > drain_all_pages() expects that it drains only pages from a single zone
> > and assumes that caller holds zone->lock.
> 
> As it turns out, with some more testing on SMP systems, this whole patch
> turned out to be incorrect.
> 
> We have been thinking about other approach and, if we were to use something
> else then the first patch from CMAv17[1], the best thing we could came up
> with was to unconditionally call drain_all_pages() at the beginning of
> set_migratetype_isolate() before the call to spin_lock_irqsave().  It has
> a possible race condition but a nightly stress test did have not shown any
> problems.
> 
> Nonetheless, the cleanest, in my opinion, solution is to use the first patch
>  from CMAv17 which can be found at [1].
> 
> So, to sum up: if you intend to test CMAv18, instead of applying this first
> patch either use first patch from CMAv17[1] or put an unconditional call to
> drain_all_pages() at the beginning of set_migrate_isolate() function.
> 
> Sorry for the troubles.
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg148494.html

I've updated our public git repository to include this workaround. You can
pull the patches from the following addresses:

git://git.infradead.org/users/kmpark/linux-samsung 3.2-rc7-cma-v18

http://git.infradead.org/users/kmpark/linux-samsung/shortlog/refs/heads/3.2-rc7-cma-v18

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>