Re: [PATCH] IO: Intelligent device lookup on bus

[Avi Kivity <avi@redhat.com>]

On 07/21/2011 11:43 AM, Sasha Levin wrote:
> On Wed, 2011-07-20 at 14:43 +0200, Jan Kiszka wrote:
> >  On 2011-07-20 14:11, Sasha Levin wrote:
> >  >  Currently the method of dealing with an IO operation on a bus (PIO/MMIO)
> >  >  is to call the read or write callback for each device registered
> >  >  on the bus until we find a device which handles it.
> >  >
> >  >  Since the number of devices on a bus can be significant due to ioeventfds
> >  >  and coalesced MMIO zones, this leads to a lot of overhead on each IO
> >  >  operation.
> >  >
> >  >  Instead of registering devices, we now register ranges which points to
> >  >  a device. Lookup is done using an efficient bsearch instead of a linear
> >  >  search.
> >  >
> >  >  This should speed up all IO operations generated by the guest.
> >  >
> >  >  Cc: Avi Kivity<avi@redhat.com>
> >  >  Cc: Marcelo Tosatti<mtosatti@redhat.com>
> >  >  Signed-off-by: Sasha Levin<levinsasha928@gmail.com>
> >  >  ---
> >  >
> >  >  This patch depends on '[PATCH v3] MMIO: Make coalesced mmio use a device
> >  >  per zone'.
> >  >
> >  >   arch/x86/kvm/i8254.c      |    4 +-
> >  >   arch/x86/kvm/i8259.c      |    4 +-
> >  >   include/linux/kvm_host.h  |   18 ++++----
> >  >   virt/kvm/coalesced_mmio.c |    6 +--
> >  >   virt/kvm/eventfd.c        |    3 +-
> >  >   virt/kvm/ioapic.c         |   13 +-----
> >  >   virt/kvm/kvm_main.c       |  107 +++++++++++++++++++++++++++++++++++++++-----
> >  >   7 files changed, 115 insertions(+), 40 deletions(-)
> >  >
> >  >  diff --git a/arch/x86/kvm/i8254.c b/arch/x86/kvm/i8254.c
> >  >  index efad723..61d193c 100644
> >  >  --- a/arch/x86/kvm/i8254.c
> >  >  +++ b/arch/x86/kvm/i8254.c
> >  >  @@ -713,13 +713,15 @@ struct kvm_pit *kvm_create_pit(struct kvm *kvm, u32 flags)
> >  >   	kvm_register_irq_mask_notifier(kvm, 0,&pit->mask_notifier);
> >  >
> >  >   	kvm_iodevice_init(&pit->dev,&pit_dev_ops);
> >  >  -	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS,&pit->dev);
> >  >  +	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, KVM_PIT_BASE_ADDRESS,
> >  >  +					KVM_PIT_MEM_LENGTH,&pit->dev);
> >  >   	if (ret<  0)
> >  >   		goto fail;
> >  >
> >  >   	if (flags&  KVM_PIT_SPEAKER_DUMMY) {
> >  >   		kvm_iodevice_init(&pit->speaker_dev,&speaker_dev_ops);
> >  >   		ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS,
> >  >  +						KVM_SPEAKER_BASE_ADDRESS, 4,
> >  >   						&pit->speaker_dev);
> >  >   		if (ret<  0)
> >  >   			goto fail_unregister;
> >  >  diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c
> >  >  index 19fe855..c2295af 100644
> >  >  --- a/arch/x86/kvm/i8259.c
> >  >  +++ b/arch/x86/kvm/i8259.c
> >  >  @@ -562,7 +562,9 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm)
> >  >   	*/
> >  >   	kvm_iodevice_init(&s->dev,&picdev_ops);
> >  >   	mutex_lock(&kvm->slots_lock);
> >  >  -	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS,&s->dev);
> >  >  +	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, 0x20, 2,&s->dev);
> >  >  +	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, 0xa0, 2,&s->dev);
> >  >  +	ret = kvm_io_bus_register_dev(kvm, KVM_PIO_BUS, 0x4d0, 2,&s->dev);
> >
> >  This made me wonder if you are incorrectly registering the same device
> >  multiple times. kvm_io_bus_register_dev is no longer optimally
> >  describing what is done here: the registration of an IO range with the
> >  bus. And that range is handled by a specific device. Conceptually, a
> >  device is only attached once to some bus.
>
> I agree. My initial thoughts were to leave it this way to simplify
> registrations, but as you said - it makes it somewhat confusing.
>
> I'll modify kvm_io_bus_register_dev to receive an array or ranges
> instead of a single range.

Icky.  How about using multiple kvm_io_device for the pic instead?

Otherwise you'll get the destructor running multiple times.

Note you have to be prepared for the same range to be registered 
multiple times, due to ioeventfd's data match feature.

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