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

[Ingo Molnar <mingo@elte.hu>]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> On Sat, 2011-06-04 at 12:17 +0200, Ingo Molnar wrote:
> > * Sasha Levin <levinsasha928@gmail.com> wrote:
> > 
> > > On Sat, 2011-06-04 at 11:38 +0200, Ingo Molnar wrote:
> > > > * Sasha Levin <levinsasha928@gmail.com> wrote:
> > > > 
> > > > > Coalescing MMIO allows us to avoid an exit every time we have a
> > > > > MMIO write, instead - MMIO writes are coalesced in a ring which
> > > > > can be flushed once an exit for a different reason is needed.
> > > > > A MMIO exit is also trigged once the ring is full.
> > > > > 
> > > > > Coalesce all MMIO regions registered in the MMIO mapper.
> > > > > Add a coalescing handler under kvm_cpu.
> > > > 
> > > > Does this have any effect on latency? I.e. does the guest side 
> > > > guarantee that the pending queue will be flushed after a group of 
> > > > updates have been done?
> > > 
> > > Theres nothing that detects groups of MMIO writes, but the ring size is
> > > a bit less than PAGE_SIZE (half of it is overhead - rest is data) and
> > > we'll exit once the ring is full.
> > 
> > But if the page is only filled partially and if mmio is not submitted 
> > by the guest indefinitely (say it runs a lot of user-space code) then 
> > the mmio remains pending in the partial-page buffer?
> 
> We flush the ring on any exit from the guest, not just MMIO exit.
> But yes, from what I understand from the code - if the buffer is only
> partially full and we don't take an exit, the buffer doesn't get back to
> the host.
> 
> ioeventfds and such are making exits less common, so yes - it's possible
> we won't have an exit in a while.
> 
> > If that's how it works then i *really* don't like this, this looks 
> > like a seriously mis-designed batching feature which might have 
> > improved a few server benchmarks but which will introduce random, 
> > hard to debug delays all around the place!

The proper way to implement batching is not to do it blindly like 
here, but to do what we do in the TLB coalescing/gather code in the 
kernel:

	gather();

	... submit individual TLB flushes ...

	flush();

That's how it should be done here too: each virtio driver that issues 
a group of MMIOs should first start batching, then issue the 
individual MMIOs and then flush them.

That can be simplified to leave out the gather() phase, i.e. just 
issue batched MMIOs and flush them before exiting the virtio (guest 
side) driver routines.

KVM_CAP_COALESCED_MMIO is an unsafe shortcut hack in its current form 
and it looks completely unsafe.

Thanks,

	Ingo