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

[Sasha Levin <levinsasha928@gmail.com>]

On 09/05/2013 12:39 PM, Jonathan Austin wrote:
> Hi Sasha,
>
> On 04/09/13 19:01, Sasha Levin wrote:
>> On 09/04/2013 01:48 PM, Pekka Enberg wrote:
>>> On Wed, Sep 4, 2013 at 8:40 PM, Jonathan Austin <jonathan.austin@arm.com> wrote:
>>>> '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.
>>>
>>> Yeah, looks good to me and makes top work again.
>>
>> We might want to make sure performance isn't hit with stuff that's intensive on the serial console.
>
> Indeed, the intention here is very much to reduce overhead...
>
> Do you have an idea already of what you'd like to test?
>
> I've written a little testcase that just prints an incrementing counter to the console in a tight
> loop for 5 seconds (I've tested both buffered and unbuffered output). The measure of 'performance'
> is how high we count in those 5 seconds.
>
> These are averages (mean) of 5 runs on x86.
>
> ----------------+unbuffered+-buffered-+
> native          |  36880   |  40354   |
> ----------------+----------+----------+
> lkvm - original |  24302   |  25335   |
> ----------------+----------+----------+
> lkvm - no-tick  |  22895   |  28202   |
> ----------------+----------+----------+
>
> I ran these all on the framebuffer console. I found that the numbers on gnome-terminal seemed to be
> affected by the activity level of other gui-ish things, and the numbers were different in
> gnome-terminal and xterm. If you want to see more testing then a suggestion of a way we can take
> host terminal performance out of the equation would make me more comfortable with the numbers. I do
> think that as comparisons to each other they're reasonable as they are, though.
>
> So at least in this reasonably artificial case it looks like a minor win for buffered output and a
> minor loss in the unbuffered case.
>
> Happy to try out other things if you're interested.

I've played around with it over here. Looks good to me. Thanks!



Thanks,
Sasha