Re: [PATCH v2] MMIO: Make coalesced mmio use a device per zone

Avi Kivity
2011-07-19T10:57:10Z

On 07/19/2011 01:31 PM, Sasha Levin wrote:
> This patch changes coalesced mmio to create one mmio device per
> zone instead of handling all zones in one device.
>
> Doing so enables us to take advantage of existing locking and prevents
> a race condition between coalesced mmio registration/unregistration
> and lookups.
>
> @@ -63,7 +63,7 @@ extern struct kmem_cache *kvm_vcpu_cache;
> */
> struct kvm_io_bus {
> int dev_count;
> -#define NR_IOBUS_DEVS 200
> +#define NR_IOBUS_DEVS 300
> struct kvm_io_device *devs[NR_IOBUS_DEVS];
> };

This means that a lot of non-coalesced-mmio users can squeeze out
coalesced-mmio. I don't know if it's really worthwhile, but the 100
coalesced mmio slots should be reserved so we are guaranteed they are
available.

>
> @@ -95,6 +85,8 @@ static void coalesced_mmio_destructor(struct kvm_io_device *this)
> {
> struct kvm_coalesced_mmio_dev *dev = to_mmio(this);
>
> + list_del(&dev->list);
> +
> kfree(dev);
> }
>

No lock?

> int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
> struct kvm_coalesced_mmio_zone *zone)
> {
> - int i;
> - struct kvm_coalesced_mmio_dev *dev = kvm->coalesced_mmio_dev;
> - struct kvm_coalesced_mmio_zone *z;
> -
> - if (dev == NULL)
> - return -ENXIO;
> + struct kvm_coalesced_mmio_dev *dev;
>
> mutex_lock(&kvm->slots_lock);
>
> - i = dev->nb_zones;
> - while (i) {
> - z =&dev->zone[i - 1];
> -
> - /* unregister all zones
> - * included in (zone->addr, zone->size)
> - */
> -
> - if (zone->addr<= z->addr&&
> - z->addr + z->size<= zone->addr + zone->size) {
> - dev->nb_zones--;
> - *z = dev->zone[dev->nb_zones];
> + list_for_each_entry(dev,&kvm->coalesced_zones.items, list)
> + if (coalesced_mmio_in_range(dev, zone->addr, zone->size)) {
> + kvm_io_bus_unregister_dev(kvm, KVM_MMIO_BUS,&dev->dev);
> + kvm_iodevice_destructor(&dev->dev);
> }
> - i--;
> - }

No lock?

>
> struct kvm_coalesced_mmio_dev {
> + struct list_head list;
> struct kvm_io_device dev;
> struct kvm *kvm;
> - spinlock_t lock;
> - int nb_zones;
> - struct kvm_coalesced_mmio_zone zone[KVM_COALESCED_MMIO_ZONE_MAX];
> + struct kvm_coalesced_mmio_zone zone;
> };
>

Why a list instead of a linear array?

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