Re: [PATCH] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind

From: Yang Shi <yang.shi@linux.alibaba.com>
To: Vlastimil Babka <vbabka@suse.cz>, Michal Hocko <mhocko@kernel.org>
Cc: akpm@linux-foundation.org, mgorman@techsingularity.net,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	netdev@vger.kernel.org
Subject: Re: [PATCH] mm: mempolicy: handle vma with unmovable pages mapped correctly in mbind
Date: Wed, 19 Jun 2019 11:19:09 -0700	[thread overview]
Message-ID: <55eb2ea9-2c74-87b1-4568-b620c7913e17@linux.alibaba.com> (raw)
In-Reply-To: <687f4e57-5c50-7900-645e-6ef3a5c1c0c7@linux.alibaba.com>

On 6/19/19 9:21 AM, Yang Shi wrote:
>
>
> On 6/19/19 1:22 AM, Vlastimil Babka wrote:
>> On 6/19/19 7:21 AM, Michal Hocko wrote:
>>> On Tue 18-06-19 14:13:16, Yang Shi wrote:
>>> [...]
>>>> I used to have !__PageMovable(page), but it was removed since the
>>>> aforementioned reason. I could add it back.
>>>>
>>>> For the temporary off LRU page, I did a quick search, it looks the 
>>>> most
>>>> paths have to acquire mmap_sem, so it can't race with us here. Page
>>>> reclaim/compaction looks like the only race. But, since the mapping 
>>>> should
>>>> be preserved even though the page is off LRU temporarily unless the 
>>>> page is
>>>> reclaimed, so we should be able to exclude temporary off LRU pages by
>>>> calling page_mapping() and page_anon_vma().
>>>>
>>>> So, the fix may look like:
>>>>
>>>> if (!PageLRU(head) && !__PageMovable(page)) {
>>>>      if (!(page_mapping(page) || page_anon_vma(page)))
>>>>          return -EIO;
>>> This is getting even more muddy TBH. Is there any reason that we 
>>> have to
>>> handle this problem during the isolation phase rather the migration?
>> I think it was already said that if pages can't be isolated, then
>> migration phase won't process them, so they're just ignored.
>
> Yes，exactly.
>
>> However I think the patch is wrong to abort immediately when
>> encountering such page that cannot be isolated (AFAICS). IMHO it should
>> still try to migrate everything it can, and only then return -EIO.
>
> It is fine too. I don't see mbind semantics define how to handle such 
> case other than returning -EIO.

By looking into the code, it looks not that easy as what I thought. 
do_mbind() would check the return value of queue_pages_range(), it just 
applies the policy and manipulates vmas as long as the return value is 0 
(success), then migrate pages on the list. We could put the movable 
pages on the list by not breaking immediately, but they will be ignored. 
If we migrate the pages regardless of the return value, it may break the 
policy since the policy will *not* be applied at all.

>
>