[PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[David Hildenbrand]

This series improves yielding on architectures that cannot disable preemption
while entering the guest and makes the creating thread of a VCPU the owning
thread and therefore the yield target when yielding to that VCPU.

We should focus on the case creating thread == executing thread and therefore
remove the complicated handling of PIDs involving synchronize_rcus.

This way we can speed up the creation of VCPUs and directly yield to the
executing vcpu threads.

Please note that - in theory - all VCPU ioctls should be triggered from the same
VCPU thread, so changing threads is not a scenario we should optimize.


David Hildenbrand (2):
  KVM: don't check for PF_VCPU when yielding
  KVM: thread creating a vcpu is the owner of that vcpu

 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 22 ++--------------------
 2 files changed, 3 insertions(+), 20 deletions(-)

-- 
1.8.5.5

[PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[David Hildenbrand]

As some architectures (e.g. s390) can't disable preemption while
entering/leaving the guest, they won't receive the yield in all situations.

kvm_enter_guest() has to be called with preemption_disabled and will set
PF_VCPU. After that point e.g. s390 reenables preemption and starts to execute the
guest. The thread might therefore be scheduled out between kvm_enter_guest() and
kvm_exit_guest(), resulting in PF_VCPU being set but not being run.

Please note that preemption has to stay enabled in order to correctly process
page faults on s390.

Current code takes PF_VCPU as a hint that the VCPU thread is running and
therefore needs no yield. yield_to() checks whether the target thread is running,
so let's use the inbuilt functionality to make it independent of PF_VCPU and
preemption.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 virt/kvm/kvm_main.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 5b45330..184f52e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1782,10 +1782,6 @@ int kvm_vcpu_yield_to(struct kvm_vcpu *target)
 	rcu_read_unlock();
 	if (!task)
 		return ret;
-	if (task->flags & PF_VCPU) {
-		put_task_struct(task);
-		return ret;
-	}
 	ret = yield_to(task, 1);
 	put_task_struct(task);
 
-- 
1.8.5.5

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Christian Borntraeger]

Am 25.11.2014 um 17:04 schrieb David Hildenbrand:
> As some architectures (e.g. s390) can't disable preemption while
> entering/leaving the guest, they won't receive the yield in all situations.
> 
> kvm_enter_guest() has to be called with preemption_disabled and will set
> PF_VCPU. After that point e.g. s390 reenables preemption and starts to execute the
> guest. The thread might therefore be scheduled out between kvm_enter_guest() and
> kvm_exit_guest(), resulting in PF_VCPU being set but not being run.
> 
> Please note that preemption has to stay enabled in order to correctly process
> page faults on s390.
> 
> Current code takes PF_VCPU as a hint that the VCPU thread is running and
> therefore needs no yield. yield_to() checks whether the target thread is running,
> so let's use the inbuilt functionality to make it independent of PF_VCPU and
> preemption.

This change is a trade-off.
PRO: This patch would improve the case of preemption on s390. This is probably a corner case as most distros have preemption off anyway.
CON: The downside is that kvm_vcpu_yield_to is called also from kvm_vcpu_on_spin. Here we want to avoid the scheduler overhead for a wrong decision. 

So I think this patch is probably not what we want in most cases.

> 
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
>  virt/kvm/kvm_main.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 5b45330..184f52e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1782,10 +1782,6 @@ int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>  	rcu_read_unlock();
>  	if (!task)
>  		return ret;
> -	if (task->flags & PF_VCPU) {
> -		put_task_struct(task);
> -		return ret;
> -	}
>  	ret = yield_to(task, 1);
>  	put_task_struct(task);
> 

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[David Hildenbrand]

> This change is a trade-off.
> PRO: This patch would improve the case of preemption on s390. This is probably a corner case as most distros have preemption off anyway.
> CON: The downside is that kvm_vcpu_yield_to is called also from kvm_vcpu_on_spin. Here we want to avoid the scheduler overhead for a wrong decision.   

Won't most of that part be covered by:
	if (!ACCESS_ONCE(vcpu->preempted))

vcpu->preempted is only set when scheduled out involuntarily. It is cleared
when scheduled in. s390 sets it manually, to speed up waking up a vcpu.

So when our task is scheduled in (an PF_VCPU is set), this check will already
avoid scheduler overhead in kvm_vcpu_on_spin() or am I missing something?

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Christian Borntraeger]

Am 26.11.2014 um 10:23 schrieb David Hildenbrand:
>> This change is a trade-off.
>> PRO: This patch would improve the case of preemption on s390. This is probably a corner case as most distros have preemption off anyway.
>> CON: The downside is that kvm_vcpu_yield_to is called also from kvm_vcpu_on_spin. Here we want to avoid the scheduler overhead for a wrong decision.   
> 
> Won't most of that part be covered by:
> 	if (!ACCESS_ONCE(vcpu->preempted))

Hmm, right. Checking vcpu->preempted and PF_VCPU might boil down to the same.
Would be good if to have to performance regression test, though. 

> 
> vcpu->preempted is only set when scheduled out involuntarily. It is cleared
> when scheduled in. s390 sets it manually, to speed up waking up a vcpu.
> 
> So when our task is scheduled in (an PF_VCPU is set), this check will already
> avoid scheduler overhead in kvm_vcpu_on_spin() or am I missing something?
> 

CC Raghavendra KT. Could be rerun your kernbench/sysbench/ebizzy setup on x86 to see if the patch in this thread causes any regression? If think your commit 7bc7ae25b143"kvm: Iterate over only vcpus that are preempted" might have really made the PF_VCPU check unnecessary

CC Michael Mueller, do we still have our yield performance setup handy to check if this patch causes any regression?


Christian

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Raghavendra KT]

Was able to test the patch, here is the result: I have not tested with
bigger VMs though. Results make it difficult to talk about any side
effect of
patch if any.

System 16 core 32cpu (+ht) sandybridge
with 4 guests of 16vcpu each

+-----------+-----------+-----------+------------+-----------+
             kernbench (time taken lower is better)
+-----------+-----------+-----------+------------+-----------+
     base       %stdev      patched      %stdev    %improvement
+-----------+-----------+-----------+------------+-----------+
1x   53.1421     2.3086        54.6671     2.9673      -2.86966
2x   89.6858     6.4540        94.0626     6.8317      -4.88015
+-----------+-----------+-----------+------------+-----------+

+-----------+-----------+-----------+------------+-----------+
             ebizzy  (recors/sec higher is better)
+-----------+-----------+-----------+------------+-----------+
     base        %stdev          patched      %stdev    %improvement
+-----------+-----------+-----------+------------+-----------+
1x 14523.2500     8.4388    14928.8750     3.0478       2.79294
2x  3338.8750     1.4592     3270.8750     2.3980      -2.03661
+-----------+-----------+-----------+------------+-----------+
+-----------+-----------+-----------+------------+-----------+
             dbench  (Throughput higher is better)
+-----------+-----------+-----------+------------+-----------+
     base       %stdev           patched      %stdev    %improvement
+-----------+-----------+-----------+------------+-----------+
1x  6386.4737     1.0487    6703.9113     1.2298       4.97047
2x  2571.4712     1.3733    2571.8175     1.6919       0.01347
+-----------+-----------+-----------+------------+-----------+

Raghu

On Wed, Nov 26, 2014 at 3:01 PM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> Am 26.11.2014 um 10:23 schrieb David Hildenbrand:
>>> This change is a trade-off.
>>> PRO: This patch would improve the case of preemption on s390. This is probably a corner case as most distros have preemption off anyway.
>>> CON: The downside is that kvm_vcpu_yield_to is called also from kvm_vcpu_on_spin. Here we want to avoid the scheduler overhead for a wrong decision.
>>
>> Won't most of that part be covered by:
>>       if (!ACCESS_ONCE(vcpu->preempted))
>
> Hmm, right. Checking vcpu->preempted and PF_VCPU might boil down to the same.
> Would be good if to have to performance regression test, though.
>
>>
>> vcpu->preempted is only set when scheduled out involuntarily. It is cleared
>> when scheduled in. s390 sets it manually, to speed up waking up a vcpu.
>>
>> So when our task is scheduled in (an PF_VCPU is set), this check will already
>> avoid scheduler overhead in kvm_vcpu_on_spin() or am I missing something?
>>
>
> CC Raghavendra KT. Could be rerun your kernbench/sysbench/ebizzy setup on x86 to see if the patch in this thread causes any regression? If think your commit 7bc7ae25b143"kvm: Iterate over only vcpus that are preempted" might have really made the PF_VCPU check unnecessary
>
> CC Michael Mueller, do we still have our yield performance setup handy to check if this patch causes any regression?
>
>
> Christian
>

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Christian Borntraeger]

Am 28.11.2014 um 11:08 schrieb Raghavendra KT:
> Was able to test the patch, here is the result: I have not tested with
> bigger VMs though. Results make it difficult to talk about any side
> effect of
> patch if any.

Thanks a log.

If our assumption is correct, then this patch should have no side effect on x86. Do you have any confidence guess if the numbers below mean: no-change vs. regression vs improvement?

Christian


> 
> System 16 core 32cpu (+ht) sandybridge
> with 4 guests of 16vcpu each
> 
> +-----------+-----------+-----------+------------+-----------+
>              kernbench (time taken lower is better)
> +-----------+-----------+-----------+------------+-----------+
>      base       %stdev      patched      %stdev    %improvement
> +-----------+-----------+-----------+------------+-----------+
> 1x   53.1421     2.3086        54.6671     2.9673      -2.86966
> 2x   89.6858     6.4540        94.0626     6.8317      -4.88015
> +-----------+-----------+-----------+------------+-----------+
> 
> +-----------+-----------+-----------+------------+-----------+
>              ebizzy  (recors/sec higher is better)
> +-----------+-----------+-----------+------------+-----------+
>      base        %stdev          patched      %stdev    %improvement
> +-----------+-----------+-----------+------------+-----------+
> 1x 14523.2500     8.4388    14928.8750     3.0478       2.79294
> 2x  3338.8750     1.4592     3270.8750     2.3980      -2.03661
> +-----------+-----------+-----------+------------+-----------+
> +-----------+-----------+-----------+------------+-----------+
>              dbench  (Throughput higher is better)
> +-----------+-----------+-----------+------------+-----------+
>      base       %stdev           patched      %stdev    %improvement
> +-----------+-----------+-----------+------------+-----------+
> 1x  6386.4737     1.0487    6703.9113     1.2298       4.97047
> 2x  2571.4712     1.3733    2571.8175     1.6919       0.01347
> +-----------+-----------+-----------+------------+-----------+
> 
> Raghu
> 
> On Wed, Nov 26, 2014 at 3:01 PM, Christian Borntraeger
> <borntraeger@de.ibm.com> wrote:
>> Am 26.11.2014 um 10:23 schrieb David Hildenbrand:
>>>> This change is a trade-off.
>>>> PRO: This patch would improve the case of preemption on s390. This is probably a corner case as most distros have preemption off anyway.
>>>> CON: The downside is that kvm_vcpu_yield_to is called also from kvm_vcpu_on_spin. Here we want to avoid the scheduler overhead for a wrong decision.
>>>
>>> Won't most of that part be covered by:
>>>       if (!ACCESS_ONCE(vcpu->preempted))
>>
>> Hmm, right. Checking vcpu->preempted and PF_VCPU might boil down to the same.
>> Would be good if to have to performance regression test, though.
>>
>>>
>>> vcpu->preempted is only set when scheduled out involuntarily. It is cleared
>>> when scheduled in. s390 sets it manually, to speed up waking up a vcpu.
>>>
>>> So when our task is scheduled in (an PF_VCPU is set), this check will already
>>> avoid scheduler overhead in kvm_vcpu_on_spin() or am I missing something?
>>>
>>
>> CC Raghavendra KT. Could be rerun your kernbench/sysbench/ebizzy setup on x86 to see if the patch in this thread causes any regression? If think your commit 7bc7ae25b143"kvm: Iterate over only vcpus that are preempted" might have really made the PF_VCPU check unnecessary
>>
>> CC Michael Mueller, do we still have our yield performance setup handy to check if this patch causes any regression?
>>
>>
>> Christian
>>
> --
> 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 RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Raghavendra K T]

On 11/28/2014 04:28 PM, Christian Borntraeger wrote:
> Am 28.11.2014 um 11:08 schrieb Raghavendra KT:
>> Was able to test the patch, here is the result: I have not tested with
>> bigger VMs though. Results make it difficult to talk about any side
>> effect of
>> patch if any.
>
> Thanks a log.
>
> If our assumption is correct, then this patch should have no side effect on x86. Do you have any confidence guess if the numbers below mean: no-change vs. regression vs improvement?
>

I am seeing very small improvement in <= 1x commit cases
and for >1x overcommit, a very slight regression. But considering the
test environment noises, I do not see much effect from the
patch.

But I admit, I have not explored deeply about,
1. assumption of preempted approximately equals PF_VCPU case logic,
2. whether it helps for any future usages of yield_to against current
sole usage of virtualization.

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[David Hildenbrand]

> On 11/28/2014 04:28 PM, Christian Borntraeger wrote:
> > Am 28.11.2014 um 11:08 schrieb Raghavendra KT:
> >> Was able to test the patch, here is the result: I have not tested with
> >> bigger VMs though. Results make it difficult to talk about any side
> >> effect of
> >> patch if any.
> >
> > Thanks a log.
> >
> > If our assumption is correct, then this patch should have no side effect on x86. Do you have any confidence guess if the numbers below mean: no-change vs. regression vs improvement?
> >
> 
> I am seeing very small improvement in <= 1x commit cases
> and for >1x overcommit, a very slight regression. But considering the
> test environment noises, I do not see much effect from the
> patch.
> 
> But I admit, I have not explored deeply about,
> 1. assumption of preempted approximately equals PF_VCPU case logic,

PF_VCPU is only a hint whether the target vcpu is executing the guest.
If preemption is off or !s390, PF_VCPU means that the target vcpu is running
and can't be preempted.

Although for preemption on and s390, this statement is false. Therefore this
check is not always right.

> 2. whether it helps for any future usages of yield_to against current
> sole usage of virtualization.
> 
> 
> 
Thanks for your test!

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Paolo Bonzini]

On 28/11/2014 12:40, Raghavendra K T wrote:
> I am seeing very small improvement in <= 1x commit cases
> and for >1x overcommit, a very slight regression. But considering the
> test environment noises, I do not see much effect from the
> patch.

I think these results are the only one that could be statisically significant:

                   base     %stdev      patched     %stdev    %improvement
kernbench 1x    53.1421     2.3086      54.6671     2.9673      -2.86966
dbench    1x  6386.4737     1.0487    6703.9113     1.2298       4.97047

and, of course :) one of them says things get worse and the other
says things get better.

Paolo

> But I admit, I have not explored deeply about,
> 1. assumption of preempted approximately equals PF_VCPU case logic,
> 2. whether it helps for any future usages of yield_to against current
> sole usage of virtualization.
> 
> 
> 

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[Paolo Bonzini]

On 25/11/2014 17:04, David Hildenbrand wrote:
> As some architectures (e.g. s390) can't disable preemption while
> entering/leaving the guest, they won't receive the yield in all situations.
> 
> kvm_enter_guest() has to be called with preemption_disabled and will set
> PF_VCPU. After that point e.g. s390 reenables preemption and starts to execute the
> guest. The thread might therefore be scheduled out between kvm_enter_guest() and
> kvm_exit_guest(), resulting in PF_VCPU being set but not being run.
> 
> Please note that preemption has to stay enabled in order to correctly process
> page faults on s390.
> 
> Current code takes PF_VCPU as a hint that the VCPU thread is running and
> therefore needs no yield. yield_to() checks whether the target thread is running,
> so let's use the inbuilt functionality to make it independent of PF_VCPU and
> preemption.
> 
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> ---
>  virt/kvm/kvm_main.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 5b45330..184f52e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1782,10 +1782,6 @@ int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>  	rcu_read_unlock();
>  	if (!task)
>  		return ret;
> -	if (task->flags & PF_VCPU) {
> -		put_task_struct(task);
> -		return ret;
> -	}
>  	ret = yield_to(task, 1);
>  	put_task_struct(task);
>  
> 

Applied with a rewritten commit message:

KVM: don't check for PF_VCPU when yielding

kvm_enter_guest() has to be called with preemption disabled and will
set PF_VCPU.  Current code takes PF_VCPU as a hint that the VCPU thread
is running and therefore needs no yield.

However, the check on PF_VCPU is wrong on s390, where preemption
has to stay enabled on s390 in order to correctly process page faults.
Thus, s390 reenables preemption and starts to execute the guest.
The thread might be scheduled out between kvm_enter_guest() and
kvm_exit_guest(), resulting in PF_VCPU being set but not being run.
When this happens, the opportunity for directed yield is missed.

However, this check is done already in kvm_vcpu_on_spin before calling
kvm_vcpu_yield_loop:

        if (!ACCESS_ONCE(vcpu->preempted))
                continue;

so the check on PF_VCPU is superfluous in general, and this patch 
removes it.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [PATCH RFC 1/2] KVM: don't check for PF_VCPU when yielding

[David Hildenbrand]

> Applied with a rewritten commit message:
> 
> KVM: don't check for PF_VCPU when yielding
> 
> kvm_enter_guest() has to be called with preemption disabled and will
> set PF_VCPU.  Current code takes PF_VCPU as a hint that the VCPU thread
> is running and therefore needs no yield.
> 
> However, the check on PF_VCPU is wrong on s390, where preemption
> has to stay enabled on s390 in order to correctly process page faults.
> Thus, s390 reenables preemption and starts to execute the guest.
> The thread might be scheduled out between kvm_enter_guest() and
> kvm_exit_guest(), resulting in PF_VCPU being set but not being run.
> When this happens, the opportunity for directed yield is missed.
> 
> However, this check is done already in kvm_vcpu_on_spin before calling
> kvm_vcpu_yield_loop:
> 
>         if (!ACCESS_ONCE(vcpu->preempted))
>                 continue;
> 
> so the check on PF_VCPU is superfluous in general, and this patch 
> removes it.
> 
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 

Perfect, thanks!

David

[PATCH RFC 2/2] KVM: thread creating a vcpu is the owner of that vcpu

[David Hildenbrand]

Currently, we allow changing the PID of a VCPU. This PID is used to
identify the thread to yield to if we want to yield to this specific
VCPU.

In practice (e.g. QEMU), the thread creating and executing the VCPU remains
always the same. Temporarily exchanging the PID (e.g. because an ioctl is
triggered from a wrong thread) doesn't really make sense.

The PID is exchanged and a synchronize_rcu() is called. When the executing
thread tries to run the VCPU again, another synchronize_rcu() happens.

If a yield to that VCPU is triggered while the PID of the wrong thread is active,
the wrong thread might receive a yield, but this will most likely not
help the executing thread at all. The executing thread won't have a higher
priority after the wrong thread has finished with the ioctl. The wrong thread
will even receive yields afterwards that were targeted to the executing vcpu,
until the executing VCPU has replaced the PID on the next ioctl - doesn't feel
correct to me.

This patch makes the creating thread the owning thread, and therefore the only
valid yield candidate (especially because VCPU ioctls are - in theory - only
valid when triggered from the owning thread - old user space versions may not
stick to this rule). This should also speed up the initial start of all VCPUs,
when the PID is assigned for the first time.

Should be backwards compatible - if there is any old user space version out
there that doesn't stick to the creating == executing thread rule, yields will
not work as intended.

Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
---
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 18 ++----------------
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index aa56894..f1fe655 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -245,6 +245,7 @@ struct kvm_vcpu {
 	int fpu_active;
 	int guest_fpu_loaded, guest_xcr0_loaded;
 	wait_queue_head_t wq;
+	/* the pid owning this vcpu - target for vcpu yields */
 	struct pid *pid;
 	int sigset_active;
 	sigset_t sigset;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 184f52e..4ba7810 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -124,15 +124,6 @@ int vcpu_load(struct kvm_vcpu *vcpu)
 
 	if (mutex_lock_killable(&vcpu->mutex))
 		return -EINTR;
-	if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) {
-		/* The thread running this VCPU changed. */
-		struct pid *oldpid = vcpu->pid;
-		struct pid *newpid = get_task_pid(current, PIDTYPE_PID);
-		rcu_assign_pointer(vcpu->pid, newpid);
-		if (oldpid)
-			synchronize_rcu();
-		put_pid(oldpid);
-	}
 	cpu = get_cpu();
 	preempt_notifier_register(&vcpu->preempt_notifier);
 	kvm_arch_vcpu_load(vcpu, cpu);
@@ -220,7 +211,7 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->cpu = -1;
 	vcpu->kvm = kvm;
 	vcpu->vcpu_id = id;
-	vcpu->pid = NULL;
+	vcpu->pid = get_task_pid(current, PIDTYPE_PID);
 	init_waitqueue_head(&vcpu->wq);
 	kvm_async_pf_vcpu_init(vcpu);
 
@@ -1771,15 +1762,10 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
 
 int kvm_vcpu_yield_to(struct kvm_vcpu *target)
 {
-	struct pid *pid;
 	struct task_struct *task = NULL;
 	int ret = 0;
 
-	rcu_read_lock();
-	pid = rcu_dereference(target->pid);
-	if (pid)
-		task = get_pid_task(pid, PIDTYPE_PID);
-	rcu_read_unlock();
+	task = get_pid_task(target->pid, PIDTYPE_PID);
 	if (!task)
 		return ret;
 	ret = yield_to(task, 1);
-- 
1.8.5.5

Re: [PATCH RFC 2/2] KVM: thread creating a vcpu is the owner of that vcpu

[Christian Borntraeger]

Am 25.11.2014 um 17:04 schrieb David Hildenbrand:
> Currently, we allow changing the PID of a VCPU. This PID is used to
> identify the thread to yield to if we want to yield to this specific
> VCPU.
> 
> In practice (e.g. QEMU), the thread creating and executing the VCPU remains
> always the same. Temporarily exchanging the PID (e.g. because an ioctl is
> triggered from a wrong thread) doesn't really make sense.
> 
> The PID is exchanged and a synchronize_rcu() is called. When the executing
> thread tries to run the VCPU again, another synchronize_rcu() happens.
> 
> If a yield to that VCPU is triggered while the PID of the wrong thread is active,
> the wrong thread might receive a yield, but this will most likely not
> help the executing thread at all. The executing thread won't have a higher
> priority after the wrong thread has finished with the ioctl. The wrong thread
> will even receive yields afterwards that were targeted to the executing vcpu,
> until the executing VCPU has replaced the PID on the next ioctl - doesn't feel
> correct to me.
> 
> This patch makes the creating thread the owning thread, and therefore the only
> valid yield candidate (especially because VCPU ioctls are - in theory - only
> valid when triggered from the owning thread - old user space versions may not
> stick to this rule). This should also speed up the initial start of all VCPUs,
> when the PID is assigned for the first time.
> 
> Should be backwards compatible - if there is any old user space version out
> there that doesn't stick to the creating == executing thread rule, yields will
> not work as intended.
> 
> Signed-off-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

This change actually makes perfect sense to me:
- The runtime change logic was problematic, (e.g. see commit 7103f60de8 "KVM: avoid unnecessary synchronize_rc" and the qemu fixes for s390 to bring all vCPU ioctls in the right thread).
- It makes vcpu_load cheaper
- It emphasizes what in api.txt: " Only run vcpu ioctls from the same thread that was used to create the
   vcpu."


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

> ---
>  include/linux/kvm_host.h |  1 +
>  virt/kvm/kvm_main.c      | 18 ++----------------
>  2 files changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index aa56894..f1fe655 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -245,6 +245,7 @@ struct kvm_vcpu {
>  	int fpu_active;
>  	int guest_fpu_loaded, guest_xcr0_loaded;
>  	wait_queue_head_t wq;
> +	/* the pid owning this vcpu - target for vcpu yields */
>  	struct pid *pid;
>  	int sigset_active;
>  	sigset_t sigset;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 184f52e..4ba7810 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -124,15 +124,6 @@ int vcpu_load(struct kvm_vcpu *vcpu)
> 
>  	if (mutex_lock_killable(&vcpu->mutex))
>  		return -EINTR;
> -	if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) {
> -		/* The thread running this VCPU changed. */
> -		struct pid *oldpid = vcpu->pid;
> -		struct pid *newpid = get_task_pid(current, PIDTYPE_PID);
> -		rcu_assign_pointer(vcpu->pid, newpid);
> -		if (oldpid)
> -			synchronize_rcu();
> -		put_pid(oldpid);
> -	}
>  	cpu = get_cpu();
>  	preempt_notifier_register(&vcpu->preempt_notifier);
>  	kvm_arch_vcpu_load(vcpu, cpu);
> @@ -220,7 +211,7 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
>  	vcpu->cpu = -1;
>  	vcpu->kvm = kvm;
>  	vcpu->vcpu_id = id;
> -	vcpu->pid = NULL;
> +	vcpu->pid = get_task_pid(current, PIDTYPE_PID);
>  	init_waitqueue_head(&vcpu->wq);
>  	kvm_async_pf_vcpu_init(vcpu);
> 
> @@ -1771,15 +1762,10 @@ EXPORT_SYMBOL_GPL(kvm_vcpu_kick);
> 
>  int kvm_vcpu_yield_to(struct kvm_vcpu *target)
>  {
> -	struct pid *pid;
>  	struct task_struct *task = NULL;
>  	int ret = 0;
> 
> -	rcu_read_lock();
> -	pid = rcu_dereference(target->pid);
> -	if (pid)
> -		task = get_pid_task(pid, PIDTYPE_PID);
> -	rcu_read_unlock();
> +	task = get_pid_task(target->pid, PIDTYPE_PID);
>  	if (!task)
>  		return ret;
>  	ret = yield_to(task, 1);
> 

Re: [PATCH RFC 2/2] KVM: thread creating a vcpu is the owner of that vcpu

[Paolo Bonzini]

On 25/11/2014 17:04, David Hildenbrand wrote:
> @@ -124,15 +124,6 @@ int vcpu_load(struct kvm_vcpu *vcpu)
>  
>  	if (mutex_lock_killable(&vcpu->mutex))
>  		return -EINTR;
> -	if (unlikely(vcpu->pid != current->pids[PIDTYPE_PID].pid)) {
> -		/* The thread running this VCPU changed. */
> -		struct pid *oldpid = vcpu->pid;
> -		struct pid *newpid = get_task_pid(current, PIDTYPE_PID);
> -		rcu_assign_pointer(vcpu->pid, newpid);
> -		if (oldpid)
> -			synchronize_rcu();
> -		put_pid(oldpid);
> -	}

I think it would make more sense to do this only for the KVM_RUN ioctl.

Paolo

Re: [PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[David Hildenbrand]

> This series improves yielding on architectures that cannot disable preemption
> while entering the guest and makes the creating thread of a VCPU the owning
> thread and therefore the yield target when yielding to that VCPU.
> 
> We should focus on the case creating thread == executing thread and therefore
> remove the complicated handling of PIDs involving synchronize_rcus.
> 
> This way we can speed up the creation of VCPUs and directly yield to the
> executing vcpu threads.
> 
> Please note that - in theory - all VCPU ioctls should be triggered from the same
> VCPU thread, so changing threads is not a scenario we should optimize.
> 
> 
> David Hildenbrand (2):
>   KVM: don't check for PF_VCPU when yielding
>   KVM: thread creating a vcpu is the owner of that vcpu
> 
>  include/linux/kvm_host.h |  1 +
>  virt/kvm/kvm_main.c      | 22 ++--------------------
>  2 files changed, 3 insertions(+), 20 deletions(-)
> 

Hi Paolo,

would be good if you could have a look at these patches.

Thanks!

Re: [PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[Paolo Bonzini]

On 03/12/2014 13:12, David Hildenbrand wrote:
>> This series improves yielding on architectures that cannot disable preemption
>> while entering the guest and makes the creating thread of a VCPU the owning
>> thread and therefore the yield target when yielding to that VCPU.
>>
>> We should focus on the case creating thread == executing thread and therefore
>> remove the complicated handling of PIDs involving synchronize_rcus.
>>
>> This way we can speed up the creation of VCPUs and directly yield to the
>> executing vcpu threads.
>>
>> Please note that - in theory - all VCPU ioctls should be triggered from the same
>> VCPU thread, so changing threads is not a scenario we should optimize.
>>
>>
>> David Hildenbrand (2):
>>   KVM: don't check for PF_VCPU when yielding
>>   KVM: thread creating a vcpu is the owner of that vcpu
>>
>>  include/linux/kvm_host.h |  1 +
>>  virt/kvm/kvm_main.c      | 22 ++--------------------
>>  2 files changed, 3 insertions(+), 20 deletions(-)
>>
> 
> Hi Paolo,
> 
> would be good if you could have a look at these patches.

Sure.

I think patch 1 is fine and I am applying it.  For patch 2, what about
moving the ->pid assignment in the KVM_RUN case of kvm_vcpu_ioctl?

Paolo

Re: [PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[David Hildenbrand]

> 
> 
> On 03/12/2014 13:12, David Hildenbrand wrote:
> >> This series improves yielding on architectures that cannot disable preemption
> >> while entering the guest and makes the creating thread of a VCPU the owning
> >> thread and therefore the yield target when yielding to that VCPU.
> >>
> >> We should focus on the case creating thread == executing thread and therefore
> >> remove the complicated handling of PIDs involving synchronize_rcus.
> >>
> >> This way we can speed up the creation of VCPUs and directly yield to the
> >> executing vcpu threads.
> >>
> >> Please note that - in theory - all VCPU ioctls should be triggered from the same
> >> VCPU thread, so changing threads is not a scenario we should optimize.
> >>
> >>
> >> David Hildenbrand (2):
> >>   KVM: don't check for PF_VCPU when yielding
> >>   KVM: thread creating a vcpu is the owner of that vcpu
> >>
> >>  include/linux/kvm_host.h |  1 +
> >>  virt/kvm/kvm_main.c      | 22 ++--------------------
> >>  2 files changed, 3 insertions(+), 20 deletions(-)
> >>
> > 
> > Hi Paolo,
> > 
> > would be good if you could have a look at these patches.
> 
> Sure.
> 
> I think patch 1 is fine and I am applying it.  For patch 2, what about
> moving the ->pid assignment in the KVM_RUN case of kvm_vcpu_ioctl?

Thanks Paolo!

Well, do we have any known user that relies on this thread-switching in case
of KVM_RUN? If yes, I am totally with you.

If not I'd prefer to get this code completely out, as it contains some
unnecessary complexity. And maintaining such code that already had a couple of
bugs in it without any benefit doesn't make much sense. What do you think?

David

> 
> Paolo
> 

Re: [PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[Christian Borntraeger]

Am 03.12.2014 um 13:54 schrieb Paolo Bonzini:
> 
> 
> On 03/12/2014 13:12, David Hildenbrand wrote:
>>> This series improves yielding on architectures that cannot disable preemption
>>> while entering the guest and makes the creating thread of a VCPU the owning
>>> thread and therefore the yield target when yielding to that VCPU.
>>>
>>> We should focus on the case creating thread == executing thread and therefore
>>> remove the complicated handling of PIDs involving synchronize_rcus.
>>>
>>> This way we can speed up the creation of VCPUs and directly yield to the
>>> executing vcpu threads.
>>>
>>> Please note that - in theory - all VCPU ioctls should be triggered from the same
>>> VCPU thread, so changing threads is not a scenario we should optimize.
>>>
>>>
>>> David Hildenbrand (2):
>>>   KVM: don't check for PF_VCPU when yielding
>>>   KVM: thread creating a vcpu is the owner of that vcpu
>>>
>>>  include/linux/kvm_host.h |  1 +
>>>  virt/kvm/kvm_main.c      | 22 ++--------------------
>>>  2 files changed, 3 insertions(+), 20 deletions(-)
>>>
>>
>> Hi Paolo,
>>
>> would be good if you could have a look at these patches.
> 
> Sure.
> 
> I think patch 1 is fine and I am applying it.  For patch 2, what about
> moving the ->pid assignment in the KVM_RUN case of kvm_vcpu_ioctl?

That was my initial patch for the rcu specific latencies (do you remember?) But IMHO  patch 2 is actually the proper thing to do, no?

Christian

Re: [PATCH RFC 0/2] assign each vcpu an owning thread and improve yielding

[Paolo Bonzini]

On 03/12/2014 14:00, Christian Borntraeger wrote:
> Am 03.12.2014 um 13:54 schrieb Paolo Bonzini:
>>
>>
>> On 03/12/2014 13:12, David Hildenbrand wrote:
>>>> This series improves yielding on architectures that cannot disable preemption
>>>> while entering the guest and makes the creating thread of a VCPU the owning
>>>> thread and therefore the yield target when yielding to that VCPU.
>>>>
>>>> We should focus on the case creating thread == executing thread and therefore
>>>> remove the complicated handling of PIDs involving synchronize_rcus.
>>>>
>>>> This way we can speed up the creation of VCPUs and directly yield to the
>>>> executing vcpu threads.
>>>>
>>>> Please note that - in theory - all VCPU ioctls should be triggered from the same
>>>> VCPU thread, so changing threads is not a scenario we should optimize.
>>>>
>>>>
>>>> David Hildenbrand (2):
>>>>   KVM: don't check for PF_VCPU when yielding
>>>>   KVM: thread creating a vcpu is the owner of that vcpu
>>>>
>>>>  include/linux/kvm_host.h |  1 +
>>>>  virt/kvm/kvm_main.c      | 22 ++--------------------
>>>>  2 files changed, 3 insertions(+), 20 deletions(-)
>>>>
>>>
>>> Hi Paolo,
>>>
>>> would be good if you could have a look at these patches.
>>
>> Sure.
>>
>> I think patch 1 is fine and I am applying it.  For patch 2, what about
>> moving the ->pid assignment in the KVM_RUN case of kvm_vcpu_ioctl?
> 
> That was my initial patch for the rcu specific latencies (do you
> remember?) But IMHO patch 2 is actually the proper thing to do, no?

Was it? :)  Found it:
http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/125694

I don't know... I think it's feasible to have a userspace that creates
all VCPUs in the main thread, and then runs them from multiple threads.
 Sure, those people would not have read the docs carefully, but it has
worked until now.

Wrongly-directed yields are a much better reason to apply your patch
than QEMU slowness, and I've done that now.

Paolo