Re: [PATCH v4] kvm: make vcpu life cycle separated from kvm instance

[Liu ping fan]

2011/12/15 Takuya Yoshikawa <yoshikawa.takuya@xxxxxxxxxxxxx>:
> (2011/12/15 13:28), Liu Ping Fan wrote:
>> From: Liu Ping Fan<pingfank@xxxxxxxxxxxxxxxxxx>
>>
>> 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.
>
Suppose the following scene,
Firstly, creating 10 kvm_vcpu for guest to take the advantage of
multi-core. Now, reclaiming some of the kvm_vcpu, so we can limit the
guest's usage of cpu. Then what about the kvm_vcpu unused? Currently
they are just idle in kernel, but with this patch, we can remove them.

>>
>> Signed-off-by: Liu Ping Fan<pingfank@xxxxxxxxxxxxxxxxxx>
>> ---
>
> ...
>
>> 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)
>
:-), I just want to change based on old code. But your style is OK too.

> Â Â Â Â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?
>
zap = destroy, so I think it is OK.
> ...
>
>> 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<linux/slab.h>
>> Â #include<linux/rcupdate.h>
>> Â #include<linux/ratelimit.h>
>> +#include<linux/atomic.h>
>> Â #include<asm/signal.h>
>>
>> Â #include<linux/kvm.h>
>> @@ -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.
>
I guest kvm_for_each_vcpu is designed for hiding the details of
internal implement, and currently it is implemented by array, and my
patch will change it to linked-list,
so IMO, we can still hide the details.

Regards,
ping fan

>>
>> -#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/