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

[Bob Liu <lliubbo@xxxxxxxxx>]

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?

Thanks

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