Re: [PATCH] kvm tools: Add MMIO coalescing support

[Sasha Levin <levinsasha928@gmail.com>]

On Sat, 2011-06-04 at 18:34 +0200, Ingo Molnar wrote:
> * Alexander Graf <agraf@suse.de> wrote:
> 
> > 
> > On 04.06.2011, at 16:46, Ingo Molnar wrote:
> > 
> > > 
> > > * Alexander Graf <agraf@suse.de> wrote:
> > > 
> > >> So the simple rule is: don't register a coalesced MMIO region for a 
> > >> region where latency matters. [...]
> > > 
> > > So my first suspicion is confirmed.
> > > 
> > > A quick look at Qemu sources shows that lots of drivers are using 
> > > coalesced_mmio without being aware of the latency effects and only 
> > > one seems to make use of qemu_flush_coalesced_mmio_buffer(). Drivers 
> > > like hw/e1000.c sure look latency critical to me.
> > 
> > e1000 maps its NVRAM on coalesced mmio - which is completely ok.
> 
> Ok!
> 
> > > So i maintain my initial opinion: this is a pretty dangerous 
> > > 'optimization' that should be used with extreme care: i can tell 
> > > it you with pretty good authority that latency problems are much 
> > > more easy to introduce than to find and remove ...
> > 
> > Yup, which is why it's very sparsely used in qemu :). Basically, 
> > it's only e1000 and vga, both of which are heavily used and tested 
> > drivers.
> 
> Ok, so this change in:
> 
>  commit 73389b5ea017288a949ae27536c8cfd298d3e317
>  Author: Sasha Levin <levinsasha928@gmail.com>
>  Date:   Fri Jun 3 22:51:08 2011 +0300
> 
>     kvm tools: Add MMIO coalescing support
> 
> @@ -67,6 +70,16 @@ bool kvm__register_mmio(u64 phys_addr, u64 phys_addr_len, void (*kvm_mmio_callba
>                 .kvm_mmio_callback_fn = kvm_mmio_callback_fn,
>         };
>  
> +       zone = (struct kvm_coalesced_mmio_zone) {
> +               .addr   = phys_addr,
> +               .size   = phys_addr_len,
> +       };
> +       ret = ioctl(kvm->vm_fd, KVM_REGISTER_COALESCED_MMIO, &zone);
> +       if (ret < 0) {
> +               free(mmio);
> +               return false;
> +       }
> 
> Seems completely wrong, because it indiscriminately registers *all* 
> mmio regions as coalesced ones.

Yes. I'll add a flag instead of making all of them coalesced.

-- 

Sasha.