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

[Pingfan Liu <kernelfans@gmail.com>]

Thanks for your explanation.

On Tue, Mar 24, 2020 at 8:20 AM Ralph Campbell <rcampbell@nvidia.com> wrote:
>
>
> 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?
> >
> >> 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;
>
> Pages can be mapped in multiple vmas. Returning false here will only break out
> of the loop and skip any other vmas mapping this page which is a minor optimization
> but shouldn't really affect what try_to_unmap_one() does.
Yes, it returns false to terminate further iteration. And I think
fs-dax page would not go through this path.

The unmap of fs-dax should go through: umount fs, and arch_remove_memory().
>
> >>      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;
>
> Since the page was checked for !device private, this is more clear but shouldn't
> change anything.
Yes. It just makes things clear.

Thanks,
Pingfan