Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760895Ab2FVBUn (ORCPT ); Thu, 21 Jun 2012 21:20:43 -0400 Received: from plane.gmane.org ([80.91.229.3]:58187 "EHLO plane.gmane.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760866Ab2FVBU1 (ORCPT ); Thu, 21 Jun 2012 21:20:27 -0400 X-Injected-Via-Gmane: http://gmane.org/ To: linux-kernel@vger.kernel.org From: Minchan Kim Subject: Re: Accounting problem of MIGRATE_ISOLATED freed page Date: Fri, 22 Jun 2012 10:20:32 +0900 Message-ID: <4FE3C860.4000401@kernel.org> References: <4FE169B1.7020600@kernel.org> <4FE16E80.9000306@gmail.com> <4FE18187.3050103@kernel.org> <4FE23069.5030702@gmail.com> <4FE26470.90401@kernel.org> <4FE27F15.8050102@kernel.org> <4FE2A937.6040701@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-Complaints-To: usenet@dough.gmane.org X-Gmane-NNTP-Posting-Host: 121.50.20.41 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 In-Reply-To: Cc: linux-mm@kvack.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11912 Lines: 329 Hi Aaditya, On 06/21/2012 08:02 PM, Aaditya Kumar wrote: > On Thu, Jun 21, 2012 at 10:25 AM, Minchan Kim wrote: >> On 06/21/2012 11:45 AM, KOSAKI Motohiro wrote: >> >>> On Wed, Jun 20, 2012 at 9:55 PM, Minchan Kim wrote: >>>> On 06/21/2012 10:39 AM, KOSAKI Motohiro wrote: >>>> >>>>>>> number of isolate page block is almost always 0. then if we have such counter, >>>>>>> we almost always can avoid zone->lock. Just idea. >>>>>> >>>>>> Yeb. I thought about it but unfortunately we can't have a counter for MIGRATE_ISOLATE. >>>>>> Because we have to tweak in page free path for pages which are going to free later after we >>>>>> mark pageblock type to MIGRATE_ISOLATE. >>>>> >>>>> I mean, >>>>> >>>>> if (nr_isolate_pageblock != 0) >>>>> free_pages -= nr_isolated_free_pages(); // your counting logic >>>>> >>>>> return __zone_watermark_ok(z, alloc_order, mark, >>>>> classzone_idx, alloc_flags, free_pages); >>>>> >>>>> >>>>> I don't think this logic affect your race. zone_watermark_ok() is already >>>>> racy. then new little race is no big matter. >>>> >>>> >>>> It seems my explanation wasn't enough. :( >>>> I already understand your intention but we can't make nr_isolate_pageblock. >>>> Because we should count two type of free pages. >>> >>> I mean, move_freepages_block increment number of page *block*, not pages. >>> number of free *pages* are counted by zone_watermark_ok_safe(). >>> >>> >>>> 1. Already freed page so they are already in buddy list. >>>> Of course, we can count it with return value of move_freepages_block(zone, page, MIGRATE_ISOLATE) easily. >>>> >>>> 2. Will be FREEed page by do_migrate_range. >>>> It's a _PROBLEM_. For it, we should tweak free path. No? >>> >>> No. >>> >>> >>>> If All of pages are PageLRU when hot-plug happens(ie, 2), nr_isolate_pagblock is zero and >>>> zone_watermk_ok_safe can't do his role. >>> >>> number of isolate pageblock don't depend on number of free pages. It's >>> a concept of >>> an attribute of PFN range. >> >> >> It seems you mean is_migrate_isolate as a just flag, NOT nr_isolate_pageblock. >> So do you mean this? >> >> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h >> index 3bdcab3..7f4d19c 100644 >> --- a/include/linux/page-isolation.h >> +++ b/include/linux/page-isolation.h >> @@ -1,6 +1,7 @@ >> #ifndef __LINUX_PAGEISOLATION_H >> #define __LINUX_PAGEISOLATION_H >> >> +extern bool is_migrate_isolate; >> /* >> * Changes migrate type in [start_pfn, end_pfn) to be MIGRATE_ISOLATE. >> * If specified range includes migrate types other than MOVABLE or CMA, >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index d2a515d..b997cb3 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -1756,6 +1756,27 @@ bool zone_watermark_ok_safe(struct zone *z, int order, unsigned long ma >> if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark) >> free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES); >> >> +#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG >> + if (unlikely(is_migrate_isolate)) { >> + unsigned long flags; >> + spin_lock_irqsave(&z->lock, flags); >> + for (order = MAX_ORDER - 1; order >= 0; order--) { >> + struct free_area *area = &z->free_area[order]; >> + long count = 0; >> + struct list_head *curr; >> + >> + list_for_each(curr, &area->free_list[MIGRATE_ISOLATE]) >> + count++; >> + >> + free_pages -= (count << order); >> + if (free_pages < 0) { >> + free_pages = 0; >> + break; >> + } >> + } >> + spin_unlock_irqrestore(&z->lock, flags); >> + } >> +#endif >> return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags, >> free_pages); >> } >> diff --git a/mm/page_isolation.c b/mm/page_isolation.c >> index c9f0477..212e526 100644 >> --- a/mm/page_isolation.c >> +++ b/mm/page_isolation.c >> @@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages) >> return pfn_to_page(pfn + i); >> } >> >> +bool is_migrate_isolate = false; >> + >> /* >> * start_isolate_page_range() -- make page-allocation-type of range of pages >> * to be MIGRATE_ISOLATE. >> @@ -43,6 +45,8 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, >> BUG_ON((start_pfn) & (pageblock_nr_pages - 1)); >> BUG_ON((end_pfn) & (pageblock_nr_pages - 1)); >> >> + is_migrate_isolate = true; >> + >> for (pfn = start_pfn; >> pfn < end_pfn; >> pfn += pageblock_nr_pages) { >> @@ -59,6 +63,7 @@ undo: >> pfn += pageblock_nr_pages) >> unset_migratetype_isolate(pfn_to_page(pfn), migratetype); >> >> + is_migrate_isolate = false; >> return -EBUSY; >> } >> >> @@ -80,6 +85,9 @@ int undo_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn, >> continue; >> unset_migratetype_isolate(page, migratetype); >> } >> + >> + is_migrate_isolate = false; >> + >> return 0; >> } >> /* >> > > Hello Minchan, > > Sorry for delayed response. > > Instead of above how about something like this: > > diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h > index 3bdcab3..fe9215f 100644 > --- a/include/linux/page-isolation.h > +++ b/include/linux/page-isolation.h > @@ -34,4 +34,6 @@ extern int set_migratetype_isolate(struct page *page); > extern void unset_migratetype_isolate(struct page *page, unsigned migratetype); > > > +extern atomic_t is_migrate_isolated; > + > #endif > diff --git a/mm/memory-failure.c b/mm/memory-failure.c > index ab1e714..e076fa2 100644 > --- a/mm/memory-failure.c > +++ b/mm/memory-failure.c > @@ -1381,6 +1381,7 @@ static int get_any_page(struct page *p, unsigned > long pfn, int flags) > * Isolate the page, so that it doesn't get reallocated if it > * was free. > */ > + atomic_inc(&is_migrate_isolated); I didn't take a detail look in your patch yet. Yes. In my patch, I missed several caller. It was just a patch for showing my intention, NOT formal patch. But I admit I didn't consider nesting case. brain-dead :( Technically other problem about this is atomic doesn't imply memory barrier so we need barrier. But the concern about this approach is following as Copy/Paste from my reply of Kame. *** But the concern about second approach is how to make sure matched count increase/decrease of nr_isolated_areas. I mean how to make sure nr_isolated_areas would be zero when isolation is done. Of course, we can investigate all of current caller and make sure they don't make mistake now. But it's very error-prone if we consider future's user. So we might need test_set_pageblock_migratetype(page, MIGRATE_ISOLATE); IMHO, ideal solution is that we remove MIGRATE_ISOLATE type totally in buddy. ... ... *** Of course, We can choose this approach as interim. What do you think about it, Fujitsu guys? > set_migratetype_isolate(p); > /* > * When the target page is a free hugepage, just remove it > @@ -1406,6 +1407,7 @@ static int get_any_page(struct page *p, unsigned > long pfn, int flags) > } > unset_migratetype_isolate(p, MIGRATE_MOVABLE); > unlock_memory_hotplug(); > + atomic_dec(&is_migrate_isolated); > return ret; > } > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 0d7e3ec..cd7805c 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -892,6 +892,7 @@ static int __ref offline_pages(unsigned long start_pfn, > nr_pages = end_pfn - start_pfn; > > /* set above range as isolated */ > + atomic_inc(&is_migrate_isolated); > ret = start_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE); > if (ret) > goto out; > @@ -958,6 +959,7 @@ repeat: > offline_isolated_pages(start_pfn, end_pfn); > /* reset pagetype flags and makes migrate type to be MOVABLE */ > undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE); > + atomic_dec(&is_migrate_isolated); > /* removal success */ > zone->present_pages -= offlined_pages; > zone->zone_pgdat->node_present_pages -= offlined_pages; > @@ -986,6 +988,7 @@ failed_removal: > undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE); > > out: > + atomic_dec(&is_migrate_isolated); > unlock_memory_hotplug(); > return ret; > } > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 4403009..f549361 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -1632,6 +1632,28 @@ bool zone_watermark_ok_safe(struct zone *z, int > order, unsigned long mark, > if (z->percpu_drift_mark && free_pages < z->percpu_drift_mark) > free_pages = zone_page_state_snapshot(z, NR_FREE_PAGES); > > +#if defined CONFIG_CMA || CONFIG_MEMORY_HOTPLUG > + if (unlikely(atomic_read(is_migrate_isolated)) { > + unsigned long flags; > + spin_lock_irqsave(&z->lock, flags); > + for (order = MAX_ORDER - 1; order >= 0; order--) { > + struct free_area *area = &z->free_area[order]; > + long count = 0; > + struct list_head *curr; > + > + list_for_each(curr, &area->free_list[MIGRATE_ISOLATE]) > + count++; > + > + free_pages -= (count << order); > + if (free_pages < 0) { > + free_pages = 0; > + break; > + } > + } > + spin_unlock_irqrestore(&z->lock, flags); > + } > +#endif > + > return __zone_watermark_ok(z, order, mark, classzone_idx, alloc_flags, > free_pages); > } > @@ -5785,6 +5807,7 @@ int alloc_contig_range(unsigned long start, > unsigned long end, > * put back to page allocator so that buddy can use them. > */ > > + atomic_inc(&is_migrate_isolated); > ret = start_isolate_page_range(pfn_max_align_down(start), > pfn_max_align_up(end), migratetype); > if (ret) > @@ -5854,6 +5877,7 @@ int alloc_contig_range(unsigned long start, > unsigned long end, > done: > undo_isolate_page_range(pfn_max_align_down(start), > pfn_max_align_up(end), migratetype); > + atomic_dec(&is_migrate_isolated); > return ret; > } > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c > index c9f0477..e8eb241 100644 > --- a/mm/page_isolation.c > +++ b/mm/page_isolation.c > @@ -19,6 +19,8 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages) > return pfn_to_page(pfn + i); > } > > +atomic_t is_migrate_isolated; > + > /* > * start_isolate_page_range() -- make page-allocation-type of range of pages > * to be MIGRATE_ISOLATE. > > >> It is still racy as you already mentioned and I don't think it's trivial. >> Direct reclaim can't wake up kswapd forever by current fragile zone->all_unreclaimable. >> So it's a livelock. >> Then, do you want to fix this problem by your patch[1]? >> >> It could solve the livelock by OOM kill if we apply your patch[1] but still doesn't wake up >> kswapd although it's not critical. Okay. Then, please write down this problem in detail >> in your patch's changelog and resend, please. >> >> [1] http://lkml.org/lkml/2012/6/14/74 >> >> -- >> Kind regards, >> Minchan Kim -- Kind regards, Minchan Kim -- 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/