Re: [PATCH 2/3] do_migrate_range: exit loop if not_managed is true.

[Wu Fengguang <fengguang.wu@intel.com>]

On Fri, Oct 22, 2010 at 04:41:40PM +0800, Bob Liu wrote:
> On Fri, Oct 22, 2010 at 11:22 AM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> > On Fri, Oct 22, 2010 at 10:48:51AM +0800, Bob Liu wrote:
> >> On Thu, Oct 21, 2010 at 10:25 PM, Wu Fengguang <fengguang.wu@intel.com> wrote:
> >> > On Thu, Oct 21, 2010 at 09:28:20PM +0800, Bob Liu wrote:
> >> >> If not_managed is true all pages will be putback to lru, so
> >> >> break the loop earlier to skip other pages isolate.
> >> >
> >> > It's good fix in itself. However it's normal for isolate_lru_page() to
> >> > fail at times (when there are active reclaimers). The failures are
> >> > typically temporal and may well go away when offline_pages() retries
> >> > the call. So it seems more reasonable to migrate as much as possible
> >> > to increase the chance of complete success in next retry.
> >> >
> >>
> >> Hi, Wu
> >>
> >> The original code will try to migrate pages as much as possible except
> >> page_count(page) is true.
> >> If page_count(page) is true, isolate more pages is mean-less, because
> >> all of them will
> >> be put back after the loop.
> >>
> >> Or maybe we can skip the page_count() check? A It seems unreasonable,
> >> if isolate one page failed and
> >> that page was in use why it needs to put back the whole isolated list?
> >
> > My suggestion was to keep the page_count() check and remove
> > putback_lru_pages() and call migrate_pages() regardless of
> > not_managed.
> >
> 
> If not_managed is no more used, page_count() will also meanless.

Ah yes!

> You mean patch like this:

That's it.

I guess the typical usage would be

        for each 128M block
                try to offline it
                if failed
                        break

So it makes sense to avoid failure as much as possible, since the
above process is virtually not revertible -- if we failed at the
10th 128M block, the pages in the first 9 blocks have already been
migrated. It helps little to avoid page migration inside the 10th
block. The best we can do is to ensure success as much as possible
once the memory offline process starts.

And it's nice to reduce some code :)

Thanks,
Fengguang

> ==
> @@ -687,7 +687,6 @@
>  	unsigned long pfn;
>  	struct page *page;
>  	int move_pages = NR_OFFLINE_AT_ONCE_PAGES;
> -	int not_managed = 0;
>  	int ret = 0;
>  	LIST_HEAD(source);
> 
> @@ -709,10 +708,6 @@
>  					    page_is_file_cache(page));
> 
>  		} else {
> -			/* Becasue we don't have big zone->lock. we should
> -			   check this again here. */
> -			if (page_count(page))
> -				not_managed++;
>  #ifdef CONFIG_DEBUG_VM
>  			printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
>  			       pfn);
> @@ -720,12 +715,6 @@
>  #endif
>  		}
>  	}
> -	ret = -EBUSY;
> -	if (not_managed) {
> -		if (!list_empty(&source))
> -			putback_lru_pages(&source);
> -		goto out;
> -	}
>  	ret = 0;
>  	if (list_empty(&source))
>  		goto out;
> ==
> Thanks,
> 
> > Does that make sense for typical memory hot remove scenarios?
> > That will increase the possibility of success at the cost of some more
> > migrated pages in case memory offline fails.
> >
> > Thanks,
> > Fengguang
> >
> >> >> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> >> >> ---
> >> >> A mm/memory_hotplug.c | A  10 ++++++----
> >> >> A 1 files changed, 6 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> >> index d4e940a..4f72184 100644
> >> >> --- a/mm/memory_hotplug.c
> >> >> +++ b/mm/memory_hotplug.c
> >> >> @@ -709,15 +709,17 @@ do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
> >> >> A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  page_is_file_cache(page));
> >> >>
> >> >> A  A  A  A  A  A  A  } else {
> >> >> - A  A  A  A  A  A  A  A  A  A  /* Becasue we don't have big zone->lock. we should
> >> >> - A  A  A  A  A  A  A  A  A  A  A  A check this again here. */
> >> >> - A  A  A  A  A  A  A  A  A  A  if (page_count(page))
> >> >> - A  A  A  A  A  A  A  A  A  A  A  A  A  A  not_managed++;
> >> >> A #ifdef CONFIG_DEBUG_VM
> >> >> A  A  A  A  A  A  A  A  A  A  A  printk(KERN_ALERT "removing pfn %lx from LRU failed\n",
> >> >> A  A  A  A  A  A  A  A  A  A  A  A  A  A  A pfn);
> >> >> A  A  A  A  A  A  A  A  A  A  A  dump_page(page);
> >> >> A #endif
> >> >> + A  A  A  A  A  A  A  A  A  A  /* Becasue we don't have big zone->lock. we should
> >> >> + A  A  A  A  A  A  A  A  A  A  A  A check this again here. */
> >> >> + A  A  A  A  A  A  A  A  A  A  if (page_count(page)) {
> >> >> + A  A  A  A  A  A  A  A  A  A  A  A  A  A  not_managed++;
> >> >> + A  A  A  A  A  A  A  A  A  A  A  A  A  A  break;
> >> >> + A  A  A  A  A  A  A  A  A  A  }
> >> >> A  A  A  A  A  A  A  }
> >> >> A  A  A  }
> >> >> A  A  A  ret = -EBUSY;
> >> >> --
> -- 
> Regards,
> --Bob

--
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/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>