Skip to contentSkip to conversation

mm: Remove src/dst mm parameter in copy_page_range()

PATCH Superseded Peter Xu 2 participants 5 messages Sep 30, 2020 – Oct 2, 2020

Subsystems (4)
Memory management MM core Scheduler exec and binfmt

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Peter Xu

author
#0 lore raw

PATCH 1/1 mm: Remove src/dst mm parameter in copy_page_range()

Both of the mm pointers are not needed after commit 7a4830c380f3 ("mm/fork:
Pass new vma pointer into copy_page_range()").

Reported-by Kirill A. Shutemov <[email protected]>
Signed-off-by Peter Xu <[email protected]>
---
 include/linux/mm.h |  3 +--
 kernel/fork.c      |  2 +-
 mm/memory.c        | 43 ++++++++++++++++++++++---------------------
 3 files changed, 24 insertions(+), 24 deletions(-)
include/linux/mm.h +1 -2
index 16b799a0522c..8a0ec8dce5f6 100644
@@ -1645,8 +1645,7 @@ struct mmu_notifier_range;
 
 void free_pgd_range(struct mmu_gather *tlb, unsigned long addr,
 		unsigned long end, unsigned long floor, unsigned long ceiling);
-int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
-		    struct vm_area_struct *vma, struct vm_area_struct *new);
+int copy_page_range(struct vm_area_struct *vma, struct vm_area_struct *new);
 int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 		   struct mmu_notifier_range *range,
 		   pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp);
kernel/fork.c +1 -1
index da8d360fb032..5f42d4afe0ae 100644
@@ -589,7 +589,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
 
 		mm->map_count++;
 		if (!(tmp->vm_flags & VM_WIPEONFORK))
-			retval = copy_page_range(mm, oldmm, mpnt, tmp);
+			retval = copy_page_range(mpnt, tmp);
 
 		if (tmp->vm_ops && tmp->vm_ops->open)
 			tmp->vm_ops->open(tmp);
mm/memory.c +22 -21

107 lines not on this page · raw message

Signature · 1 line
2.26.2
#1 lore raw
Quoted 43 lines · Peter Xu

Both of the mm pointers are not needed after commit 7a4830c380f3 ("mm/fork:
Pass new vma pointer into copy_page_range()").

Reported-by Kirill A. Shutemov <[email protected]>
Signed-off-by Peter Xu <[email protected]>
---
 include/linux/mm.h |  3 +--
 kernel/fork.c      |  2 +-
 mm/memory.c        | 43 ++++++++++++++++++++++---------------------
 3 files changed, 24 insertions(+), 24 deletions(-)
include/linux/mm.h +1 -2
index 16b799a0522c..8a0ec8dce5f6 100644
@@ -1645,8 +1645,7 @@ struct mmu_notifier_range;
 
 void free_pgd_range(struct mmu_gather *tlb, unsigned long addr,
 		unsigned long end, unsigned long floor, unsigned long ceiling);
-int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
-		    struct vm_area_struct *vma, struct vm_area_struct *new);
+int copy_page_range(struct vm_area_struct *vma, struct vm_area_struct *new);
 int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
 		   struct mmu_notifier_range *range,
 		   pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp);
kernel/fork.c +1 -1
index da8d360fb032..5f42d4afe0ae 100644
@@ -589,7 +589,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
 
 		mm->map_count++;
 		if (!(tmp->vm_flags & VM_WIPEONFORK))
-			retval = copy_page_range(mm, oldmm, mpnt, tmp);
+			retval = copy_page_range(mpnt, tmp);
 
 		if (tmp->vm_ops && tmp->vm_ops->open)
 			tmp->vm_ops->open(tmp);
mm/memory.c +0 -0
index fcfc4ca36eba..251bb5082f4e 100644

mm/memory.c page_copy_prealloc(struct mm_struct *src_mm, struct vm_area_struct *vma, · line 962

Quoted 9 lines
mm/memory.c +2 -3
@@ -957,11 +957,12 @@ page_copy_prealloc(struct mm_struct *src_mm, struct vm_area_struct *vma,
 	return new_page;
 }
 
-static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		   pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
-		   struct vm_area_struct *new,
+static int copy_pte_range(pmd_t *dst_pmd, pmd_t *src_pmd,
+		   struct vm_area_struct *vma, struct vm_area_struct *new,
 		   unsigned long addr, unsigned long end)

I link this, my only minor quibble is the mixing of dst/src and new language, and then reversing the order in each place. Would read better to be consistent:

copy_pte_range(dst_vma, dst_pmd, src_vma, src_pmd, addr, end)

Regards,
Jason

author
#2 lore raw
Quoted 11 lines · Jason Gunthorpe

mm/memory.c page_copy_prealloc(struct mm_struct *src_mm, struct vm_area_struct *vma, · line 962

Quoted 6 lines
mm/memory.c +2 -3
-static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		   pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
-		   struct vm_area_struct *new,
+static int copy_pte_range(pmd_t *dst_pmd, pmd_t *src_pmd,
+		   struct vm_area_struct *vma, struct vm_area_struct *new,
 		   unsigned long addr, unsigned long end)

I link this, my only minor quibble is the mixing of dst/src and new language, and then reversing the order in each place. Would read better to be consistent:

Quoted 1 line

copy_pte_range(dst_vma, dst_pmd, src_vma, src_pmd, addr, end)

I have no strong opinion on the ordering, but I agree the names are clearer. Considering normally we put the same type of parameters to be together, how about:

copy_pte_range(dst_vma, src_vma, dst_pmd, src_pmd, addr, end)

Thanks,

Signature · 1 line
Peter Xu
#3 lore raw
Quoted 12 lines · Peter Xu
Quoted 11 lines · Jason Gunthorpe

mm/memory.c page_copy_prealloc(struct mm_struct *src_mm, struct vm_area_struct *vma, · line 962

Quoted 6 lines
mm/memory.c +2 -3
-static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		   pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
-		   struct vm_area_struct *new,
+static int copy_pte_range(pmd_t *dst_pmd, pmd_t *src_pmd,
+		   struct vm_area_struct *vma, struct vm_area_struct *new,
 		   unsigned long addr, unsigned long end)

I link this, my only minor quibble is the mixing of dst/src and new language, and then reversing the order in each place. Would read better to be consistent:

Quoted 7 lines
Quoted 1 line

copy_pte_range(dst_vma, dst_pmd, src_vma, src_pmd, addr, end)

I have no strong opinion on the ordering, but I agree the names are clearer. Considering normally we put the same type of parameters to be together, how about:

copy_pte_range(dst_vma, src_vma, dst_pmd, src_pmd, addr, end)

I was looking at the order of (dst_pmd, src_pmd, src_vma, dest_vma)

Whichever, just have some logic to it :)

Jason

author
#4 lore raw
Quoted 19 lines · Jason Gunthorpe
Quoted 18 lines · Peter Xu
Quoted 11 lines · Jason Gunthorpe

mm/memory.c page_copy_prealloc(struct mm_struct *src_mm, struct vm_area_struct *vma, · line 962

Quoted 6 lines
mm/memory.c +2 -3
-static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		   pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
-		   struct vm_area_struct *new,
+static int copy_pte_range(pmd_t *dst_pmd, pmd_t *src_pmd,
+		   struct vm_area_struct *vma, struct vm_area_struct *new,
 		   unsigned long addr, unsigned long end)

I link this, my only minor quibble is the mixing of dst/src and new language, and then reversing the order in each place. Would read better to be consistent:

Quoted 1 line

copy_pte_range(dst_vma, dst_pmd, src_vma, src_pmd, addr, end)

I have no strong opinion on the ordering, but I agree the names are clearer. Considering normally we put the same type of parameters to be together, how about:

Quoted 5 lines
Quoted 1 line

copy_pte_range(dst_vma, src_vma, dst_pmd, src_pmd, addr, end)

I was looking at the order of (dst_pmd, src_pmd, src_vma, dest_vma)

Whichever, just have some logic to it :)

Oh, sure. :)

Signature · 1 line
Peter Xu