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

[Pingfan Liu <kernelfans@gmail.com>]

On Mon, Mar 23, 2020 at 3:34 PM Michal Hocko <mhocko@kernel.org> 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?
I am not quite sure about zone_device, but I will try to explain it later.

But first of all, I think the code conflicts with the logic behind it.
If try_to_unmap_one() success to unmap a page, then it should kill the
pte, and return true. But the original code return true before the
code like "ptep_clear_flush()"

Now, I try to say about !device_private zone device. (Please pardon
and correct me if I make a mistake)
memmap_init_zone_device() raises an extra _refcount on all zone
device. And private-device should lifts the count later, otherwise it
can not migrate. But I did not find the exact place yet.

While this extra _refcount will block migration, it is not the whole
reason if a zone device page is mapped.

If  a zone device page is mapped, then I think the original code
happen to work due to it skip the call of page_remove_rmap(), and in
try_to_unmap(){ return !page_mapcount(page) ? true : false;}.
>
> What is the real user visible problem here?
As explained, the original code happens to work, but it conflicts with
the logic.

Thanks,
Pingfan