Re: [PATCH] KVM: Ensure the exit frequency to QEmu for coalesced MMIO

[Sheng Yang <sheng@linux.intel.com>]

On Wednesday 20 January 2010 17:11:51 Avi Kivity wrote:
> On 01/20/2010 10:35 AM, Sheng Yang wrote:
> > The default action of calesced MMIO is, cache the writing in buffer,
> > until: 1. The buffer is full.
> > 2. Or the exit to QEmu due to other reasons.
> >
> > But this would result in a very late writing in some condition.
> > 1. The each time write to MMIO content is small.
> > 2. The writing interval is big.
> > 3. No need for input or accessing other devices frequently.
> >
> > This issue was observed in a experimental embbed system. The test image
> > simply print "test" every 1 seconds. The output in QEmu meets
> > expectation, but the output in KVM is delayed for seconds.
> >
> > To solve this issue, a timeout is added, to ensure userspace exit
> > freqency is high enough(mostly target for video now) to write the
> > buffered MMIO data. Current the maximum exit interval is 1/25s(so 25
> > times exit to userspace per second at least, pretty low compared to
> > normal environment), and reused KVM_EXIT_IRQ_WINDOW_OPEN as exit reason,
> > for it would doing nothing at all in userspace handler.
> 
> This can be done from userspace, by scheduling a SIGALRM in the desired
> frequency.  This way, userspace has control over the update frequency
> (for example, if the SDL window is minimized or VNC is disconnected, we
> don't need to poll).

I also thought of using alarm from userspace. But the issue I thought is a SIGALRM should shoot 
down a vcpu unconditionally. I think in the real world, this situation is not that common, but 
the SIGALRM would definitely bring overhead in every situation. So I choose the current method - 
though still not that elegant.
> 
> I think we can even do this from the I/O thread, without stopping a
> vcpu, since the colaesced mmio page is not tied to a vcpu but is a vm
> property.

This one sounds better. But I've taken a look at the current userspace code:

>#if defined(KVM_CAP_COALESCED_MMIO)
>    if (kvm_state->coalesced_mmio) {
>        struct kvm_coalesced_mmio_ring *ring =
>            (void *) run + kvm_state->coalesced_mmio * PAGE_SIZE;
>        while (ring->first != ring->last) {
>            cpu_physical_memory_rw(ring->coalesced_mmio[ring->first].phys_addr,
>                           &ring->coalesced_mmio[ring->first].data[0],
>                           ring->coalesced_mmio[ring->first].len, 1);
>            smp_wmb();
>            ring->first = (ring->first + 1) % KVM_COALESCED_MMIO_MAX;
>        }
>    }
>#endif

No protection for ring->first and ring->last? Seems it can writing the same element pointed by 
ring->first twice, then skip one element at (ring->first + 1)...

-- 
regards
Yang, Sheng