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

Avi Kivity
2011-07-19T12:24:36Z

On 07/19/2011 02:05 PM, Sasha Levin wrote:
> On Tue, 2011-07-19 at 13:57 +0300, Avi Kivity wrote:
> > 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.
>
> We are currently registering 4 devices, plus how many
> ioeventfds/coalesced mmio zones the user wants. I felt bad about upping
> it to 300 really.

It's just a few kilobytes, where even a small guest occupies half a
gigabyte. Even just its pagetables swallow up megabytes.

An array means less opportunities to screw up the code and better cache
usage with small objects.

> >
> > >
> > > @@ -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?
>
> The lock is there to synchronize access to the coalesced ring (it was
> here before this patch too, it's not something new), not the device
> list.
>
> The device list is only accessed when kvm->slots_lock is held, so it
> takes care of that.

Right. A comment please.

btw, don't we leak all zones on guest destruction? the array didn't need
any cleanup, but this list does.

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