Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754718AbaKNI1u (ORCPT ); Fri, 14 Nov 2014 03:27:50 -0500 Received: from mail-ig0-f171.google.com ([209.85.213.171]:53917 "EHLO mail-ig0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754693AbaKNI1q (ORCPT ); Fri, 14 Nov 2014 03:27:46 -0500 MIME-Version: 1.0 In-Reply-To: <1414740330-4086-2-git-send-email-iamjoonsoo.kim@lge.com> References: <1414740330-4086-1-git-send-email-iamjoonsoo.kim@lge.com> <1414740330-4086-2-git-send-email-iamjoonsoo.kim@lge.com> Date: Fri, 14 Nov 2014 16:27:46 +0800 Message-ID: Subject: Re: [PATCH v5 1/4] mm/page_alloc: fix incorrect isolation behavior by rechecking migratetype From: Weijie Yang To: Joonsoo Kim Cc: Andrew Morton , "Kirill A. Shutemov" , Rik van Riel , Peter Zijlstra , Mel Gorman , Johannes Weiner , Minchan Kim , Yasuaki Ishimatsu , Zhang Yanfei , Tang Chen , Naoya Horiguchi , Bartlomiej Zolnierkiewicz , Wen Congyang , Marek Szyprowski , Michal Nazarewicz , Laura Abbott , Heesub Shin , "Aneesh Kumar K.V" , Ritesh Harjani , t.stanislaws@samsung.com, Gioh Kim , Vlastimil Babka , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 31, 2014 at 3:25 PM, Joonsoo Kim wrote: > There are two paths to reach core free function of buddy allocator, > __free_one_page(), one is free_one_page()->__free_one_page() and the > other is free_hot_cold_page()->free_pcppages_bulk()->__free_one_page(). > Each paths has race condition causing serious problems. At first, this > patch is focused on first type of freepath. And then, following patch > will solve the problem in second type of freepath. > > In the first type of freepath, we got migratetype of freeing page without > holding the zone lock, so it could be racy. There are two cases of this > race. > > 1. pages are added to isolate buddy list after restoring orignal > migratetype > > CPU1 CPU2 > > get migratetype => return MIGRATE_ISOLATE > call free_one_page() with MIGRATE_ISOLATE > > grab the zone lock > unisolate pageblock > release the zone lock > > grab the zone lock > call __free_one_page() with MIGRATE_ISOLATE > freepage go into isolate buddy list, > although pageblock is already unisolated > > This may cause two problems. One is that we can't use this page anymore > until next isolation attempt of this pageblock, because freepage is on > isolate buddy list. The other is that freepage accouting could be wrong > due to merging between different buddy list. Freepages on isolate buddy > list aren't counted as freepage, but ones on normal buddy list are counted > as freepage. If merge happens, buddy freepage on normal buddy list is > inevitably moved to isolate buddy list without any consideration of > freepage accouting so it could be incorrect. > > 2. pages are added to normal buddy list while pageblock is isolated. > It is similar with above case. > > This also may cause two problems. One is that we can't keep these > freepages from being allocated. Although this pageblock is isolated, > freepage would be added to normal buddy list so that it could be > allocated without any restriction. And the other problem is same as > case 1, that it, incorrect freepage accouting. > > This race condition would be prevented by checking migratetype again > with holding the zone lock. Because it is somewhat heavy operation > and it isn't needed in common case, we want to avoid rechecking as much > as possible. So this patch introduce new variable, nr_isolate_pageblock > in struct zone to check if there is isolated pageblock. > With this, we can avoid to re-check migratetype in common case and do > it only if there is isolated pageblock or migratetype is MIGRATE_ISOLATE. > This solve above mentioned problems. > > Changes from v3: > Add one more check in free_one_page() that checks whether migratetype is > MIGRATE_ISOLATE or not. Without this, abovementioned case 1 could happens. > > Cc: > Acked-by: Minchan Kim > Acked-by: Michal Nazarewicz > Acked-by: Vlastimil Babka > Signed-off-by: Joonsoo Kim > --- > include/linux/mmzone.h | 9 +++++++++ > include/linux/page-isolation.h | 8 ++++++++ > mm/page_alloc.c | 11 +++++++++-- > mm/page_isolation.c | 2 ++ > 4 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h > index 4593567..3d090af 100644 > --- a/include/linux/mmzone.h > +++ b/include/linux/mmzone.h > @@ -431,6 +431,15 @@ struct zone { > */ > int nr_migrate_reserve_block; > > +#ifdef CONFIG_MEMORY_ISOLATION > + /* > + * Number of isolated pageblock. It is used to solve incorrect > + * freepage counting problem due to racy retrieving migratetype > + * of pageblock. Protected by zone->lock. > + */ > + unsigned long nr_isolate_pageblock; > +#endif > + First sorry for this deferred reply, I see these patches have been merged into the mainline. However, I still have a tiny question: Why use ZONE_PADDING(_pad1_) seperate it and zone->lock? How about move it to the same cacheline with zone->lock, because it is accessed under zone->lock? > #ifdef CONFIG_MEMORY_HOTPLUG > /* see spanned/present_pages for more description */ > seqlock_t span_seqlock; > diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h > index 3fff8e7..2dc1e16 100644 > --- a/include/linux/page-isolation.h > +++ b/include/linux/page-isolation.h > @@ -2,6 +2,10 @@ > #define __LINUX_PAGEISOLATION_H > > #ifdef CONFIG_MEMORY_ISOLATION > +static inline bool has_isolate_pageblock(struct zone *zone) > +{ > + return zone->nr_isolate_pageblock; > +} > static inline bool is_migrate_isolate_page(struct page *page) > { > return get_pageblock_migratetype(page) == MIGRATE_ISOLATE; > @@ -11,6 +15,10 @@ static inline bool is_migrate_isolate(int migratetype) > return migratetype == MIGRATE_ISOLATE; > } > #else > +static inline bool has_isolate_pageblock(struct zone *zone) > +{ > + return false; > +} > static inline bool is_migrate_isolate_page(struct page *page) > { > return false; > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d3ed723..f7a867e 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -749,9 +749,16 @@ static void free_one_page(struct zone *zone, > if (nr_scanned) > __mod_zone_page_state(zone, NR_PAGES_SCANNED, -nr_scanned); > > + if (unlikely(has_isolate_pageblock(zone) || > + is_migrate_isolate(migratetype))) { > + migratetype = get_pfnblock_migratetype(page, pfn); > + if (is_migrate_isolate(migratetype)) > + goto skip_counting; > + } > + __mod_zone_freepage_state(zone, 1 << order, migratetype); > + > +skip_counting: > __free_one_page(page, pfn, zone, order, migratetype); > - if (unlikely(!is_migrate_isolate(migratetype))) > - __mod_zone_freepage_state(zone, 1 << order, migratetype); > spin_unlock(&zone->lock); > } > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index d1473b2..1fa4a4d 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -60,6 +60,7 @@ out: > int migratetype = get_pageblock_migratetype(page); > > set_pageblock_migratetype(page, MIGRATE_ISOLATE); > + zone->nr_isolate_pageblock++; > nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE); > > __mod_zone_freepage_state(zone, -nr_pages, migratetype); > @@ -83,6 +84,7 @@ void unset_migratetype_isolate(struct page *page, unsigned migratetype) > nr_pages = move_freepages_block(zone, page, migratetype); > __mod_zone_freepage_state(zone, nr_pages, migratetype); > set_pageblock_migratetype(page, migratetype); > + zone->nr_isolate_pageblock--; > out: > spin_unlock_irqrestore(&zone->lock, flags); > } > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/