Thanks for your explanation.
On Tue, Mar 24, 2020 at 8:20 AM Ralph Campbell <rcampbell@xxxxxxxxxx> 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@xxxxxxxxx>
> >> Cc: Jérôme Glisse <jglisse@xxxxxxxxxx>
> >> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> >> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> >> Cc: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> >> Cc: Aneesh Kumar <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> >> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> >> To: linux-mm@xxxxxxxxx
> >> ---
> >> 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
