On Fri, Oct 22, 2010 at 11:22 AM, Wu Fengguang <fengguang.wu@xxxxxxxxx> 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@xxxxxxxxx> 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? Â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.
You mean patch like this:
==
@@ -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@xxxxxxxxx>
>> >> ---
>> >> Âmm/memory_hotplug.c | Â 10 ++++++----
>> >> Â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)
>> >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â 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);
>> >> Â Â Â Â Â Â Â Â Â Â Â dump_page(page);
>> >> Â#endif
>> >> + Â Â Â Â Â Â Â Â Â Â /* Becasue we don't have big zone->lock. we should
>> >> + Â Â Â Â Â Â Â Â Â Â Â Âcheck this again here. */
>> >> + Â Â Â Â Â Â Â Â Â Â if (page_count(page)) {
>> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â not_managed++;
>> >> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
>> >> + Â Â Â Â Â Â Â Â Â Â }
>> >> Â Â Â Â Â Â Â }
>> >> Â Â Â }
>> >> Â Â Â ret = -EBUSY;
>> >> --
--
Regards,
--Bob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href