From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Borntraeger Subject: Re: [PATCH RFC 2/2] KVM: thread creating a vcpu is the owner of that vcpu Date: Wed, 26 Nov 2014 08:54:54 +0100 Message-ID: <5475874E.5080701@de.ibm.com> References: <1416931449-24585-1-git-send-email-dahi@linux.vnet.ibm.com> <1416931449-24585-3-git-send-email-dahi@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, gleb@kernel.org, jfrei@linux.vnet.ibm.com, cornelia.huck@de.ibm.com To: David Hildenbrand , kvm@vger.kernel.org Return-path: Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:47206 "EHLO e06smtp17.uk.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752135AbaKZHzA (ORCPT ); Wed, 26 Nov 2014 02:55:00 -0500 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 26 Nov 2014 07:54:58 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id AA08B1B0804B for ; Wed, 26 Nov 2014 07:55:10 +0000 (GMT) Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sAQ7stXx16253332 for ; Wed, 26 Nov 2014 07:54:55 GMT Received: from d06av09.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sAQ7ssvB004079 for ; Wed, 26 Nov 2014 00:54:55 -0700 In-Reply-To: <1416931449-24585-3-git-send-email-dahi@linux.vnet.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 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 > --- > 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); >