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
