Re: [PATCH V3 2/5] mm: update ptep_modify_prot_commit to take old pte value as arg

2018-12-04 Thread Christophe LEROY 




Le 05/12/2018 à 05:06, Aneesh Kumar K.V a écrit :

On 12/5/18 9:32 AM, Christophe LEROY wrote:



Le 05/12/2018 à 04:09, Aneesh Kumar K.V a écrit :
Architectures like ppc64 requires to do a conditional tlb flush based 
on the old

and new value of pte. Enable that by passing old pte value as the arg.

Signed-off-by: Aneesh Kumar K.V 
---
  arch/s390/include/asm/pgtable.h | 3 ++-
  arch/s390/mm/pgtable.c  | 2 +-
  arch/x86/include/asm/paravirt.h | 2 +-
  fs/proc/task_mmu.c  | 8 +---
  include/asm-generic/pgtable.h   | 2 +-
  mm/memory.c | 8 
  mm/mprotect.c   | 6 +++---
  7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/s390/include/asm/pgtable.h 
b/arch/s390/include/asm/pgtable.h

index 5d730199e37b..76dc344edb8c 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1070,7 +1070,8 @@ static inline pte_t ptep_get_and_clear(struct 
mm_struct *mm,

  #define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
  pte_t ptep_modify_prot_start(struct vm_area_struct *, unsigned 
long, pte_t *);
-void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long, 
pte_t *, pte_t);

+void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long,
+ pte_t *, pte_t, pte_t);
  #define __HAVE_ARCH_PTEP_CLEAR_FLUSH
  static inline pte_t ptep_clear_flush(struct vm_area_struct *vma,
diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
index 29c0a21cd34a..b283b92722cc 100644
--- a/arch/s390/mm/pgtable.c
+++ b/arch/s390/mm/pgtable.c
@@ -322,7 +322,7 @@ pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma, unsigned long addr,

  EXPORT_SYMBOL(ptep_modify_prot_start);
  void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned 
long addr,

- pte_t *ptep, pte_t pte)
+ pte_t *ptep, pte_t old_pte, pte_t pte)
  {
  pgste_t pgste;
  struct mm_struct *mm = vma->vm_mm;
diff --git a/arch/x86/include/asm/paravirt.h 
b/arch/x86/include/asm/paravirt.h

index 1154f154025d..0d75a4f60500 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -429,7 +429,7 @@ static inline pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma, unsigned

  }
  static inline void ptep_modify_prot_commit(struct vm_area_struct 
*vma, unsigned long addr,

-   pte_t *ptep, pte_t pte)
+   pte_t *ptep, pte_t old_pte, pte_t pte)
  {
  struct mm_struct *mm = vma->vm_mm;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9952d7185170..8d62891d38a8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -940,10 +940,12 @@ static inline void clear_soft_dirty(struct 
vm_area_struct *vma,

  pte_t ptent = *pte;
  if (pte_present(ptent)) {
-    ptent = ptep_modify_prot_start(vma, addr, pte);
-    ptent = pte_wrprotect(ptent);
+    pte_t old_pte;
+
+    old_pte = ptep_modify_prot_start(vma, addr, pte);
+    ptent = pte_wrprotect(old_pte);


This change doesn't seem to fit with the commit description. Why write 
protecting in addition to clearing dirty ?





The hunk above use a new variable old_pte. There is no functional change 
in that hunk.




Oops, sorry, I misread the patch, don't know why.

Christophe

Re: [PATCH V3 2/5] mm: update ptep_modify_prot_commit to take old pte value as arg

2018-12-04 Thread Aneesh Kumar K.V 

On 12/5/18 9:32 AM, Christophe LEROY wrote:



Le 05/12/2018 à 04:09, Aneesh Kumar K.V a écrit :
Architectures like ppc64 requires to do a conditional tlb flush based 
on the old

and new value of pte. Enable that by passing old pte value as the arg.

Signed-off-by: Aneesh Kumar K.V 
---
  arch/s390/include/asm/pgtable.h | 3 ++-
  arch/s390/mm/pgtable.c  | 2 +-
  arch/x86/include/asm/paravirt.h | 2 +-
  fs/proc/task_mmu.c  | 8 +---
  include/asm-generic/pgtable.h   | 2 +-
  mm/memory.c | 8 
  mm/mprotect.c   | 6 +++---
  7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/s390/include/asm/pgtable.h 
b/arch/s390/include/asm/pgtable.h

index 5d730199e37b..76dc344edb8c 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1070,7 +1070,8 @@ static inline pte_t ptep_get_and_clear(struct 
mm_struct *mm,

  #define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
  pte_t ptep_modify_prot_start(struct vm_area_struct *, unsigned long, 
pte_t *);
-void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long, 
pte_t *, pte_t);

+void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long,
+ pte_t *, pte_t, pte_t);
  #define __HAVE_ARCH_PTEP_CLEAR_FLUSH
  static inline pte_t ptep_clear_flush(struct vm_area_struct *vma,
diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
index 29c0a21cd34a..b283b92722cc 100644
--- a/arch/s390/mm/pgtable.c
+++ b/arch/s390/mm/pgtable.c
@@ -322,7 +322,7 @@ pte_t ptep_modify_prot_start(struct vm_area_struct 
*vma, unsigned long addr,

  EXPORT_SYMBOL(ptep_modify_prot_start);
  void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned 
long addr,

- pte_t *ptep, pte_t pte)
+ pte_t *ptep, pte_t old_pte, pte_t pte)
  {
  pgste_t pgste;
  struct mm_struct *mm = vma->vm_mm;
diff --git a/arch/x86/include/asm/paravirt.h 
b/arch/x86/include/asm/paravirt.h

index 1154f154025d..0d75a4f60500 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -429,7 +429,7 @@ static inline pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma, unsigned

  }
  static inline void ptep_modify_prot_commit(struct vm_area_struct 
*vma, unsigned long addr,

-   pte_t *ptep, pte_t pte)
+   pte_t *ptep, pte_t old_pte, pte_t pte)
  {
  struct mm_struct *mm = vma->vm_mm;
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9952d7185170..8d62891d38a8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -940,10 +940,12 @@ static inline void clear_soft_dirty(struct 
vm_area_struct *vma,

  pte_t ptent = *pte;
  if (pte_present(ptent)) {
-    ptent = ptep_modify_prot_start(vma, addr, pte);
-    ptent = pte_wrprotect(ptent);
+    pte_t old_pte;
+
+    old_pte = ptep_modify_prot_start(vma, addr, pte);
+    ptent = pte_wrprotect(old_pte);


This change doesn't seem to fit with the commit description. Why write 
protecting in addition to clearing dirty ?





The hunk above use a new variable old_pte. There is no functional change 
in that hunk.


-aneesh

Re: [PATCH V3 2/5] mm: update ptep_modify_prot_commit to take old pte value as arg

2018-12-04 Thread Christophe LEROY 




Le 05/12/2018 à 04:09, Aneesh Kumar K.V a écrit :

Architectures like ppc64 requires to do a conditional tlb flush based on the old
and new value of pte. Enable that by passing old pte value as the arg.

Signed-off-by: Aneesh Kumar K.V 
---
  arch/s390/include/asm/pgtable.h | 3 ++-
  arch/s390/mm/pgtable.c  | 2 +-
  arch/x86/include/asm/paravirt.h | 2 +-
  fs/proc/task_mmu.c  | 8 +---
  include/asm-generic/pgtable.h   | 2 +-
  mm/memory.c | 8 
  mm/mprotect.c   | 6 +++---
  7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 5d730199e37b..76dc344edb8c 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1070,7 +1070,8 @@ static inline pte_t ptep_get_and_clear(struct mm_struct 
*mm,
  
  #define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION

  pte_t ptep_modify_prot_start(struct vm_area_struct *, unsigned long, pte_t *);
-void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long, pte_t *, 
pte_t);
+void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long,
+pte_t *, pte_t, pte_t);
  
  #define __HAVE_ARCH_PTEP_CLEAR_FLUSH

  static inline pte_t ptep_clear_flush(struct vm_area_struct *vma,
diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
index 29c0a21cd34a..b283b92722cc 100644
--- a/arch/s390/mm/pgtable.c
+++ b/arch/s390/mm/pgtable.c
@@ -322,7 +322,7 @@ pte_t ptep_modify_prot_start(struct vm_area_struct *vma, 
unsigned long addr,
  EXPORT_SYMBOL(ptep_modify_prot_start);
  
  void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr,

-pte_t *ptep, pte_t pte)
+pte_t *ptep, pte_t old_pte, pte_t pte)
  {
pgste_t pgste;
struct mm_struct *mm = vma->vm_mm;
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 1154f154025d..0d75a4f60500 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -429,7 +429,7 @@ static inline pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma, unsigned
  }
  
  static inline void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr,

-  pte_t *ptep, pte_t pte)
+  pte_t *ptep, pte_t old_pte, pte_t 
pte)
  {
struct mm_struct *mm = vma->vm_mm;
  
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c

index 9952d7185170..8d62891d38a8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -940,10 +940,12 @@ static inline void clear_soft_dirty(struct vm_area_struct 
*vma,
pte_t ptent = *pte;
  
  	if (pte_present(ptent)) {

-   ptent = ptep_modify_prot_start(vma, addr, pte);
-   ptent = pte_wrprotect(ptent);
+   pte_t old_pte;
+
+   old_pte = ptep_modify_prot_start(vma, addr, pte);
+   ptent = pte_wrprotect(old_pte);


This change doesn't seem to fit with the commit description. Why write 
protecting in addition to clearing dirty ?


Christophe


ptent = pte_clear_soft_dirty(ptent);
-   ptep_modify_prot_commit(vma, addr, pte, ptent);
+   ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
} else if (is_swap_pte(ptent)) {
ptent = pte_swp_clear_soft_dirty(ptent);
set_pte_at(vma->vm_mm, addr, pte, ptent);
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index c9897dcc46c4..37039e918f17 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -619,7 +619,7 @@ static inline pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma,
   */
  static inline void ptep_modify_prot_commit(struct vm_area_struct *vma,
   unsigned long addr,
-  pte_t *ptep, pte_t pte)
+  pte_t *ptep, pte_t old_pte, pte_t 
pte)
  {
__ptep_modify_prot_commit(vma->vm_mm, addr, ptep, pte);
  }
diff --git a/mm/memory.c b/mm/memory.c
index d36b0eaa7862..4f3ddaedc764 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3568,7 +3568,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
int last_cpupid;
int target_nid;
bool migrated = false;
-   pte_t pte;
+   pte_t pte, old_pte;
bool was_writable = pte_savedwrite(vmf->orig_pte);
int flags = 0;
  
@@ -3588,12 +3588,12 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)

 * Make it present again, Depending on how arch implementes non
 * accessible ptes, some can allow access by kernel mode.
 */
-   pte = ptep_modify_prot_start(vma, vmf->address, vmf->pte);
-   pte = pte_modify(pte, vma->vm_page_prot);
+   old_pte = ptep_modify_prot_start(vma, vmf->address, vmf->pte);
+

[PATCH V3 2/5] mm: update ptep_modify_prot_commit to take old pte value as arg

2018-12-04 Thread Aneesh Kumar K.V 
Architectures like ppc64 requires to do a conditional tlb flush based on the old
and new value of pte. Enable that by passing old pte value as the arg.

Signed-off-by: Aneesh Kumar K.V 
---
 arch/s390/include/asm/pgtable.h | 3 ++-
 arch/s390/mm/pgtable.c  | 2 +-
 arch/x86/include/asm/paravirt.h | 2 +-
 fs/proc/task_mmu.c  | 8 +---
 include/asm-generic/pgtable.h   | 2 +-
 mm/memory.c | 8 
 mm/mprotect.c   | 6 +++---
 7 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 5d730199e37b..76dc344edb8c 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1070,7 +1070,8 @@ static inline pte_t ptep_get_and_clear(struct mm_struct 
*mm,
 
 #define __HAVE_ARCH_PTEP_MODIFY_PROT_TRANSACTION
 pte_t ptep_modify_prot_start(struct vm_area_struct *, unsigned long, pte_t *);
-void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long, pte_t *, 
pte_t);
+void ptep_modify_prot_commit(struct vm_area_struct *, unsigned long,
+pte_t *, pte_t, pte_t);
 
 #define __HAVE_ARCH_PTEP_CLEAR_FLUSH
 static inline pte_t ptep_clear_flush(struct vm_area_struct *vma,
diff --git a/arch/s390/mm/pgtable.c b/arch/s390/mm/pgtable.c
index 29c0a21cd34a..b283b92722cc 100644
--- a/arch/s390/mm/pgtable.c
+++ b/arch/s390/mm/pgtable.c
@@ -322,7 +322,7 @@ pte_t ptep_modify_prot_start(struct vm_area_struct *vma, 
unsigned long addr,
 EXPORT_SYMBOL(ptep_modify_prot_start);
 
 void ptep_modify_prot_commit(struct vm_area_struct *vma, unsigned long addr,
-pte_t *ptep, pte_t pte)
+pte_t *ptep, pte_t old_pte, pte_t pte)
 {
pgste_t pgste;
struct mm_struct *mm = vma->vm_mm;
diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 1154f154025d..0d75a4f60500 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -429,7 +429,7 @@ static inline pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma, unsigned
 }
 
 static inline void ptep_modify_prot_commit(struct vm_area_struct *vma, 
unsigned long addr,
-  pte_t *ptep, pte_t pte)
+  pte_t *ptep, pte_t old_pte, pte_t 
pte)
 {
struct mm_struct *mm = vma->vm_mm;
 
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9952d7185170..8d62891d38a8 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -940,10 +940,12 @@ static inline void clear_soft_dirty(struct vm_area_struct 
*vma,
pte_t ptent = *pte;
 
if (pte_present(ptent)) {
-   ptent = ptep_modify_prot_start(vma, addr, pte);
-   ptent = pte_wrprotect(ptent);
+   pte_t old_pte;
+
+   old_pte = ptep_modify_prot_start(vma, addr, pte);
+   ptent = pte_wrprotect(old_pte);
ptent = pte_clear_soft_dirty(ptent);
-   ptep_modify_prot_commit(vma, addr, pte, ptent);
+   ptep_modify_prot_commit(vma, addr, pte, old_pte, ptent);
} else if (is_swap_pte(ptent)) {
ptent = pte_swp_clear_soft_dirty(ptent);
set_pte_at(vma->vm_mm, addr, pte, ptent);
diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
index c9897dcc46c4..37039e918f17 100644
--- a/include/asm-generic/pgtable.h
+++ b/include/asm-generic/pgtable.h
@@ -619,7 +619,7 @@ static inline pte_t ptep_modify_prot_start(struct 
vm_area_struct *vma,
  */
 static inline void ptep_modify_prot_commit(struct vm_area_struct *vma,
   unsigned long addr,
-  pte_t *ptep, pte_t pte)
+  pte_t *ptep, pte_t old_pte, pte_t 
pte)
 {
__ptep_modify_prot_commit(vma->vm_mm, addr, ptep, pte);
 }
diff --git a/mm/memory.c b/mm/memory.c
index d36b0eaa7862..4f3ddaedc764 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3568,7 +3568,7 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
int last_cpupid;
int target_nid;
bool migrated = false;
-   pte_t pte;
+   pte_t pte, old_pte;
bool was_writable = pte_savedwrite(vmf->orig_pte);
int flags = 0;
 
@@ -3588,12 +3588,12 @@ static vm_fault_t do_numa_page(struct vm_fault *vmf)
 * Make it present again, Depending on how arch implementes non
 * accessible ptes, some can allow access by kernel mode.
 */
-   pte = ptep_modify_prot_start(vma, vmf->address, vmf->pte);
-   pte = pte_modify(pte, vma->vm_page_prot);
+   old_pte = ptep_modify_prot_start(vma, vmf->address, vmf->pte);
+   pte = pte_modify(old_pte, vma->vm_page_prot);
pte = pte_mkyoung(pte);
if (was_writable)
pte = pte_mkwrite(pte);
-   ptep_modify_prot_commit(vma, vmf->address, vmf->pte, pte);
+