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

Marcelo Tosatti
2009-05-26T13:15:58Z

On Tue, May 26, 2009 at 02:24:33PM +0300, Avi Kivity wrote:
> 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?

Don't like using slots_lock to protect the entire iodevice list, its
reverse progress in my opinion. The PIO/MMIO device lists are data
structures used in hot path, they are not directly related to the
memslots, and therefore deserve a separate lock. Whenever slots_lock
becomes an issue, you'll have to entangle the mess, so better start
doing it now.

Sure can switch to a internal spinlock for coalesced_mmio, but breaking
out if spin_trylock fails. You're OK with that?