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

[Sasha Levin <levinsasha928@gmail.com>]

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.

-- 

Sasha.