Re: [PATCH v2 2/3] kvm tools: remove periodic tick in favour of a polling thread

[Jonathan Austin <jonathan.austin@arm.com>]

Hi Pekka,

On 04/09/13 16:58, Pekka Enberg wrote:
> Hi Jonathan,
> 
> On Wed, Sep 4, 2013 at 4:25 PM, Jonathan Austin <jonathan.austin@arm.com> wrote:
>> Currently the only use of the periodic timer tick in kvmtool is to
>> handle reading from stdin. Though functional, this periodic tick can be
>> problematic on slow (eg FPGA) platforms and can cause low interactivity or
>> even stop the execution from progressing at all.
>>
>> This patch removes the periodic tick in favour of a dedicated thread blocked
>> waiting for input from the console. In order to reflect the new behaviour,
>> the old 'kvm__arch_periodic_tick' function is renamed to 'kvm__arch_read_term'.
>>
>> Signed-off-by: Jonathan Austin <jonathan.austin@arm.com>
>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> I'm afraid this breaks "top" on x86. Does it work on arm?
> 

Sorry about that...

'top' works on ARM with virtio console. I've just done some new testing
and with the serial console emulation and I see the same as you're reporting.
Previously with the 8250 emulation I'd booted to a prompt but didn't actually
test top...

I'm looking in to fixing this now... Looks like I need to find the right place
from which to call serial8250_flush_tx now that it isn't getting called every tick.

I've done the following and it works fixes 'top' with serial8250:
-------8<----------
diff --git a/tools/kvm/hw/serial.c b/tools/kvm/hw/serial.c
index 931067f..a71e68d 100644
--- a/tools/kvm/hw/serial.c
+++ b/tools/kvm/hw/serial.c
@@ -260,6 +260,7 @@ static bool serial8250_out(struct ioport *ioport, struct kvm *kvm, u16 port,
                        dev->lsr &= ~UART_LSR_TEMT;
                        if (dev->txcnt == FIFO_LEN / 2)
                                dev->lsr &= ~UART_LSR_THRE;
+                       serial8250_flush_tx(kvm, dev);
                } else {
                        /* Should never happpen */
                        dev->lsr &= ~(UART_LSR_TEMT | UART_LSR_THRE);

------------->8-----------

I guess it's a shame that we'll be printing each character (admittedly the rate will always be
relatively low...) rather than flushing the buffer in a batch. Without a timer, though, I'm
not sure I see a better option - every N chars doesn't seem like a good one to me.

If you think that looks about right then I'll fold that in to the patch series, probably also
removing the call to serial8250_flush_tx() in serial8250__receive.

Thanks,

Jonny