lkml.org 
[lkml]   [2018]   [Jan]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Lock mmap_sem when calling migrate_pages() in do_move_pages_to_node()


Michal Hocko wrote:
> On Mon 29-01-18 22:00:11, Zi Yan wrote:
>> From: Zi Yan <zi.yan@cs.rutgers.edu>
>>
>> migrate_pages() requires at least down_read(mmap_sem) to protect
>> related page tables and VMAs from changing. Let's do it in
>> 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().
>
> This doesn't make much sense to me, to be honest. We are holding
> mmap_sem for _read_ so we allow parallel updates like page faults
> or unmaps. Therefore we are isolating pages prior to the migration.
>
> The sole purpose of the mmap_sem in add_page_for_migration is to protect
> from vma going away _while_ need it to get the proper page.

Then, I am wondering why we are holding mmap_sem when calling
migrate_pages() in existing code.
http://elixir.free-electrons.com/linux/latest/source/mm/migrate.c#L1576

>
> Moving the lock up is just wrong because it allows caller to hold the
> lock for way too long if a lot of pages is migrated. Not only that,
> it is even incorrect because we are doing get_user() (aka page fault)
> and while read lock recursion is OK, we might block and deadlock when
> there is a writer pending. I haven't checked the current implementation
> of semaphores but I believe we do not allow recursive locking.
>

Sorry, I missed that. If mmap_sem is not needed for migrate_pages(),
please ignore this patch.


>> Signed-off-by: Zi Yan <zi.yan@cs.rutgers.edu>
>> ---
>> 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.
>> */
>> 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
>

--
Best Regards,
Yan Zi

[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2018-01-30 16:53    [W:0.057 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site