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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 30 Jan 2017 23:14:23 +0800
From:   Yisheng Xie <ysxie@...mail.com>
To:     Michal Hocko <mhocko@...nel.org>,
        Yisheng Xie <xieyisheng1@...wei.com>
Cc:     linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org, vbabka@...e.cz,
        mgorman@...hsingularity.net, hannes@...xchg.org,
        iamjoonsoo.kim@....com, izumi.taku@...fujitsu.com,
        arbab@...ux.vnet.ibm.com, vkuznets@...hat.com, ak@...ux.intel.com,
        n-horiguchi@...jp.nec.com, minchan@...nel.org, qiuxishi@...wei.com,
        guohanjun@...wei.com
Subject: Re: [RFC v2 PATCH] mm/hotplug: enable memory hotplug for non-lru
 movable pages


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