Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()
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
Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()
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()
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()
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
> Signed-off-by: Peter Xu
> ---
> 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
