mm/rmap: fix the handling of device private page in try_to_unmap_one()

PATCH In review Pingfan Liu 5 participants 15 messages Mar 22, 2020 – Apr 2, 2020

Subsystems (2)

Memory management MM rmap

lore

Share & tools

lore

Michal Hocko

2020-03-23 07:34 UTC #1 lore raw

Quoted 2 lines · Pingfan Liu

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?

Quoted 36 lines

Signed-off-by Pingfan Liu <[email protected]>

Cc Jérôme Glisse <[email protected]>

Cc Michal Hocko <[email protected]>

Cc John Hubbard <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Cc Aneesh Kumar <[email protected]>

Cc Andrew Morton <[email protected]>

To: [email protected]

---
 mm/rmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

mm/rmap.c +2 -3

index b838647..ffadf3e 100644
@@ -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

Signature · 2 lines

Michal Hocko
SUSE Labs

John Hubbard

2020-03-23 23:32 UTC #2 lore raw

Quoted 4 lines · Michal Hocko

Quoted 2 lines · Pingfan Liu
For zone_device, migration can only happen on is_device_private_page(page). Correct the logic in try_to_unmap_one().

Quoted 8 lines

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,

Signature · 2 lines

John Hubbard
NVIDIA

Quoted 37 lines

Quoted 36 lines

Signed-off-by Pingfan Liu <[email protected]>

Cc Jérôme Glisse <[email protected]>

Cc Michal Hocko <[email protected]>

Cc John Hubbard <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Cc Aneesh Kumar <[email protected]>

Cc Andrew Morton <[email protected]>

To: [email protected]

---
  mm/rmap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

mm/rmap.c +2 -3

index b838647..ffadf3e 100644
@@ -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

Pingfan Liu

author

2020-03-24 03:50 UTC #3 lore raw

Quoted 12 lines · John Hubbard

Quoted 10 lines · Michal Hocko
Quoted 2 lines · Pingfan Liu
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?

Quoted 7 lines

Quoted 2 lines
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.

:), please see my reply to Michal.

Quoted 3 lines

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.)

Appreciate you to do that. In fact, I am not sure about some detail, and hope someone can correct me if I make mistake.

Thanks,
Pingfan

Ralph Campbell

2020-03-24 00:20 UTC #4 lore raw

In reply to Michal Hocko

Quoted 30 lines · Michal Hocko

Quoted 2 lines · Pingfan Liu
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?
Quoted 18 lines
Signed-off-by Pingfan Liu <[email protected]>
Cc Jérôme Glisse <[email protected]>
Cc Michal Hocko <[email protected]>
Cc John Hubbard <[email protected]>
Cc Kirill A. Shutemov <[email protected]>
Cc Aneesh Kumar <[email protected]>
Cc Andrew Morton <[email protected]>
To: [email protected]
---
  mm/rmap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
mm/rmap.c +0 -0
index b838647..ffadf3e 100644
@@ -1380,7 +1380,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 

Quoted 4 lines

mm/rmap.c static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, · line 1383
Quoted 4 lines
mm/rmap.c +1 -1
  	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.

Quoted 10 lines

mm/rmap.c static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, · line 1492

Quoted 10 lines

mm/rmap.c +1 -1

  	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.

Pingfan Liu

author

2020-03-24 04:22 UTC #5 lore raw

Thanks for your explanation.

Quoted 33 lines · Ralph Campbell

Quoted 30 lines · Michal Hocko
Quoted 2 lines · Pingfan Liu
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?
Quoted 18 lines
Signed-off-by Pingfan Liu <[email protected]>
Cc Jérôme Glisse <[email protected]>
Cc Michal Hocko <[email protected]>
Cc John Hubbard <[email protected]>
Cc Kirill A. Shutemov <[email protected]>
Cc Aneesh Kumar <[email protected]>
Cc Andrew Morton <[email protected]>
To: [email protected]
---
  mm/rmap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
mm/rmap.c +0 -0
index b838647..ffadf3e 100644
@@ -1380,7 +1380,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 

Quoted 8 lines

Quoted 4 lines
mm/rmap.c static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, · line 1383
Quoted 4 lines
mm/rmap.c +1 -1
     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().

Quoted 5 lines

mm/rmap.c +0 -0

     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,

Quoted 9 lines

Quoted 6 lines
mm/rmap.c static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, · line 1492
Quoted 6 lines
mm/rmap.c +1 -1
             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

Pingfan Liu

author

2020-03-24 03:47 UTC #6 lore raw

In reply to Michal Hocko

Quoted 10 lines · Michal Hocko

Quoted 2 lines · Pingfan Liu
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;}.

Quoted 2 lines

What is the real user visible problem here?

As explained, the original code happens to work, but it conflicts with the logic.

Thanks,
Pingfan

Michal Hocko

2020-03-24 09:14 UTC #7 lore raw

Quoted 24 lines · Pingfan Liu

Quoted 10 lines · Michal Hocko
Quoted 2 lines · Pingfan Liu
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.

Quoted 6 lines

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;}.

OK, you made me look more closely.

The lack of documentation and therefore the expected semantic doesn't really help. So we can only deduce from the existing code which is a recipe for cargo cult programming :/

The only difference betweena rmap_one returning true and false is that the VMA walk stops for the later and done() callback is not called. Does rmap_one success means the mapping for the vma has been torn down? No. As we can see for the munlock case which just shows hot vague the semantic of this callback might be.

I believe the zone device path was just copying it. Is that wrong? Well, it is less optimal than necessary because the property we are checking is not VMA specific so all other VMAs (if there are any at all) will have the same to say.

So the only last remaining point is the done() callback. That one is documented as a check. There is no note about potential side effects but the current implementation is really only a check so skipping it doesn't make any real difference.

Quoted 3 lines

Quoted 1 line
What is the real user visible problem here?
As explained, the original code happens to work, but it conflicts with the logic.

Your changelog should be explicit about this being a pure code refinement/cleanup without any functional changes.

The rmap walk and callbacks would benefit from a proper documentation. Hint...

Signature · 2 lines

Michal Hocko
SUSE Labs

Pingfan Liu

author

2020-03-25 10:54 UTC #8 lore raw

Quoted 55 lines · Michal Hocko

Quoted 24 lines · Pingfan Liu
Quoted 10 lines · Michal Hocko
Quoted 2 lines · Pingfan Liu
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.
Quoted 6 lines
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;}.
OK, you made me look more closely.
The lack of documentation and therefore the expected semantic doesn't really help. So we can only deduce from the existing code which is a recipe for cargo cult programming :/
The only difference betweena rmap_one returning true and false is that the VMA walk stops for the later and done() callback is not called. Does rmap_one success means the mapping for the vma has been torn down? No. As we can see for the munlock case which just shows hot vague the semantic of this callback might be.
I believe the zone device path was just copying it. Is that wrong? Well, it is less optimal than necessary because the property we are checking is not VMA specific so all other VMAs (if there are any at all) will have the same to say.
So the only last remaining point is the done() callback. That one is documented as a check. There is no note about potential side effects but the current implementation is really only a check so skipping it doesn't make any real difference.

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
What is the real user visible problem here?
As explained, the original code happens to work, but it conflicts with the logic.
Your changelog should be explicit about this being a pure code refinement/cleanup without any functional changes.

OK, I will do that.

Quoted 3 lines

The rmap walk and callbacks would benefit from a proper documentation. Hint...

I will add some note around try_to_unmap_one(), and update the commit log. (Hope my understanding of zone device is right, and will cc more people for comment)

Thanks,
Pingfan

Pingfan Liu

author

2020-04-01 14:10 UTC #9 lore raw

Quoted 57 lines · Pingfan Liu

Quoted 55 lines · Michal Hocko
Quoted 24 lines · Pingfan Liu
Quoted 10 lines · Michal Hocko
Quoted 2 lines · Pingfan Liu
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.
Quoted 6 lines
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;}.
OK, you made me look more closely.
The lack of documentation and therefore the expected semantic doesn't really help. So we can only deduce from the existing code which is a recipe for cargo cult programming :/
The only difference betweena rmap_one returning true and false is that the VMA walk stops for the later and done() callback is not called. Does rmap_one success means the mapping for the vma has been torn down? No. As we can see for the munlock case which just shows hot vague the semantic of this callback might be.
I believe the zone device path was just copying it. Is that wrong? Well, it is less optimal than necessary because the property we are checking is not VMA specific so all other VMAs (if there are any at all) will have the same to say.
So the only last remaining point is the done() callback. That one is documented as a check. There is no note about potential side effects but the current implementation is really only a check so skipping it doesn't make any real difference.

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
What is the real user visible problem here?
As explained, the original code happens to work, but it conflicts with the logic.
Your changelog should be explicit about this being a pure code refinement/cleanup without any functional changes.
OK, I will do that.

It took me some time to make clear try_to_munlock(). And now I can make some notes. I will send out V2 soon.

Thanks,
Pingfan

Balbir Singh

2020-03-24 00:04 UTC #10 lore raw

In reply to Pingfan Liu

Quoted 2 lines · Pingfan Liu

For zone_device, migration can only happen on is_device_private_page(page). Correct the logic in try_to_unmap_one().

!DEVICE_PRIVATE implies it's a cache coherent page and we shouldn't bail out - Could you clarify what issue your fixing?

Balbir Singh.

Pingfan Liu

author

2020-03-24 03:55 UTC #11 lore raw

Quoted 7 lines · Balbir Singh

Quoted 2 lines · Pingfan Liu
For zone_device, migration can only happen on is_device_private_page(page). Correct the logic in try_to_unmap_one().
!DEVICE_PRIVATE implies it's a cache coherent page and we shouldn't bail out - Could you clarify what issue your fixing?

As I replied to Michal, I tried to fix the code logic, but not really hit a bug.

Thanks,
Pingfan

Pingfan Liu

author

2020-04-01 14:20 UTC #12 lore raw

PATCH 1/1 mm/rmap: fix the handling of !private device page in try_to_unmap_one()

In reply to Pingfan Liu

This patch is a pure code refinement without any functional changes.

try_to_unmap_one() is shared by try_to_unmap() and try_to_munlock(). As for unmap, if try_to_unmap_one() return true, it means the pte has been teared down and mapcount dec. Apparently the current code

        if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) &&
            is_zone_device_page(page) && !is_device_private_page(page))
               return true;

conflicts with this logic.

Further more, as for zone_device, migration can only happen on is_device_private_page(page). For other zone_device, memmap_init_zone_device() raises an extra _refcount on all zone pages. This extra _refcount will block migration. So in try_to_unmap_one(), it can just return false for other zone device.

The reason why original code happen to work one !private zone_device. -1. if page mapped, then try_to_unmap_one()->page_remove_rmap() is skipped, and finally try_to_unmap(){ return !page_mapcount(page) ? true : false;} will return false.
-2. if page not mapped, the extra _refcount will prevent the migration.

Signed-off-by Pingfan Liu <[email protected]>

Cc Jérôme Glisse <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Ralph Campbell <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Dan Williams <[email protected]>

Cc John Hubbard <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Cc Aneesh Kumar <[email protected]>

Cc Andrew Morton <[email protected]>

To: [email protected]
---
v1 -> v2: improve commit log and note in code

 mm/rmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/rmap.c +6 -2

index b838647..723af4f 100644
@@ -1358,6 +1358,10 @@ void page_remove_rmap(struct page *page, bool compound)
 
 /*
  * @arg: enum ttu_flags will be passed to this argument
+ *
+ * For munlock, return true if @page is not mlocked by @vma without killing pte
+ * For unmap, return true after tearing down pte.
+ * For both cases, return false if rmap_walk should be stopped.
  */
 static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 		     unsigned long address, void *arg)

@@ -1380,7 +1384,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 +1491,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

Michal Hocko

2020-04-01 15:58 UTC #13 lore raw

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

Quoted 5 lines · Pingfan Liu

This patch is a pure code refinement without any functional changes.
try_to_unmap_one() is shared by try_to_unmap() and try_to_munlock(). As for unmap, if try_to_unmap_one() return true, it means the pte has been teared down and mapcount dec.

I haven't really checked the full history of the rmap walk but this is certainly not the currently implemented semantic of this callback. Returing true only tells the caller that it should continue with other VMAs which map the given page. It doesn't really mean that the pte has been torn down. The munlock case is a nice example of how that is use properly while migration path for device pages how it is used incorrectly because it doesn't make any sense to walk other VMAs because is_device_private_page is a property of the page not the VMA. And that is the only reason to drop that.

Quoted 14 lines

Apparently the current code
        if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) &&
            is_zone_device_page(page) && !is_device_private_page(page))
               return true;
conflicts with this logic.
Further more, as for zone_device, migration can only happen on is_device_private_page(page). For other zone_device, memmap_init_zone_device() raises an extra _refcount on all zone pages. This extra _refcount will block migration. So in try_to_unmap_one(), it can just return false for other zone device.

Quoted 5 lines

The reason why original code happen to work one !private zone_device. -1. if page mapped, then try_to_unmap_one()->page_remove_rmap() is skipped, and finally try_to_unmap(){ return !page_mapcount(page) ? true : false;} will return false.
-2. if page not mapped, the extra _refcount will prevent the migration.

And this sounds like a separate change to me worth a patch on its own.

Quoted 51 lines

Signed-off-by Pingfan Liu <[email protected]>

Cc Jérôme Glisse <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Ralph Campbell <[email protected]>

Cc Balbir Singh <[email protected]>

Cc Dan Williams <[email protected]>

Cc John Hubbard <[email protected]>

Cc Kirill A. Shutemov <[email protected]>

Cc Aneesh Kumar <[email protected]>

Cc Andrew Morton <[email protected]>

To: [email protected]
---
v1 -> v2: improve commit log and note in code

 mm/rmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

mm/rmap.c +6 -3

index b838647..723af4f 100644
@@ -1358,6 +1358,10 @@ void page_remove_rmap(struct page *page, bool compound)
 
 /*
  * @arg: enum ttu_flags will be passed to this argument
+ *
+ * For munlock, return true if @page is not mlocked by @vma without killing pte
+ * For unmap, return true after tearing down pte.
+ * For both cases, return false if rmap_walk should be stopped.
  */
 static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
 		     unsigned long address, void *arg)

@@ -1380,7 +1384,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 +1491,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

Signature · 2 lines

Michal Hocko
SUSE Labs

Pingfan Liu

author

2020-04-02 07:40 UTC #14 lore raw

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

Quoted 8 lines · Michal Hocko

Quoted 5 lines · Pingfan Liu
This patch is a pure code refinement without any functional changes.
try_to_unmap_one() is shared by try_to_unmap() and try_to_munlock(). As for unmap, if try_to_unmap_one() return true, it means the pte has been teared down and mapcount dec.

Quoted 5 lines

I haven't really checked the full history of the rmap walk but this is certainly not the currently implemented semantic of this callback. Returing true only tells the caller that it should continue with other VMAs which map the given page. It doesn't really mean that the pte has been torn down. The munlock case is a nice example of how that is use

I did not paste the whole story in commit log, but note them in the code. For munlock, we only care about the page will be put to correct lru. But as commit "As for unmap", it should tear down pte, otherwise the page may be accessed by and old mapping. Also here omit an assumption, for !private device page, e.g. fs-dax, there is no need to mlock them.

Quoted 13 lines

properly while migration path for device pages how it is used incorrectly because it doesn't make any sense to walk other VMAs because is_device_private_page is a property of the page not the VMA. And that is the only reason to drop that.
Quoted 8 lines
Apparently the current code
        if (IS_ENABLED(CONFIG_MIGRATION) && (flags & TTU_MIGRATION) &&
            is_zone_device_page(page) && !is_device_private_page(page))
               return true;
conflicts with this logic.

[...]

Quoted 4 lines

Quoted 4 lines
/*
* @arg: enum ttu_flags will be passed to this argument
+ *
+ * For munlock, return true if @page is not mlocked by @vma without killing pte

Here is the note for munlock case

Thanks,
Pingfan

Quoted 29 lines

Quoted 24 lines

+ * For unmap, return true after tearing down pte.
+ * For both cases, return false if rmap_walk should be stopped. */
static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
unsigned long address, void *arg)

mm/rmap.c +2 -3

@@ -1380,7 +1384,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 +1491,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;
 
--

Quoted 1 line

2.7.5

--
Michal Hocko
SUSE Labs