From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Subject: Re: [PATCH v4] kvm: make vcpu life cycle separated from kvm instance Date: Thu, 15 Dec 2011 15:48:38 +0900 Message-ID: <4EE99846.5080302@oss.ntt.co.jp> References: <1323923328-917-1-git-send-email-kernelfans@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, avi@redhat.com, aliguori@us.ibm.com, gleb@redhat.com, mtosatti@redhat.com, jan.kiszka@web.de To: Liu Ping Fan Return-path: In-Reply-To: <1323923328-917-1-git-send-email-kernelfans@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org (2011/12/15 13:28), Liu Ping Fan wrote: > From: Liu Ping Fan > > Currently, vcpu can be destructed only when kvm instance destroyed. > Change this to vcpu's destruction before kvm instance, so vcpu MUST > and CAN be destroyed before kvm's destroy. Could you explain why this change is needed here? Would be helpful for those, including me, who will read the commit later. > > Signed-off-by: Liu Ping Fan > --- ... > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > index cac4746..f275b8c 100644 > --- a/arch/x86/kvm/i8259.c > +++ b/arch/x86/kvm/i8259.c > @@ -50,25 +50,28 @@ static void pic_unlock(struct kvm_pic *s) > { > bool wakeup = s->wakeup_needed; > struct kvm_vcpu *vcpu, *found = NULL; > - int i; > + struct kvm *kvm = s->kvm; > > s->wakeup_needed = false; > > spin_unlock(&s->lock); > > if (wakeup) { > - kvm_for_each_vcpu(i, vcpu, s->kvm) { > + rcu_read_lock(); > + kvm_for_each_vcpu(vcpu, kvm) > if (kvm_apic_accept_pic_intr(vcpu)) { > found = vcpu; > break; > } > - } > > - if (!found) > + if (!found) { > + rcu_read_unlock(); > return; > + } > > kvm_make_request(KVM_REQ_EVENT, found); > kvm_vcpu_kick(found); > + rcu_read_unlock(); > } > } How about this? (just about stylistic issues) if (!wakeup) return; rcu_read_lock(); kvm_for_each_vcpu(vcpu, kvm) if (kvm_apic_accept_pic_intr(vcpu)) { found = vcpu; break; } if (!found) goto out; kvm_make_request(KVM_REQ_EVENT, found); kvm_vcpu_kick(found); out: rcu_read_unlock(); ... > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c ... > +void kvm_arch_vcpu_zap(struct work_struct *work) > +{ > + struct kvm_vcpu *vcpu = container_of(work, struct kvm_vcpu, > + zap_work); > + struct kvm *kvm = vcpu->kvm; > > - atomic_set(&kvm->online_vcpus, 0); > - mutex_unlock(&kvm->lock); > + kvm_clear_async_pf_completion_queue(vcpu); > + kvm_unload_vcpu_mmu(vcpu); > + kvm_arch_vcpu_free(vcpu); > + kvm_put_kvm(kvm); > } zap is really a good name for this? ... > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index d526231..733de1c 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -113,6 +114,10 @@ enum { > > struct kvm_vcpu { > struct kvm *kvm; > + atomic_t refcount; > + struct list_head list; > + struct rcu_head head; > + struct work_struct zap_work; How about adding some comments? zap_work is not at all self explanatory, IMO. > #ifdef CONFIG_PREEMPT_NOTIFIERS > struct preempt_notifier preempt_notifier; > #endif > @@ -241,9 +246,9 @@ struct kvm { > u32 bsp_vcpu_id; > struct kvm_vcpu *bsp_vcpu; > #endif > - struct kvm_vcpu *vcpus[KVM_MAX_VCPUS]; > + struct list_head vcpus; > atomic_t online_vcpus; > - int last_boosted_vcpu; > + struct kvm_vcpu *last_boosted_vcpu; > struct list_head vm_list; > struct mutex lock; > struct kvm_io_bus *buses[KVM_NR_BUSES]; > @@ -290,17 +295,15 @@ struct kvm { > #define kvm_printf(kvm, fmt ...) printk(KERN_DEBUG fmt) > #define vcpu_printf(vcpu, fmt...) kvm_printf(vcpu->kvm, fmt) > > -static inline struct kvm_vcpu *kvm_get_vcpu(struct kvm *kvm, int i) > -{ > - smp_rmb(); > - return kvm->vcpus[i]; > -} > +struct kvm_vcpu *kvm_vcpu_get(struct kvm_vcpu *vcpu); > +void kvm_vcpu_put(struct kvm_vcpu *vcpu); > +void kvm_arch_vcpu_zap(struct work_struct *work); > + > +#define kvm_for_each_vcpu(vcpu, kvm) \ > + list_for_each_entry_rcu(vcpu,&kvm->vcpus, list) Is this macro really worth it? _rcu shows readers important information, I think. > > -#define kvm_for_each_vcpu(idx, vcpup, kvm) \ > - for (idx = 0; \ > - idx< atomic_read(&kvm->online_vcpus)&& \ > - (vcpup = kvm_get_vcpu(kvm, idx)) != NULL; \ > - idx++) > +#define kvm_for_each_vcpu_continue(vcpu, kvm) \ > + list_for_each_entry_continue_rcu(vcpu,&kvm->vcpus, list) Same here. Why do you want to hide _rcu from readers? Takuya