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

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

[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 <kirill@shutemov.name>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/linux/mm.h |  3 +--
 kernel/fork.c      |  2 +-
 mm/memory.c        | 43 ++++++++++++++++++++++---------------------
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 16b799a0522c..8a0ec8dce5f6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -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);
diff --git a/kernel/fork.c b/kernel/fork.c
index da8d360fb032..5f42d4afe0ae 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -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);
diff --git a/mm/memory.c b/mm/memory.c
index fcfc4ca36eba..251bb5082f4e 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -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)
 {
+	struct mm_struct *dst_mm = new->vm_mm;
+	struct mm_struct *src_mm = vma->vm_mm;
 	pte_t *orig_src_pte, *orig_dst_pte;
 	pte_t *src_pte, *dst_pte;
 	spinlock_t *src_ptl, *dst_ptl;
@@ -1061,11 +1062,12 @@ static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 	return ret;
 }
 
-static inline int copy_pmd_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		pud_t *dst_pud, pud_t *src_pud, struct vm_area_struct *vma,
-		struct vm_area_struct *new,
+static inline int copy_pmd_range(pud_t *dst_pud, pud_t *src_pud,
+		struct vm_area_struct *vma, struct vm_area_struct *new,
 		unsigned long addr, unsigned long end)
 {
+	struct mm_struct *dst_mm = new->vm_mm;
+	struct mm_struct *src_mm = vma->vm_mm;
 	pmd_t *src_pmd, *dst_pmd;
 	unsigned long next;
 
@@ -1089,18 +1091,18 @@ static inline int copy_pmd_range(struct mm_struct *dst_mm, struct mm_struct *src
 		}
 		if (pmd_none_or_clear_bad(src_pmd))
 			continue;
-		if (copy_pte_range(dst_mm, src_mm, dst_pmd, src_pmd,
-				   vma, new, addr, next))
+		if (copy_pte_range(dst_pmd, src_pmd, vma, new, addr, next))
 			return -ENOMEM;
 	} while (dst_pmd++, src_pmd++, addr = next, addr != end);
 	return 0;
 }
 
-static inline int copy_pud_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		p4d_t *dst_p4d, p4d_t *src_p4d, struct vm_area_struct *vma,
-		struct vm_area_struct *new,
+static inline int copy_pud_range(p4d_t *dst_p4d, p4d_t *src_p4d,
+		struct vm_area_struct *vma, struct vm_area_struct *new,
 		unsigned long addr, unsigned long end)
 {
+	struct mm_struct *dst_mm = new->vm_mm;
+	struct mm_struct *src_mm = vma->vm_mm;
 	pud_t *src_pud, *dst_pud;
 	unsigned long next;
 
@@ -1124,18 +1126,17 @@ static inline int copy_pud_range(struct mm_struct *dst_mm, struct mm_struct *src
 		}
 		if (pud_none_or_clear_bad(src_pud))
 			continue;
-		if (copy_pmd_range(dst_mm, src_mm, dst_pud, src_pud,
-				   vma, new, addr, next))
+		if (copy_pmd_range(dst_pud, src_pud, vma, new, addr, next))
 			return -ENOMEM;
 	} while (dst_pud++, src_pud++, addr = next, addr != end);
 	return 0;
 }
 
-static inline int copy_p4d_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		pgd_t *dst_pgd, pgd_t *src_pgd, struct vm_area_struct *vma,
-		struct vm_area_struct *new,
+static inline int copy_p4d_range(pgd_t *dst_pgd, pgd_t *src_pgd,
+		struct vm_area_struct *vma, struct vm_area_struct *new,
 		unsigned long addr, unsigned long end)
 {
+	struct mm_struct *dst_mm = new->vm_mm;
 	p4d_t *src_p4d, *dst_p4d;
 	unsigned long next;
 
@@ -1147,20 +1148,20 @@ static inline int copy_p4d_range(struct mm_struct *dst_mm, struct mm_struct *src
 		next = p4d_addr_end(addr, end);
 		if (p4d_none_or_clear_bad(src_p4d))
 			continue;
-		if (copy_pud_range(dst_mm, src_mm, dst_p4d, src_p4d,
-				   vma, new, addr, next))
+		if (copy_pud_range(dst_p4d, src_p4d, vma, new, addr, next))
 			return -ENOMEM;
 	} while (dst_p4d++, src_p4d++, addr = next, addr != end);
 	return 0;
 }
 
-int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
-		    struct vm_area_struct *vma, struct vm_area_struct *new)
+int copy_page_range(struct vm_area_struct *vma, struct vm_area_struct *new)
 {
 	pgd_t *src_pgd, *dst_pgd;
 	unsigned long next;
 	unsigned long addr = vma->vm_start;
 	unsigned long end = vma->vm_end;
+	struct mm_struct *dst_mm = new->vm_mm;
+	struct mm_struct *src_mm = vma->vm_mm;
 	struct mmu_notifier_range range;
 	bool is_cow;
 	int ret;
@@ -1209,7 +1210,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		next = pgd_addr_end(addr, end);
 		if (pgd_none_or_clear_bad(src_pgd))
 			continue;
-		if (unlikely(copy_p4d_range(dst_mm, src_mm, dst_pgd, src_pgd,
+		if (unlikely(copy_p4d_range(dst_pgd, src_pgd,
 					    vma, new, addr, next))) {
 			ret = -ENOMEM;
 			break;
-- 
2.26.2

Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()

[Jason Gunthorpe]

On Wed, Sep 30, 2020 at 04:49:50PM -0400, Peter Xu wrote:
> 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 <kirill@shutemov.name>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/linux/mm.h |  3 +--
>  kernel/fork.c      |  2 +-
>  mm/memory.c        | 43 ++++++++++++++++++++++---------------------
>  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 16b799a0522c..8a0ec8dce5f6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -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);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index da8d360fb032..5f42d4afe0ae 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -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);
> diff --git a/mm/memory.c b/mm/memory.c
> index fcfc4ca36eba..251bb5082f4e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -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

Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()

[Peter Xu]

On Fri, Oct 02, 2020 at 08:43:12AM -0300, Jason Gunthorpe wrote:
> > -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)

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,

-- 
Peter Xu

Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()

[Jason Gunthorpe]

On Fri, Oct 02, 2020 at 01:14:29PM -0400, Peter Xu wrote:
> On Fri, Oct 02, 2020 at 08:43:12AM -0300, Jason Gunthorpe wrote:
> > > -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)
> 
> 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

Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()

[Peter Xu]

On Fri, Oct 02, 2020 at 02:28:58PM -0300, Jason Gunthorpe wrote:
> On Fri, Oct 02, 2020 at 01:14:29PM -0400, Peter Xu wrote:
> > On Fri, Oct 02, 2020 at 08:43:12AM -0300, Jason Gunthorpe wrote:
> > > > -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)
> > 
> > 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 :)

Oh, sure. :)

-- 
Peter Xu