Re: [patch 2/4] KVM: move coalesced_mmio locking to its own device

From: Avi Kivity <avi@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm@vger.kernel.org, Gregory Haskins <gregory.haskins@gmail.com>
Subject: Re: [patch 2/4] KVM: move coalesced_mmio locking to its own device
Date: Tue, 26 May 2009 14:24:33 +0300	[thread overview]
Message-ID: <4A1BD171.5010908@redhat.com> (raw)
In-Reply-To: <20090525110403.GA2789@amt.cnet>

Marcelo Tosatti wrote:
>>> ===================================================================
>>> --- kvm-irqlock.orig/virt/kvm/coalesced_mmio.c
>>> +++ kvm-irqlock/virt/kvm/coalesced_mmio.c
>>> @@ -26,9 +26,12 @@ static int coalesced_mmio_in_range(struc
>>>  	if (!is_write)
>>>  		return 0;
>>>  -	/* kvm->lock is taken by the caller and must be not released before
>>> -         * dev.read/write
>>> -         */
>>> +	/*
>>> + 	 * Some other vcpu might be batching data into the ring,
>>> + 	 * fallback to userspace. Ordering not our problem.
>>> + 	 */
>>> +	if (!atomic_add_unless(&dev->in_use, 1, 1))
>>> +		return 0;
>>>    
>>>       
>> Ordering with simultaneous writes is indeed not our problem, but the  
>> ring may contain ordered writes (even by the same vcpu!)
>>     
>
> You mean writes by a vcpu should maintain ordering even when that vcpu
> migrates to a different pcpu? 
>   

No.  Writes by a single vcpu need to preserve their ordering relative to 
each other.  If a write by vcpu A completes before a write by vcpu B 
starts, then they need to be ordered, since the vcpus may have 
synchronized in between.

Hm, I don't thimk you broke these rules.

> The smp_wmb in coalesced_write guarantees that.
>
>   
>> Suggest our own lock here.  in_use is basically a homemade lock, better  
>> to use the factory made ones which come with a warranty.
>>     
>
> The first submission had a mutex but was considered unorthodox. Although
> i agree with your argument made then, i don't see a way around that.
>
> Some pseudocode please? 
>   

Why not use slots_lock to protect the entire iodevice list (rcu one 
day), and an internal spinlock for coalesced mmio?

-- 
error compiling committee.c: too many arguments to function