[PATCH 0/2] KVM: async_pf: use_mm/mm_users fixes

[PATCH 0/2] KVM: async_pf: use_mm/mm_users fixes

[Oleg Nesterov]

Hello.

Completely untested and I know nothing about kvm ;) Please review.

But use_mm() really looks misleading, and the usage of mm_users looks
"obviously wrong". I already sent this change while we were discussing
vmacache, but it was ignored. Since then kvm_async_page_present_sync()
was added into async_pf_execute() into async_pf_execute(), but it seems
to me that use_mm() is still unnecessary.

Oleg.

 virt/kvm/async_pf.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

[PATCH 1/2] KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()

[Oleg Nesterov]

async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm)
should work just fine even if current has another or NULL ->mm.

Recently kvm_async_page_present_sync() was added insedie the "use_mm"
section, but it seems that it doesn't need current->mm too.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 virt/kvm/async_pf.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index 10df100..0ced4f3 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
 
 	might_sleep();
 
-	use_mm(mm);
 	down_read(&mm->mmap_sem);
 	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
 	up_read(&mm->mmap_sem);
 	kvm_async_page_present_sync(vcpu, apf);
-	unuse_mm(mm);
 
 	spin_lock(&vcpu->async_pf.lock);
 	list_add_tail(&apf->link, &vcpu->async_pf.done);
-- 
1.5.5.1

Re: [PATCH 1/2] KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()

[Christian Borntraeger]

On 21/04/14 15:25, Oleg Nesterov wrote:
> async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm)
> should work just fine even if current has another or NULL ->mm.
> 
> Recently kvm_async_page_present_sync() was added insedie the "use_mm"
> section, but it seems that it doesn't need current->mm too.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc.
This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present
might call apf_put_user which might call copy_to_user, so this is not ok, I guess.


> ---
>  virt/kvm/async_pf.c |    2 --
>  1 files changed, 0 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index 10df100..0ced4f3 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
> 
>  	might_sleep();
> 
> -	use_mm(mm);
>  	down_read(&mm->mmap_sem);
>  	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
>  	up_read(&mm->mmap_sem);
>  	kvm_async_page_present_sync(vcpu, apf);
> -	unuse_mm(mm);
> 
>  	spin_lock(&vcpu->async_pf.lock);
>  	list_add_tail(&apf->link, &vcpu->async_pf.done);
> 

Re: [PATCH 1/2] KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()

[Christian Borntraeger]

On 22/04/14 22:15, Christian Borntraeger wrote:
> On 21/04/14 15:25, Oleg Nesterov wrote:
>> async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm)
>> should work just fine even if current has another or NULL ->mm.
>>
>> Recently kvm_async_page_present_sync() was added insedie the "use_mm"
>> section, but it seems that it doesn't need current->mm too.
>>
>> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc.
> This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present
> might call apf_put_user which might call copy_to_user, so this is not ok, I guess.

wanted to say kvm_arch_async_page_not_present, but I have to correct myself.
x86 does the "page is there" in the cpu loop, not in the worker. The cpu look 
d oes have a valid mm. So this patch should be also ok.
 
> 
>> ---
>>  virt/kvm/async_pf.c |    2 --
>>  1 files changed, 0 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
>> index 10df100..0ced4f3 100644
>> --- a/virt/kvm/async_pf.c
>> +++ b/virt/kvm/async_pf.c
>> @@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
>>
>>  	might_sleep();
>>
>> -	use_mm(mm);
>>  	down_read(&mm->mmap_sem);
>>  	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
>>  	up_read(&mm->mmap_sem);
>>  	kvm_async_page_present_sync(vcpu, apf);
>> -	unuse_mm(mm);
>>
>>  	spin_lock(&vcpu->async_pf.lock);
>>  	list_add_tail(&apf->link, &vcpu->async_pf.done);
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()

[Oleg Nesterov]

On 04/22, Christian Borntraeger wrote:
>
> On 22/04/14 22:15, Christian Borntraeger wrote:
> > On 21/04/14 15:25, Oleg Nesterov wrote:
> >> async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm)
> >> should work just fine even if current has another or NULL ->mm.
> >>
> >> Recently kvm_async_page_present_sync() was added insedie the "use_mm"
> >> section, but it seems that it doesn't need current->mm too.
> >>
> >> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> >
> > Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc.
> > This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present
> > might call apf_put_user which might call copy_to_user, so this is not ok, I guess.
>
> wanted to say kvm_arch_async_page_not_present, but I have to correct myself.
> x86 does the "page is there" in the cpu loop, not in the worker. The cpu look
> d oes have a valid mm. So this patch should be also ok.

Thanks ;)

Btw, I forgot to mention this in the changelog, but

> >> @@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
> >>
> >>  	might_sleep();
> >>
> >> -	use_mm(mm);
> >>  	down_read(&mm->mmap_sem);
> >>  	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
> >>  	up_read(&mm->mmap_sem);
> >>  	kvm_async_page_present_sync(vcpu, apf);
> >> -	unuse_mm(mm);

it can actually do

	get_user_pages(NULL, mm, addr, 1, 1, 0, NULL, NULL);

"task" is only used to increment task_struct->xxx_flt. I don't think
async_pf_execute() actually needs this (current is PF_WQ_WORKER after
all), but I didn't dare to do another change in the code I can hardly
understand.

Oleg.

Re: [PATCH 1/2] KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()

[Andrea Arcangeli]

Hi,

On Wed, Apr 23, 2014 at 09:32:28PM +0200, Oleg Nesterov wrote:
> On 04/22, Christian Borntraeger wrote:
> >
> > On 22/04/14 22:15, Christian Borntraeger wrote:
> > > On 21/04/14 15:25, Oleg Nesterov wrote:
> > >> async_pf_execute() has no reasons to adopt apf->mm, gup(current, mm)
> > >> should work just fine even if current has another or NULL ->mm.
> > >>
> > >> Recently kvm_async_page_present_sync() was added insedie the "use_mm"
> > >> section, but it seems that it doesn't need current->mm too.
> > >>
> > >> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> > >
> > > Indeed, use/unuse_mm should only be necessary for copy_to/from_user etc.
> > > This is fine for s390, but it seems that x86 kvm_arch_async_page_not_present
> > > might call apf_put_user which might call copy_to_user, so this is not ok, I guess.
> >
> > wanted to say kvm_arch_async_page_not_present, but I have to correct myself.
> > x86 does the "page is there" in the cpu loop, not in the worker. The cpu look
> > d oes have a valid mm. So this patch should be also ok.
> 
> Thanks ;)
> 
> Btw, I forgot to mention this in the changelog, but
> 
> > >> @@ -80,12 +80,10 @@ static void async_pf_execute(struct work_struct *work)
> > >>
> > >>  	might_sleep();
> > >>
> > >> -	use_mm(mm);
> > >>  	down_read(&mm->mmap_sem);
> > >>  	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
> > >>  	up_read(&mm->mmap_sem);
> > >>  	kvm_async_page_present_sync(vcpu, apf);
> > >> -	unuse_mm(mm);
> 
> it can actually do
> 
> 	get_user_pages(NULL, mm, addr, 1, 1, 0, NULL, NULL);
> 
> "task" is only used to increment task_struct->xxx_flt. I don't think
> async_pf_execute() actually needs this (current is PF_WQ_WORKER after
> all), but I didn't dare to do another change in the code I can hardly
> understand.

Considering the faults would be randomly distributed among the kworker
threads my preference would also be for NULL instead of current.

ptrace and uprobes tends to be the only two places that look into
other mm with gup, ptrace knows the exact pid that it is triggering
the fault into, so it also can specify the correct task so the fault
goes in the right task struct. uprobes uses NULL.

Re: [PATCH 1/2] KVM: async_pf: kill the unnecessary use_mm/unuse_mm async_pf_execute()

[Paolo Bonzini]

Il 28/04/2014 16:06, Andrea Arcangeli ha scritto:
>> >
>> > "task" is only used to increment task_struct->xxx_flt. I don't think
>> > async_pf_execute() actually needs this (current is PF_WQ_WORKER after
>> > all), but I didn't dare to do another change in the code I can hardly
>> > understand.
> Considering the faults would be randomly distributed among the kworker
> threads my preference would also be for NULL instead of current.
>
> ptrace and uprobes tends to be the only two places that look into
> other mm with gup, ptrace knows the exact pid that it is triggering
> the fault into, so it also can specify the correct task so the fault
> goes in the right task struct. uprobes uses NULL.

KVM knows the correct task (it was in current when kvm_create_vm was 
called), and doing accounting right would be nice.  But I agree that 
NULL is less misleading than a dummy current, and I applied patch 3 too.

Paolo

[PATCH 2/2] KVM: async_pf: mm->mm_users can not pin apf->mm

[Oleg Nesterov]

get_user_pages(mm) is simply wrong if mm->mm_users == 0 and exit_mmap/etc
was already called (or is in progress), mm->mm_count can only pin mm->pgd
and mm_struct itself.

Change kvm_setup_async_pf/async_pf_execute to inc/dec mm->mm_users.

kvm_create_vm/kvm_destroy_vm play with ->mm_count too but this case looks
fine at first glance, it seems that this ->mm is only used to verify that
current->mm == kvm->mm.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 virt/kvm/async_pf.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index 0ced4f3..cda703e 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -99,7 +99,7 @@ static void async_pf_execute(struct work_struct *work)
 	if (waitqueue_active(&vcpu->wq))
 		wake_up_interruptible(&vcpu->wq);
 
-	mmdrop(mm);
+	mmput(mm);
 	kvm_put_kvm(vcpu->kvm);
 }
 
@@ -116,7 +116,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
 		flush_work(&work->work);
 #else
 		if (cancel_work_sync(&work->work)) {
-			mmdrop(work->mm);
+			mmput(work->mm);
 			kvm_put_kvm(vcpu->kvm); /* == work->vcpu->kvm */
 			kmem_cache_free(async_pf_cache, work);
 		}
@@ -181,7 +181,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, unsigned long hva,
 	work->addr = hva;
 	work->arch = *arch;
 	work->mm = current->mm;
-	atomic_inc(&work->mm->mm_count);
+	atomic_inc(&work->mm->mm_users);
 	kvm_get_kvm(work->vcpu->kvm);
 
 	/* this can't really happen otherwise gfn_to_pfn_async
@@ -199,7 +199,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, unsigned long hva,
 	return 1;
 retry_sync:
 	kvm_put_kvm(work->vcpu->kvm);
-	mmdrop(work->mm);
+	mmput(work->mm);
 	kmem_cache_free(async_pf_cache, work);
 	return 0;
 }
-- 
1.5.5.1

Re: [PATCH 2/2] KVM: async_pf: mm->mm_users can not pin apf->mm

[Christian Borntraeger]

On 21/04/14 15:26, Oleg Nesterov wrote:
> get_user_pages(mm) is simply wrong if mm->mm_users == 0 and exit_mmap/etc
> was already called (or is in progress), mm->mm_count can only pin mm->pgd
> and mm_struct itself.
> 
> Change kvm_setup_async_pf/async_pf_execute to inc/dec mm->mm_users.
> 
> kvm_create_vm/kvm_destroy_vm play with ->mm_count too but this case looks
> fine at first glance, it seems that this ->mm is only used to verify that
> current->mm == kvm->mm.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

This looks fine, from what I can tell. The old code has the problem mentioned
in your patch description, so your version is probably better.


> ---
>  virt/kvm/async_pf.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index 0ced4f3..cda703e 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -99,7 +99,7 @@ static void async_pf_execute(struct work_struct *work)
>  	if (waitqueue_active(&vcpu->wq))
>  		wake_up_interruptible(&vcpu->wq);
> 
> -	mmdrop(mm);
> +	mmput(mm);
>  	kvm_put_kvm(vcpu->kvm);
>  }
> 
> @@ -116,7 +116,7 @@ void kvm_clear_async_pf_completion_queue(struct kvm_vcpu *vcpu)
>  		flush_work(&work->work);
>  #else
>  		if (cancel_work_sync(&work->work)) {
> -			mmdrop(work->mm);
> +			mmput(work->mm);
>  			kvm_put_kvm(vcpu->kvm); /* == work->vcpu->kvm */
>  			kmem_cache_free(async_pf_cache, work);
>  		}
> @@ -181,7 +181,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, unsigned long hva,
>  	work->addr = hva;
>  	work->arch = *arch;
>  	work->mm = current->mm;
> -	atomic_inc(&work->mm->mm_count);
> +	atomic_inc(&work->mm->mm_users);
>  	kvm_get_kvm(work->vcpu->kvm);
> 
>  	/* this can't really happen otherwise gfn_to_pfn_async
> @@ -199,7 +199,7 @@ int kvm_setup_async_pf(struct kvm_vcpu *vcpu, gva_t gva, unsigned long hva,
>  	return 1;
>  retry_sync:
>  	kvm_put_kvm(work->vcpu->kvm);
> -	mmdrop(work->mm);
> +	mmput(work->mm);
>  	kmem_cache_free(async_pf_cache, work);
>  	return 0;
>  }
> 

Re: [PATCH 0/2] KVM: async_pf: use_mm/mm_users fixes

[Christian Borntraeger]

On 21/04/14 15:25, Oleg Nesterov wrote:
> Hello.
> 
> Completely untested and I know nothing about kvm ;) Please review.
> 
> But use_mm() really looks misleading, and the usage of mm_users looks
> "obviously wrong". I already sent this change while we were discussing
> vmacache, but it was ignored. Since then kvm_async_page_present_sync()
> was added into async_pf_execute() into async_pf_execute(), but it seems
> to me that use_mm() is still unnecessary.
> 
> Oleg.
> 
>  virt/kvm/async_pf.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
> 

I gave both patches some testing on s390, seems fine. I think patch2 really
does fix a bug. So if Paolo, Marcelo, Gleb agree (maybe do a test on x86 for
async_pf) both patches are good to go. Given that somebody tests this on x86:

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Re: [PATCH 0/2] KVM: async_pf: use_mm/mm_users fixes

[Oleg Nesterov]

On 04/24, Christian Borntraeger wrote:
>
> On 21/04/14 15:25, Oleg Nesterov wrote:
> > Hello.
> >
> > Completely untested and I know nothing about kvm ;) Please review.
> >
> > But use_mm() really looks misleading, and the usage of mm_users looks
> > "obviously wrong". I already sent this change while we were discussing
> > vmacache, but it was ignored. Since then kvm_async_page_present_sync()
> > was added into async_pf_execute() into async_pf_execute(), but it seems
> > to me that use_mm() is still unnecessary.
> >
> > Oleg.
> >
> >  virt/kvm/async_pf.c |   10 ++++------
> >  1 files changed, 4 insertions(+), 6 deletions(-)
> >
>
> I gave both patches some testing on s390, seems fine. I think patch2 really
> does fix a bug. So if Paolo, Marcelo, Gleb agree (maybe do a test on x86 for
> async_pf) both patches are good to go. Given that somebody tests this on x86:
>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

Thanks!

I think x86 should be fine, it doesn't select CONFIG_KVM_ASYNC_PF_SYNC and
get_user_pages() is certainly fine without use_mm(). And I still think it
should do get_user_pages(tsk => NULL) but this is minor.

Oleg.

Re: [PATCH 0/2] KVM: async_pf: use_mm/mm_users fixes

[Paolo Bonzini]

Il 21/04/2014 15:25, Oleg Nesterov ha scritto:
> Hello.
>
> Completely untested and I know nothing about kvm ;) Please review.
>
> But use_mm() really looks misleading, and the usage of mm_users looks
> "obviously wrong". I already sent this change while we were discussing
> vmacache, but it was ignored. Since then kvm_async_page_present_sync()
> was added into async_pf_execute() into async_pf_execute(), but it seems
> to me that use_mm() is still unnecessary.
>
> Oleg.
>
>  virt/kvm/async_pf.c |   10 ++++------
>  1 files changed, 4 insertions(+), 6 deletions(-)
>

Applying patch 2 to kvm/master (for 3.15).

Patch 1 will be for 3.16 only, I'd like a review from Marcelo or Andrea 
though (that's "KVM: async_pf: kill the unnecessary use_mm/unuse_mm 
async_pf_execute()" for easier googling).

Paolo

Re: [PATCH 0/2] KVM: async_pf: use_mm/mm_users fixes

[Andrea Arcangeli]

On Mon, Apr 28, 2014 at 01:06:05PM +0200, Paolo Bonzini wrote:
> Patch 1 will be for 3.16 only, I'd like a review from Marcelo or Andrea 
> though (that's "KVM: async_pf: kill the unnecessary use_mm/unuse_mm 
> async_pf_execute()" for easier googling).

Patch 1:

Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>

I think current->NULL would be better too.

[PATCH 3/2] KVM: async_pf: change async_pf_execute() to use get_user_pages(tsk => NULL)

[Oleg Nesterov]

On 04/28, Andrea Arcangeli wrote:
>
> On Mon, Apr 28, 2014 at 01:06:05PM +0200, Paolo Bonzini wrote:
> > Patch 1 will be for 3.16 only, I'd like a review from Marcelo or Andrea
> > though (that's "KVM: async_pf: kill the unnecessary use_mm/unuse_mm
> > async_pf_execute()" for easier googling).
>
> Patch 1:
>
> Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>

Thanks,

> I think current->NULL would be better too.

OK, let me send the trivial one-liner then. I won't mind if you fold it
into 1/2, or I can resend it with this change included.

Oleg.

[PATCH 3/2] KVM: async_pf: change async_pf_execute() to use get_user_pages(tsk => NULL)

[Oleg Nesterov]

async_pf_execute() passes tsk == current to gup(), this is doesn't
hurt but unnecessary and misleading. "tsk" is only used to account
the number of faults and current is the random workqueue thread.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 virt/kvm/async_pf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
index 0ced4f3..62f4223 100644
--- a/virt/kvm/async_pf.c
+++ b/virt/kvm/async_pf.c
@@ -81,7 +81,7 @@ static void async_pf_execute(struct work_struct *work)
 	might_sleep();
 
 	down_read(&mm->mmap_sem);
-	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
+	get_user_pages(NULL, mm, addr, 1, 1, 0, NULL, NULL);
 	up_read(&mm->mmap_sem);
 	kvm_async_page_present_sync(vcpu, apf);
 
-- 
1.5.5.1

Re: [PATCH 3/2] KVM: async_pf: change async_pf_execute() to use get_user_pages(tsk => NULL)

[Paolo Bonzini]

Il 28/04/2014 17:03, Oleg Nesterov ha scritto:
> async_pf_execute() passes tsk == current to gup(), this is doesn't
> hurt but unnecessary and misleading. "tsk" is only used to account
> the number of faults and current is the random workqueue thread.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  virt/kvm/async_pf.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/virt/kvm/async_pf.c b/virt/kvm/async_pf.c
> index 0ced4f3..62f4223 100644
> --- a/virt/kvm/async_pf.c
> +++ b/virt/kvm/async_pf.c
> @@ -81,7 +81,7 @@ static void async_pf_execute(struct work_struct *work)
>  	might_sleep();
>
>  	down_read(&mm->mmap_sem);
> -	get_user_pages(current, mm, addr, 1, 1, 0, NULL, NULL);
> +	get_user_pages(NULL, mm, addr, 1, 1, 0, NULL, NULL);
>  	up_read(&mm->mmap_sem);
>  	kvm_async_page_present_sync(vcpu, apf);
>
>

Thanks, added a Suggested-by for Andrea and applied together with 1/2 to 
kvm/queue.

(Actually, I'm back from a longish vacation and I have a pretty large 
queue, so I haven't even compile tested these for now.  Once I get round 
to at least smoke-test them, I'll really push to kvm/queue).

Paolo