Re: [PATCH] Lock mmap_sem when calling migrate_pages() in do_move_pages_to_node()

[Hugh Dickins]

On Mon, 29 Jan 2018, Zi Yan wrote:
> From: Zi Yan <zi.yan@xxxxxxxxxxxxxx>
> 
> migrate_pages() requires at least down_read(mmap_sem) to protect
> related page tables and VMAs from changing. Let's do it in

Page tables are protected by their locks.  VMAs may change while
migration is active on them, but does that need locking against?

> do_page_moves() for both do_move_pages_to_node() and
> add_page_for_migration().
> 
> Also add this lock requirement in the comment of migrate_pages().
> 
> Signed-off-by: Zi Yan <zi.yan@xxxxxxxxxxxxxx>
> ---
>  mm/migrate.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 5d0dc7b85f90..52d029953c32 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1354,6 +1354,9 @@ static int unmap_and_move_huge_page(new_page_t get_new_page,
>   * or free list only if ret != 0.
>   *
>   * Returns the number of pages that were not migrated, or an error code.
> + *
> + * The caller must hold at least down_read(mmap_sem) for to-be-migrated pages
> + * to protect related page tables and VMAs from changing.

I have not been keeping up with Michal's recent migration changes,
but migrate_pages() never used to need mmap_sem held (despite being
called with an mmap_sem held from some of its callsites), and it
would be a backward step to require that now.

There is not even an mm argument to migrate_pages(), so which
mm->mmap_sem do you think would be required for it?  There may be
particular cases in which it is required (when the new_page function
involves the old_page's vma - is that so below?), but in general not.

Hugh

>   */
>  int migrate_pages(struct list_head *from, new_page_t get_new_page,
>  		free_page_t put_new_page, unsigned long private,
> @@ -1457,6 +1460,12 @@ static int store_status(int __user *status, int start, int value, int nr)
>  	return 0;
>  }
>  
> +/*
> + * Migrates the pages from pagelist and put back those not migrated.
> + *
> + * The caller must at least hold down_read(mmap_sem), which is required
> + * for migrate_pages()
> + */
>  static int do_move_pages_to_node(struct mm_struct *mm,
>  		struct list_head *pagelist, int node)
>  {
> @@ -1487,7 +1496,6 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>  	unsigned int follflags;
>  	int err;
>  
> -	down_read(&mm->mmap_sem);
>  	err = -EFAULT;
>  	vma = find_vma(mm, addr);
>  	if (!vma || addr < vma->vm_start || !vma_migratable(vma))
> @@ -1540,7 +1548,6 @@ static int add_page_for_migration(struct mm_struct *mm, unsigned long addr,
>  	 */
>  	put_page(page);
>  out:
> -	up_read(&mm->mmap_sem);
>  	return err;
>  }
>  
> @@ -1561,6 +1568,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  
>  	migrate_prep();
>  
> +	down_read(&mm->mmap_sem);
>  	for (i = start = 0; i < nr_pages; i++) {
>  		const void __user *p;
>  		unsigned long addr;
> @@ -1628,6 +1636,7 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
>  	if (!err)
>  		err = err1;
>  out:
> +	up_read(&mm->mmap_sem);
>  	return err;
>  }
>  
> -- 
> 2.15.1