Re: [patch 0/3] kvm tool: Serial emulation overhaul

[Pekka Enberg <penberg@kernel.org>]

* Pekka Enberg <penberg@kernel.org> wrote:
>>> So i'm pretty sure it's some bug in hw/serial.c that's
>>> limiting character output by interrupts.

On Tue, 13 Dec 2011, Thomas Gleixner wrote:
> No, that's not a bug. The current emulation has no fifo and it writes
> every single character to the terminal. Go figure. The timer
> limitation is due to the missing fifo and other details, which would
> drive the serial driver into the "too much work for irq 4" case.
>
> The approach I took was to keep the emulated device as close to the
> real HW for obvious reasons. You simply cannot ignore the way how a HW
> device works and how the corresponding kernel driver expects it to
> work.

No disagreement here.

On Mon, 12 Dec 2011, Ingo Molnar wrote:
>> Serial port control flow somehow being bound by timer frequency
>> [which is not really a necessity: both the host and the guest
>> could stream on full speed] was too my observation early on.
>
> That simply does not work with the way the serial driver for the 8250
> is written.
>
> If you emulate hardware then don't expect that the shortcomings of the
> real hardware and the clusterf*ck in the corresponding device drivers
> go magically away.
>
> If you want high performance virtualization then use virtual drivers
> and stop whining about a 30 years old legacy device and its warts.

If you compare Ingo's numbers to mine, there's still some slowdown which 
cannot be explained by legacy device warts.

Btw, I was able to double the speed of serial console with this simple 
patch:

diff --git a/tools/kvm/hw/serial.c b/tools/kvm/hw/serial.c
index c0f6970..1a8eaaa 100644
--- a/tools/kvm/hw/serial.c
+++ b/tools/kvm/hw/serial.c
@@ -232,7 +232,7 @@ static bool serial8250_out(struct ioport *ioport, struct kvm *kvm, u16 port, voi
  				term_putc(CONSOLE_8250, addr, size, dev->id);
  			/* else FIXME: Inject data into rcv path for LOOP */

-			if (++dev->txcnt == 16)
+			if (++dev->txcnt == 32)
  				dev->lsr &= ~(UART_LSR_TEMT | UART_LSR_THRE);
  			break;
  		} else {

I assume '16' here is the 16550A FIFO size? Is there any reason we can't 
cheat and use a deeper FIFO internally?

 			Pekka