Re: [RFC v2 PATCH] mm/hotplug: enable memory hotplug for non-lru movable pages

[Yisheng Xie]

hi Michal,
Thank you for reviewing and sorry for late reply.

On 01/26/2017 05:43 PM, Michal Hocko wrote:
> On Wed 25-01-17 14:59:45, Yisheng Xie wrote:
>
>  static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
>  {
> @@ -1531,6 +1531,16 @@ static unsigned long scan_movable_pages(unsigned long start, unsigned long end)
>  					pfn = round_up(pfn + 1,
>  						1 << compound_order(page)) - 1;
>  			}
> +			/*
> +			 * check __PageMovable in lock_page to avoid miss some
> +			 * non-lru movable pages at race condition.
> +			 */
> +			lock_page(page);
> +			if (__PageMovable(page)) {
> +				unlock_page(page);
> +				return pfn;
> +			}
> +			unlock_page(page);
> This doesn't make any sense to me. __PageMovable can change right after
> you drop the lock so why the race matters? If we cannot tolerate races
> then the above doesn't work and if we can then taking the lock is
> pointless.
hmm, for PageLRU check may also race without lru-lockï
I think it is ok to check __PageMovable without lock_page, here.

>>  		}
>>  	}
>>  	return 0;
>> @@ -1600,21 +1610,25 @@ static struct page *new_node_page(struct page *page, unsigned long private,
>>  		if (!get_page_unless_zero(page))
>>  			continue;
>>  		/*
>> -		 * We can skip free pages. And we can only deal with pages on
>> -		 * LRU.
>> +		 * We can skip free pages. And we can deal with pages on
>> +		 * LRU and non-lru movable pages.
>>  		 */
>> -		ret = isolate_lru_page(page);
>> +		if (PageLRU(page))
>> +			ret = isolate_lru_page(page);
>> +		else
>> +			ret = !isolate_movable_page(page, ISOLATE_UNEVICTABLE);
> we really want to propagate the proper error code to the caller.
Yes , I make the same mistake again. Really sorry about that.

Maybe I can rewrite the isolate_movable_page to let it return int as isolate_lru_page
do in this patchset :)

Thanks
Yisheng Xie