From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH v2] MMIO: Make coalesced mmio use a device per zone Date: Tue, 19 Jul 2011 13:57:05 +0300 Message-ID: <4E256301.1040700@redhat.com> References: <1311071471-15546-1-git-send-email-levinsasha928@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, Marcelo Tosatti To: Sasha Levin Return-path: Received: from mx1.redhat.com ([209.132.183.28]:45064 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751032Ab1GSK5K (ORCPT ); Tue, 19 Jul 2011 06:57:10 -0400 In-Reply-To: <1311071471-15546-1-git-send-email-levinsasha928@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 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