Re: [PATCH] do_migrate_range: avoid failure as much as possible

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

On Mon, Oct 25, 2010 at 11:05:50AM +0800, KAMEZAWA Hiroyuki wrote:
> On Mon, 25 Oct 2010 10:57:03 +0800
> Wu Fengguang <fengguang.wu@intel.com> wrote:
> 
> > On Mon, Oct 25, 2010 at 10:40:17AM +0800, KAMEZAWA Hiroyuki wrote:
> > > On Mon, 25 Oct 2010 10:47:31 +0800
> > > Bob Liu <lliubbo@gmail.com> wrote:
> > > 
> > > > It's normal for isolate_lru_page() to fail at times. 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.
> > > > 
> > > > This patch remove page_count() check and remove putback_lru_pages() and
> > > > call migrate_pages() regardless of not_managed to reduce failure as much
> > > > as possible.
> > > > 
> > > > Signed-off-by: Bob Liu <lliubbo@gmail.com>
> > > 
> > > -EBUSY should be returned.
> > 
> > It does return -EBUSY when ALL pages cannot be isolated from LRU (or
> > is non-LRU pages at all). That means offline_pages() will repeat calls
> > to do_migrate_range() as fast as possible as long as it can make
> > progress.
> > 
> I read the patch wrong ? "ret = -EBUSY" is dropped and "ret" will be
> 0 or just a return code of migrate_page().

        for () {
                ret = isolate_lru_page(page);
        }

        if (list_empty(&source))
                goto out;

out:
        return ret;

So do_migrate_range() will return -EBUSY if the last isolate_lru_page() returns
-EBUSY.

> 
> 
> 
> > Is that behavior good enough? It does need some comment for this
> > non-obvious return value. 
> > 
> > btw, the caller side code can be simplified (no behavior change).
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index dd186c1..606d358 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -848,17 +848,13 @@ repeat:
> >     pfn = scan_lru_pages(start_pfn, end_pfn);
> >     if (pfn) { /* We have page on LRU */
> >             ret = do_migrate_range(pfn, end_pfn);
> > -           if (!ret) {
> > -                   drain = 1;
> > -                   goto repeat;
> > -           } else {
> > -                   if (ret < 0)
> > -                           if (--retry_max == 0)
> > -                                   goto failed_removal;
> > +           if (ret < 0) {
> > +                   if (--retry_max <= 0)
> > +                           goto failed_removal;
> >                     yield();
> > -                   drain = 1;
> > -                   goto repeat;
> >             }
> > +           drain = 1;
> > +           goto repeat;
> >     }
> 
> This changes behavior.

Ah yes!
 
> This "ret" can be > 0 because migrate_page()'s return code is
> "Return: Number of pages not migrated or error code."
> 
> Then, 
> ret < 0  ===> maybe ebusy
> ret > 0  ===> some pages are not migrated. maybe PG_writeback or some
> ret == 0 ===> ok, all condition green. try next chunk soon.
> 
> Then, I added "yield()" and --retrym_max for !ret cases.

You are right, there is the "ret > 0, some pages are not migrated" case.
But I'm not sure it's PG_writeback pages, because migrate_pages() will wait on
writeback after pass 2.

Thanks,
Fengguang

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