Re: [PATCH] mm/rmap: fix the handling of device private page in try_to_unmap_one()

[John Hubbard <jhubbard@nvidia.com>]

On 3/23/20 12:34 AM, Michal Hocko wrote:
> On Sun 22-03-20 21:57:07, Pingfan Liu wrote:
>> For zone_device, migration can only happen on is_device_private_page(page).
>> Correct the logic in try_to_unmap_one().
> 
> Maybe it is just me lacking knowledge in the zone_device ZOO. But
> this really deserves a much more detailed explanation IMHO. It seems
> a5430dda8a3a ("mm/migrate: support un-addressable ZONE_DEVICE page in
> migration") deliberately made the decision to allow unmapping these
> pages? Is the check just wrong, inncomplete? Why?
> 
> What is the real user visible problem here?
>   

I was going to guess that someone was having trouble with the behavior on
non-device-private ZONE_DEVICE pages, but...I am also at a loss as to
what triggered this patch. So, I have the exact same questions as Michal,
plus really wondering what tests you are running, and on what hardware config.

It's hard to get the right CC's, but probably Dan Williams and Ralph Campbell
should also be added, if you are sure you need this. (I'm adding them now.)

thanks,
-- 
John Hubbard
NVIDIA

>> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
>> Cc: Jérôme Glisse <jglisse@redhat.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: John Hubbard <jhubbard@nvidia.com>
>> Cc: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Cc: Aneesh Kumar <aneesh.kumar@linux.vnet.ibm.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> To: linux-mm@kvack.org
>> ---
>>   mm/rmap.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index b838647..ffadf3e 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1380,7 +1380,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>
>>   	if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) &&
>>   	    is_zone_device_page(page) && !is_device_private_page(page))
>> -		return true;
>> +		return false;
>>
>>   	if (flags & TTU_SPLIT_HUGE_PMD) {
>>   		split_huge_pmd_address(vma, address,
>> @@ -1487,7 +1487,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>>
>>   		if (IS_ENABLED(CONFIG_MIGRATION) &&
>>   		    (flags & TTU_MIGRATION) &&
>> -		    is_zone_device_page(page)) {
>> +		    is_device_private_page(page)) {
>>   			swp_entry_t entry;
>>   			pte_t swp_pte;
>>
>> --
>> 2.7.5
>