Re: [PATCH-next v2] kvm: don't try to take mmu_lock while holding the main raw kvm_lock

[Paolo Bonzini <pbonzini@redhat.com>]

Il 27/06/2013 04:56, Paul Gortmaker ha scritto:
>> Il 26/06/2013 20:11, Paul Gortmaker ha scritto:
>>> > >  		spin_unlock(&kvm->mmu_lock);
>>> > > +		kvm_put_kvm(kvm);
>>> > >  		srcu_read_unlock(&kvm->srcu, idx);
>>> > >  
>> > 
>> > kvm_put_kvm needs to go last.  I can fix when applying, but I'll wait
>> > for Gleb to take a look too.
> I'm curious why you would say that -- since the way I sent it has the
> lock tear down be symmetrical and opposite to the build up - e.g.
> 
>  		idx = srcu_read_lock(&kvm->srcu);
> 
> [...]
> 
> +		kvm_get_kvm(kvm);
> 
> [...]
>  		spin_lock(&kvm->mmu_lock);
>  
> [...]
> 
>  unlock:
>  		spin_unlock(&kvm->mmu_lock);
> +		kvm_put_kvm(kvm);
>  		srcu_read_unlock(&kvm->srcu, idx);
>  
> You'd originally said to put the kvm_get_kvm where it currently is;
> perhaps instead we want the get/put to encompass the whole 
> srcu_read locked section?

The put really needs to be the last thing you do, as the data structure
can be destroyed before it returns.  Where you put kvm_get_kvm doesn't
really matter, since you're protected by the kvm lock.  So, moving the
kvm_get_kvm before would also work---I didn't really mean that
kvm_get_kvm has to be literally just before the raw_spin_unlock.

However, I actually like having the get_kvm right there, because it
makes it explicit that you are using reference counting as a substitute
for holding the lock.  I find it quite idiomatic, and in some sense the
lock/unlock is still symmetric: the kvm_put_kvm goes exactly where you'd
have unlocked the kvm_lock.

Paolo