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

Amos Kong
2012-02-13T02:07:01Z

----- Original Message -----
> ----- Original Message -----
> > 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.
>
>
> In commit e80e2a60, it increase NR_IOBUS_DEVS because
> virtio-net(vhost=on)
> device requests 2 iobus devices.
>
> > commit e80e2a60ff7914dae691345a976c80bbbff3ec74
> > Author: Sridhar Samudrala <sri@us.ibm.com>
> > Date: Tue Mar 30 16:48:25 2010 -0700
> >
> > KVM: Increase NR_IOBUS_DEVS limit to 200
> >
> > This patch increases the current hardcoded limit of
> > NR_IOBUS_DEVS
> > from 6 to 200. We are hitting this limit when creating a guest
> > with more
> > than 1 virtio-net device using vhost-net backend. Each
> > virtio-net
> > device requires 2 such devices to service notifications from
> > rx/tx queues.
> >
> > Signed-off-by: Sridhar Samudrala <sri@us.ibm.com>
> > Signed-off-by: Avi Kivity <avi@redhat.com>
>
> Hi all,
>
> #define KVM_COALESCED_MMIO_ZONE_MAX 100
> Old maximum of coalesced mmio zone is 100. After apply this patch,
> each zone has an iobus devices, so the NR_IOBUS_DEVS is increased by
> 100.
>
> > commit 8c99ce360904ba2f46d4840f1b8df7451331ea38
> > Author: Sasha Levin <levinsasha928@gmail.com>
> > Date: Wed Jul 20 20:59:00 2011 +0300
> >
> > KVM: Make coalesced mmio use a device per zone
> >
> > 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.
> >
> > Suggested-by: Avi Kivity <avi@redhat.com>
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
>
> kvm_io_bus devices are used for ioevent, pit, pic, ioapic,
> coalesced_mmio.
>
> virt/kvm/coalesced_mmio.c:144: ret = kvm_io_bus_register_dev(kvm,
> KVM_MMIO_BUS, zone->addr, -->
> kvm_vm_ioctl_register_coalesced_mmio()
> virt/kvm/eventfd.c:589: ret = kvm_io_bus_register_dev(kvm, bus_idx,
> p->addr, p->length, --> kvm_assign_ioeventfd()
> virt/kvm/ioapic.c:397: ret = kvm_io_bus_register_dev(kvm,
> KVM_MMIO_BUS, ioapic->base_address, -->
> kvm_ioapic_init()
> arch/x86/kvm/i8254.c:716: ret = kvm_io_bus_register_dev(kvm,
> KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS, --> kvm_create_pit()
> arch/x86/kvm/i8254.c:723: ret =
> kvm_io_bus_register_dev(kvm, KVM_PIO_BUS,
> arch/x86/kvm/i8259.c:613: ret = kvm_io_bus_register_dev(kvm,
> KVM_PIO_BUS, 0x20, 2, ---> kvm_create_pic()
> arch/x86/kvm/i8259.c:618: ret = kvm_io_bus_register_dev(kvm,
> KVM_PIO_BUS, 0xa0, 2, &s->dev_slave);
> arch/x86/kvm/i8259.c:622: ret = kvm_io_bus_register_dev(kvm,
> KVM_PIO_BUS, 0x4d0, 2, &s->dev_eclr);
>
>
> Each virtio-blk device needs 1 iobus device ?
> Each virtio-net(vhost=on) device needs 2 iobus devices?
>
> Currently Qemu only emulates one PCI bus, it contains 32 slots, one
> slot contains 8 functions.
> maximum of supported PCI devices: 1 * 32 * 8 = 256, '300' IOBUS
> devices are enough?

Hi everyone,

Any thoughts about this?

Thanks,
Amos

> > > > >
> > > > > @@ -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.
> >