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

[Liu ping fan]

On Thu, Dec 15, 2011 at 1:33 PM, Xiao Guangrong
<xiaoguangrong@xxxxxxxxxxxxxxxxxx> wrote:
> On 12/15/2011 12:28 PM, Liu Ping Fan wrote:
>
>
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -1833,11 +1833,12 @@ static void kvm_mmu_put_page(struct kvm_mmu_page *sp, u64 *parent_pte)
>>
>> Âstatic void kvm_mmu_reset_last_pte_updated(struct kvm *kvm)
>> Â{
>> - Â Â int i;
>> Â Â Â struct kvm_vcpu *vcpu;
>>
>> - Â Â kvm_for_each_vcpu(i, vcpu, kvm)
>> + Â Â rcu_read_lock();
>> + Â Â kvm_for_each_vcpu(vcpu, kvm)
>> Â Â Â Â Â Â Â vcpu->arch.last_pte_updated = NULL;
>> + Â Â rcu_read_unlock();
>> Â}
>>
>
>
> I am sure that you should rebase it on the current kvm tree.
>
OK, I will rebase it in next patch

>> Âstatic void kvm_mmu_unlink_parents(struct kvm *kvm, struct kvm_mmu_page *sp)
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index c38efd7..acaa154 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1830,11 +1830,13 @@ static int get_msr_hyperv(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
>>
>> Â Â Â switch (msr) {
>> Â Â Â case HV_X64_MSR_VP_INDEX: {
>> - Â Â Â Â Â Â int r;
>> + Â Â Â Â Â Â int r = 0;
>> Â Â Â Â Â Â Â struct kvm_vcpu *v;
>> - Â Â Â Â Â Â kvm_for_each_vcpu(r, v, vcpu->kvm)
>> + Â Â Â Â Â Â kvm_for_each_vcpu(v, vcpu->kvm) {
>> Â Â Â Â Â Â Â Â Â Â Â if (v == vcpu)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â data = r;
>> + Â Â Â Â Â Â Â Â Â Â r++;
>> + Â Â Â Â Â Â }
>
>
> Do not need rcu_lock?
>
Need! Sorry, forget.

>> +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)
>>
>> -#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)
>>
>
>
> Where is it used?
>
Once I used it in kvm_vcpu_on_spin, but now, it is useless. I will remove it.

>> +struct kvm_vcpu *kvm_vcpu_get(struct kvm_vcpu *vcpu)
>> +{
>> + Â Â if (vcpu == NULL)
>> + Â Â Â Â Â Â return NULL;
>> + Â Â if (atomic_add_unless(&vcpu->refcount, 1, 0))
>
>
> Why do not use atomic_inc()?
> Also, i think a memory barrier is needed after increasing refcount.
>
Because when refcout==0, we prepare to destroy vcpu, and do not to
disturb it by increasing the refcount.
And sorry but I can not figure out the scene why memory barrier needed
here.  Seems no risks on SMP.

>> - Â Â kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu;
>> + Â Â /*Protected by kvm->lock*/
>> + Â Â list_add_rcu(&vcpu->list, &kvm->vcpus);
>> +
>> Â Â Â smp_wmb();
>
>
> This barrier can also be removed.
>
Yes, I think you are right.

Thanks and regards,
ping fan


>> Â Â Â atomic_inc(&kvm->online_vcpus);
>>
>> Â#ifdef CONFIG_KVM_APIC_ARCHITECTURE
>> Â Â Â if (kvm->bsp_vcpu_id == id)
>> - Â Â Â Â Â Â kvm->bsp_vcpu = vcpu;
>> + Â Â Â Â Â Â kvm->bsp_vcpu = kvm_vcpu_get(vcpu);
>> Â#endif
>> Â Â Â mutex_unlock(&kvm->lock);
>> Â Â Â return r;
>> @@ -2593,13 +2667,15 @@ static int vcpu_stat_get(void *_offset, u64 *val)
>> Â Â Â unsigned offset = (long)_offset;
>> Â Â Â struct kvm *kvm;
>> Â Â Â struct kvm_vcpu *vcpu;
>> - Â Â int i;
>>
>> Â Â Â *val = 0;
>> Â Â Â raw_spin_lock(&kvm_lock);
>> - Â Â list_for_each_entry(kvm, &vm_list, vm_list)
>> - Â Â Â Â Â Â kvm_for_each_vcpu(i, vcpu, kvm)
>> + Â Â list_for_each_entry(kvm, &vm_list, vm_list) {
>> + Â Â Â Â Â Â rcu_read_lock();
>> + Â Â Â Â Â Â kvm_for_each_vcpu(vcpu, kvm)
>> Â Â Â Â Â Â Â Â Â Â Â *val += *(u32 *)((void *)vcpu + offset);
>> + Â Â Â Â Â Â rcu_read_unlock();
>> + Â Â }
>>
>> Â Â Â raw_spin_unlock(&kvm_lock);
>> Â Â Â return 0;
>> @@ -2765,7 +2841,6 @@ int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
>> Â Â Â kvm_preempt_ops.sched_out = kvm_sched_out;
>>
>> Â Â Â kvm_init_debug();
>> -
>
>
> You don not change anything, please do not touch this line.
>
--
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/