Re: [PATCH] x86/mm: synchronize pgd in vmemmap_free()

[John Hubbard]

Hi Jerome,

On 05/19/2017 11:01 AM, Jérôme Glisse wrote:

When we free kernel virtual map we should synchronize p4d/pud for
all the pgds to avoid any stall entry in non canonical pgd.


"any stale entry in the non-canonical pgd", is what I think you meant to type 
there.

Also, it would be nice to clarify that commit description a bit: I'm not sure what is meant here by 
a "non-canonical pgd".


Also, it seems like the reshuffling of the internals of sync_global_pgds() deserves at least some 
mention here. More below.




Signed-off-by: Jérôme Glisse 
Cc: Kirill A. Shutemov 
Cc: Andrew Morton 
Cc: Ingo Molnar 
Cc: Michal Hocko 
Cc: Mel Gorman 
---
  arch/x86/mm/init_64.c | 17 ++---
  1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ff95fe8..df753f8 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -108,8 +108,6 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
BUILD_BUG_ON(pgd_none(*pgd_ref));
p4d_ref = p4d_offset(pgd_ref, address);
  
-		if (p4d_none(*p4d_ref))

-   continue;
  
  		spin_lock(_lock);

list_for_each_entry(page, _list, lru) {
@@ -123,12 +121,16 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
pgt_lock = _page_get_mm(page)->page_table_lock;
spin_lock(pgt_lock);
  
-			if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))

-   BUG_ON(p4d_page_vaddr(*p4d)
-  != p4d_page_vaddr(*p4d_ref));
-
-   if (p4d_none(*p4d))
+   if (p4d_none(*p4d_ref)) {
set_p4d(p4d, *p4d_ref);


Is the intention really to set p4d to a zeroed *p4d_ref, or is that a mistake?


+   } else {
+   if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))


I think the code needs to be somewhat restructured, but as it stands, the above !p4d_none(*p4d_ref) 
will always be true, because first part of the if/else checked for the opposite case: 
p4d_none(*p4d_ref).  This is a side effect of moving that block of code.



+   BUG_ON(p4d_page_vaddr(*p4d)
+  != p4d_page_vaddr(*p4d_ref));
+
+   if (p4d_none(*p4d))
+   set_p4d(p4d, *p4d_ref);
+   }
  
  			spin_unlock(pgt_lock);

}
@@ -1024,6 +1026,7 @@ remove_pagetable(unsigned long start, unsigned long end, 
bool direct)
  void __ref vmemmap_free(unsigned long start, unsigned long end)
  {
remove_pagetable(start, end, false);
+   sync_global_pgds(start, end - 1);


This does fix the HMM crash that I was seeing in hmm-next.

thanks,
John Hubbard
NVIDIA


  }
  
  #ifdef CONFIG_MEMORY_HOTREMOVE

--
2.4.11

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majord...@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

Re: [PATCH] x86/mm: synchronize pgd in vmemmap_free()

[John Hubbard]

Hi Jerome,

On 05/19/2017 11:01 AM, Jérôme Glisse wrote:

When we free kernel virtual map we should synchronize p4d/pud for
all the pgds to avoid any stall entry in non canonical pgd.


"any stale entry in the non-canonical pgd", is what I think you meant to type 
there.

Also, it would be nice to clarify that commit description a bit: I'm not sure what is meant here by 
a "non-canonical pgd".


Also, it seems like the reshuffling of the internals of sync_global_pgds() deserves at least some 
mention here. More below.




Signed-off-by: Jérôme Glisse 
Cc: Kirill A. Shutemov 
Cc: Andrew Morton 
Cc: Ingo Molnar 
Cc: Michal Hocko 
Cc: Mel Gorman 
---
  arch/x86/mm/init_64.c | 17 ++---
  1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ff95fe8..df753f8 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -108,8 +108,6 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
BUILD_BUG_ON(pgd_none(*pgd_ref));
p4d_ref = p4d_offset(pgd_ref, address);
  
-		if (p4d_none(*p4d_ref))

-   continue;
  
  		spin_lock(_lock);

list_for_each_entry(page, _list, lru) {
@@ -123,12 +121,16 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
pgt_lock = _page_get_mm(page)->page_table_lock;
spin_lock(pgt_lock);
  
-			if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))

-   BUG_ON(p4d_page_vaddr(*p4d)
-  != p4d_page_vaddr(*p4d_ref));
-
-   if (p4d_none(*p4d))
+   if (p4d_none(*p4d_ref)) {
set_p4d(p4d, *p4d_ref);


Is the intention really to set p4d to a zeroed *p4d_ref, or is that a mistake?


+   } else {
+   if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))


I think the code needs to be somewhat restructured, but as it stands, the above !p4d_none(*p4d_ref) 
will always be true, because first part of the if/else checked for the opposite case: 
p4d_none(*p4d_ref).  This is a side effect of moving that block of code.



+   BUG_ON(p4d_page_vaddr(*p4d)
+  != p4d_page_vaddr(*p4d_ref));
+
+   if (p4d_none(*p4d))
+   set_p4d(p4d, *p4d_ref);
+   }
  
  			spin_unlock(pgt_lock);

}
@@ -1024,6 +1026,7 @@ remove_pagetable(unsigned long start, unsigned long end, 
bool direct)
  void __ref vmemmap_free(unsigned long start, unsigned long end)
  {
remove_pagetable(start, end, false);
+   sync_global_pgds(start, end - 1);


This does fix the HMM crash that I was seeing in hmm-next.

thanks,
John Hubbard
NVIDIA


  }
  
  #ifdef CONFIG_MEMORY_HOTREMOVE

--
2.4.11

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majord...@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

[PATCH] x86/mm: synchronize pgd in vmemmap_free()

[Jérôme Glisse]

When we free kernel virtual map we should synchronize p4d/pud for
all the pgds to avoid any stall entry in non canonical pgd.

Signed-off-by: Jérôme Glisse 
Cc: Kirill A. Shutemov 
Cc: Andrew Morton 
Cc: Ingo Molnar 
Cc: Michal Hocko 
Cc: Mel Gorman 
---
 arch/x86/mm/init_64.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ff95fe8..df753f8 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -108,8 +108,6 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
BUILD_BUG_ON(pgd_none(*pgd_ref));
p4d_ref = p4d_offset(pgd_ref, address);
 
-   if (p4d_none(*p4d_ref))
-   continue;
 
spin_lock(_lock);
list_for_each_entry(page, _list, lru) {
@@ -123,12 +121,16 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
pgt_lock = _page_get_mm(page)->page_table_lock;
spin_lock(pgt_lock);
 
-   if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
-   BUG_ON(p4d_page_vaddr(*p4d)
-  != p4d_page_vaddr(*p4d_ref));
-
-   if (p4d_none(*p4d))
+   if (p4d_none(*p4d_ref)) {
set_p4d(p4d, *p4d_ref);
+   } else {
+   if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
+   BUG_ON(p4d_page_vaddr(*p4d)
+  != p4d_page_vaddr(*p4d_ref));
+
+   if (p4d_none(*p4d))
+   set_p4d(p4d, *p4d_ref);
+   }
 
spin_unlock(pgt_lock);
}
@@ -1024,6 +1026,7 @@ remove_pagetable(unsigned long start, unsigned long end, 
bool direct)
 void __ref vmemmap_free(unsigned long start, unsigned long end)
 {
remove_pagetable(start, end, false);
+   sync_global_pgds(start, end - 1);
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
-- 
2.4.11

[PATCH] x86/mm: synchronize pgd in vmemmap_free()

[Jérôme Glisse]

When we free kernel virtual map we should synchronize p4d/pud for
all the pgds to avoid any stall entry in non canonical pgd.

Signed-off-by: Jérôme Glisse 
Cc: Kirill A. Shutemov 
Cc: Andrew Morton 
Cc: Ingo Molnar 
Cc: Michal Hocko 
Cc: Mel Gorman 
---
 arch/x86/mm/init_64.c | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index ff95fe8..df753f8 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -108,8 +108,6 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
BUILD_BUG_ON(pgd_none(*pgd_ref));
p4d_ref = p4d_offset(pgd_ref, address);
 
-   if (p4d_none(*p4d_ref))
-   continue;
 
spin_lock(_lock);
list_for_each_entry(page, _list, lru) {
@@ -123,12 +121,16 @@ void sync_global_pgds(unsigned long start, unsigned long 
end)
pgt_lock = _page_get_mm(page)->page_table_lock;
spin_lock(pgt_lock);
 
-   if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
-   BUG_ON(p4d_page_vaddr(*p4d)
-  != p4d_page_vaddr(*p4d_ref));
-
-   if (p4d_none(*p4d))
+   if (p4d_none(*p4d_ref)) {
set_p4d(p4d, *p4d_ref);
+   } else {
+   if (!p4d_none(*p4d_ref) && !p4d_none(*p4d))
+   BUG_ON(p4d_page_vaddr(*p4d)
+  != p4d_page_vaddr(*p4d_ref));
+
+   if (p4d_none(*p4d))
+   set_p4d(p4d, *p4d_ref);
+   }
 
spin_unlock(pgt_lock);
}
@@ -1024,6 +1026,7 @@ remove_pagetable(unsigned long start, unsigned long end, 
bool direct)
 void __ref vmemmap_free(unsigned long start, unsigned long end)
 {
remove_pagetable(start, end, false);
+   sync_global_pgds(start, end - 1);
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
-- 
2.4.11