[PATCH v2 0/3] kvm tools: remove periodic tick

[PATCH v2 0/3] kvm tools: remove periodic tick

[Jonathan Austin]

This patch series removes kvm tool's periodic tick function in favour of a
thread that blocks waiting for input. The paths used for handling input are the
same as when using a periodic tick, but they're not called unless there is
actually input to be processed.

On extremely slow platforms (eg FPGAs) the overhead involved in handling the
timer tick means it is possible to make progress at all inside the VM! This
patch addresses this problem.

In doing this there are a number of small tidyups/cleanups that made sense, too:
- Use a #define for maximum number of term devices
- Refactor the method by which the virtio console handles input in order not to
  - handle input too early
  - handle input multiple times if the worker thread didn't immediately start
    work.
- Rename the periodic_poll function to reflect the functional change

---
Changes since V1
 - s/kvm__arm_read_term/kvm__arch_read_term/ in patch2's coverletter
 - make term_poll_thread static
 - Added Marc's ack

Jonathan Austin (3):
  kvm tools: use #define for maximum number of terminal devices
  kvm tools: remove periodic tick in favour of a polling thread
  kvm tools: stop virtio console doing unnecessary input handling

 tools/kvm/arm/kvm.c         |    2 +-
 tools/kvm/builtin-run.c     |   13 -----------
 tools/kvm/include/kvm/kvm.h |    2 +-
 tools/kvm/kvm.c             |   50 -------------------------------------------
 tools/kvm/powerpc/kvm.c     |    2 +-
 tools/kvm/term.c            |   38 +++++++++++++++++++++++++++++---
 tools/kvm/virtio/console.c  |   23 +++++++++++++++++---
 tools/kvm/x86/kvm.c         |    2 +-
 8 files changed, 59 insertions(+), 73 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] kvm tools: use #define for maximum number of terminal devices

[Jonathan Austin]

Though there may be no near-term plans to change the number of terminal
devices in the future, using TERM_MAX_DEVS instead of '4' makes reading
some of the loops over terminal devices clearer.

This patch makes the this substitution where required.

Signed-off-by: Jonathan Austin <jonathan.austin@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 tools/kvm/term.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/kvm/term.c b/tools/kvm/term.c
index fa85e4a..ac9c7cc 100644
--- a/tools/kvm/term.c
+++ b/tools/kvm/term.c
@@ -16,13 +16,14 @@
 
 #define TERM_FD_IN      0
 #define TERM_FD_OUT     1
+#define TERM_MAX_DEVS	4
 
 static struct termios	orig_term;
 
 int term_escape_char	= 0x01; /* ctrl-a is used for escape */
 bool term_got_escape	= false;
 
-int term_fds[4][2];
+int term_fds[TERM_MAX_DEVS][2];
 
 int term_getc(struct kvm *kvm, int term)
 {
@@ -94,7 +95,7 @@ static void term_cleanup(void)
 {
 	int i;
 
-	for (i = 0; i < 4; i++)
+	for (i = 0; i < TERM_MAX_DEVS; i++)
 		tcsetattr(term_fds[i][TERM_FD_IN], TCSANOW, &orig_term);
 }
 
@@ -140,7 +141,7 @@ int term_init(struct kvm *kvm)
 	struct termios term;
 	int i, r;
 
-	for (i = 0; i < 4; i++)
+	for (i = 0; i < TERM_MAX_DEVS; i++)
 		if (term_fds[i][TERM_FD_IN] == 0) {
 			term_fds[i][TERM_FD_IN] = STDIN_FILENO;
 			term_fds[i][TERM_FD_OUT] = STDOUT_FILENO;
-- 
1.7.9.5

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

[Jonathan Austin]

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>
---
 tools/kvm/arm/kvm.c         |    2 +-
 tools/kvm/builtin-run.c     |   13 -----------
 tools/kvm/include/kvm/kvm.h |    2 +-
 tools/kvm/kvm.c             |   50 -------------------------------------------
 tools/kvm/powerpc/kvm.c     |    2 +-
 tools/kvm/term.c            |   31 +++++++++++++++++++++++++++
 tools/kvm/x86/kvm.c         |    2 +-
 7 files changed, 35 insertions(+), 67 deletions(-)

diff --git a/tools/kvm/arm/kvm.c b/tools/kvm/arm/kvm.c
index 27e6cf4..008b7fe 100644
--- a/tools/kvm/arm/kvm.c
+++ b/tools/kvm/arm/kvm.c
@@ -46,7 +46,7 @@ void kvm__arch_delete_ram(struct kvm *kvm)
 	munmap(kvm->arch.ram_alloc_start, kvm->arch.ram_alloc_size);
 }
 
-void kvm__arch_periodic_poll(struct kvm *kvm)
+void kvm__arch_read_term(struct kvm *kvm)
 {
 	if (term_readable(0)) {
 		serial8250__update_consoles(kvm);
diff --git a/tools/kvm/builtin-run.c b/tools/kvm/builtin-run.c
index 4d7fbf9d..da95d71 100644
--- a/tools/kvm/builtin-run.c
+++ b/tools/kvm/builtin-run.c
@@ -165,13 +165,6 @@ void kvm_run_set_wrapper_sandbox(void)
 	OPT_END()							\
 	};
 
-static void handle_sigalrm(int sig, siginfo_t *si, void *uc)
-{
-	struct kvm *kvm = si->si_value.sival_ptr;
-
-	kvm__arch_periodic_poll(kvm);
-}
-
 static void *kvm_cpu_thread(void *arg)
 {
 	char name[16];
@@ -487,17 +480,11 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv)
 {
 	static char real_cmdline[2048], default_name[20];
 	unsigned int nr_online_cpus;
-	struct sigaction sa;
 	struct kvm *kvm = kvm__new();
 
 	if (IS_ERR(kvm))
 		return kvm;
 
-	sa.sa_flags = SA_SIGINFO;
-	sa.sa_sigaction = handle_sigalrm;
-	sigemptyset(&sa.sa_mask);
-	sigaction(SIGALRM, &sa, NULL);
-
 	nr_online_cpus = sysconf(_SC_NPROCESSORS_ONLN);
 	kvm->cfg.custom_rootfs_name = "default";
 
diff --git a/tools/kvm/include/kvm/kvm.h b/tools/kvm/include/kvm/kvm.h
index ad53ca7..d05b936 100644
--- a/tools/kvm/include/kvm/kvm.h
+++ b/tools/kvm/include/kvm/kvm.h
@@ -103,7 +103,7 @@ void kvm__arch_delete_ram(struct kvm *kvm);
 int kvm__arch_setup_firmware(struct kvm *kvm);
 int kvm__arch_free_firmware(struct kvm *kvm);
 bool kvm__arch_cpu_supports_vm(void);
-void kvm__arch_periodic_poll(struct kvm *kvm);
+void kvm__arch_read_term(struct kvm *kvm);
 
 void *guest_flat_to_host(struct kvm *kvm, u64 offset);
 u64 host_to_guest_flat(struct kvm *kvm, void *ptr);
diff --git a/tools/kvm/kvm.c b/tools/kvm/kvm.c
index cfd30dd..d7d2e84 100644
--- a/tools/kvm/kvm.c
+++ b/tools/kvm/kvm.c
@@ -393,56 +393,6 @@ found_kernel:
 	return ret;
 }
 
-#define TIMER_INTERVAL_NS 1000000	/* 1 msec */
-
-/*
- * This function sets up a timer that's used to inject interrupts from the
- * userspace hypervisor into the guest at periodical intervals. Please note
- * that clock interrupt, for example, is not handled here.
- */
-int kvm_timer__init(struct kvm *kvm)
-{
-	struct itimerspec its;
-	struct sigevent sev;
-	int r;
-
-	memset(&sev, 0, sizeof(struct sigevent));
-	sev.sigev_value.sival_int	= 0;
-	sev.sigev_notify		= SIGEV_THREAD_ID;
-	sev.sigev_signo			= SIGALRM;
-	sev.sigev_value.sival_ptr	= kvm;
-	sev._sigev_un._tid		= syscall(__NR_gettid);
-
-	r = timer_create(CLOCK_REALTIME, &sev, &kvm->timerid);
-	if (r < 0)
-		return r;
-
-	its.it_value.tv_sec		= TIMER_INTERVAL_NS / 1000000000;
-	its.it_value.tv_nsec		= TIMER_INTERVAL_NS % 1000000000;
-	its.it_interval.tv_sec		= its.it_value.tv_sec;
-	its.it_interval.tv_nsec		= its.it_value.tv_nsec;
-
-	r = timer_settime(kvm->timerid, 0, &its, NULL);
-	if (r < 0) {
-		timer_delete(kvm->timerid);
-		return r;
-	}
-
-	return 0;
-}
-firmware_init(kvm_timer__init);
-
-int kvm_timer__exit(struct kvm *kvm)
-{
-	if (kvm->timerid)
-		if (timer_delete(kvm->timerid) < 0)
-			die("timer_delete()");
-
-	kvm->timerid = 0;
-
-	return 0;
-}
-firmware_exit(kvm_timer__exit);
 
 void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size, int debug_fd)
 {
diff --git a/tools/kvm/powerpc/kvm.c b/tools/kvm/powerpc/kvm.c
index b4b9f82..c1712cf 100644
--- a/tools/kvm/powerpc/kvm.c
+++ b/tools/kvm/powerpc/kvm.c
@@ -151,7 +151,7 @@ void kvm__irq_trigger(struct kvm *kvm, int irq)
 	kvm__irq_line(kvm, irq, 0);
 }
 
-void kvm__arch_periodic_poll(struct kvm *kvm)
+void kvm__arch_read_term(struct kvm *kvm)
 {
 	/* FIXME: Should register callbacks to platform-specific polls */
 	spapr_hvcons_poll(kvm);
diff --git a/tools/kvm/term.c b/tools/kvm/term.c
index ac9c7cc..5c3e543 100644
--- a/tools/kvm/term.c
+++ b/tools/kvm/term.c
@@ -25,6 +25,8 @@ bool term_got_escape	= false;
 
 int term_fds[TERM_MAX_DEVS][2];
 
+static pthread_t term_poll_thread;
+
 int term_getc(struct kvm *kvm, int term)
 {
 	unsigned char c;
@@ -91,6 +93,30 @@ bool term_readable(int term)
 	return poll(&pollfd, 1, 0) > 0;
 }
 
+static void *term_poll_thread_loop(void *param)
+{
+	struct pollfd fds[TERM_MAX_DEVS];
+	struct kvm *kvm = (struct kvm *) param;
+
+	int i;
+
+	for (i = 0; i < TERM_MAX_DEVS; i++) {
+		fds[i].fd = term_fds[i][TERM_FD_IN];
+		fds[i].events = POLLIN;
+		fds[i].revents = 0;
+	}
+
+	while (1) {
+		/* Poll with infinite timeout */
+		if(poll(fds, TERM_MAX_DEVS, -1) < 1)
+			break;
+		kvm__arch_read_term(kvm);
+	}
+
+	die("term_poll_thread_loop: error polling device fds %d\n", errno);
+	return NULL;
+}
+
 static void term_cleanup(void)
 {
 	int i;
@@ -161,6 +187,11 @@ int term_init(struct kvm *kvm)
 	term.c_lflag &= ~(ICANON | ECHO | ISIG);
 	tcsetattr(STDIN_FILENO, TCSANOW, &term);
 
+
+	/* Use our own blocking thread to read stdin, don't require a tick */
+	if(pthread_create(&term_poll_thread, NULL, term_poll_thread_loop,kvm))
+		die("Unable to create console input poll thread\n");
+
 	signal(SIGTERM, term_sig_cleanup);
 	atexit(term_cleanup);
 
diff --git a/tools/kvm/x86/kvm.c b/tools/kvm/x86/kvm.c
index 9b46772..d285d1d 100644
--- a/tools/kvm/x86/kvm.c
+++ b/tools/kvm/x86/kvm.c
@@ -374,7 +374,7 @@ int kvm__arch_free_firmware(struct kvm *kvm)
 	return 0;
 }
 
-void kvm__arch_periodic_poll(struct kvm *kvm)
+void kvm__arch_read_term(struct kvm *kvm)
 {
 	serial8250__update_consoles(kvm);
 	virtio_console__inject_interrupt(kvm);
-- 
1.7.9.5

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

[Pekka Enberg]

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?

When I start it up, it seems as if it's stuck but whenever I press a
key, it prints
part of the screen.

                        Pekka

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

[Jonathan Austin]

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

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

[Pekka Enberg]

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.

                                Pekka

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

[Sasha Levin]

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.


Thanks,
Sasha

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

[Jonathan Austin]

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.

Jonny

> Thanks,
> Sasha
>
>

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

[Sasha Levin]

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

[PATCH v2 3/3] kvm tools: stop virtio console doing unnecessary input handling

[Jonathan Austin]

The asynchronous nature of the virtio input handling (using a job queue)
can result in unnecessary jobs being created if there is some delay in
handing input (the original function to handle the input returns immediately
without the file having been read, and hence poll returns immediately
informing us of data to read).

This patch adds synchronisation to the threads so that we don't start
polling input files again until we've read from the console.

Signed-off-by: Jonathan Austin <jonathan.austin@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
---
 tools/kvm/virtio/console.c |   23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/kvm/virtio/console.c b/tools/kvm/virtio/console.c
index 83c58bf..f982dab7 100644
--- a/tools/kvm/virtio/console.c
+++ b/tools/kvm/virtio/console.c
@@ -36,12 +36,17 @@ struct con_dev {
 	struct virtio_console_config	config;
 	u32				features;
 
+	pthread_cond_t			poll_cond;
+	int				vq_ready;
+
 	struct thread_pool__job		jobs[VIRTIO_CONSOLE_NUM_QUEUES];
 };
 
 static struct con_dev cdev = {
 	.mutex				= MUTEX_INITIALIZER,
 
+	.vq_ready			= 0,
+
 	.config = {
 		.cols			= 80,
 		.rows			= 24,
@@ -69,6 +74,9 @@ static void virtio_console__inject_interrupt_callback(struct kvm *kvm, void *par
 
 	vq = param;
 
+	if (!cdev.vq_ready)
+		pthread_cond_wait(&cdev.poll_cond, &cdev.mutex.mutex);
+
 	if (term_readable(0) && virt_queue__available(vq)) {
 		head = virt_queue__get_iov(vq, iov, &out, &in, kvm);
 		len = term_getc_iov(kvm, iov, in, 0);
@@ -81,7 +89,8 @@ static void virtio_console__inject_interrupt_callback(struct kvm *kvm, void *par
 
 void virtio_console__inject_interrupt(struct kvm *kvm)
 {
-	thread_pool__do_job(&cdev.jobs[VIRTIO_CONSOLE_RX_QUEUE]);
+	virtio_console__inject_interrupt_callback(kvm,
+					&cdev.vqs[VIRTIO_CONSOLE_RX_QUEUE]);
 }
 
 static void virtio_console_handle_callback(struct kvm *kvm, void *param)
@@ -141,10 +150,16 @@ static int init_vq(struct kvm *kvm, void *dev, u32 vq, u32 page_size, u32 align,
 
 	vring_init(&queue->vring, VIRTIO_CONSOLE_QUEUE_SIZE, p, align);
 
-	if (vq == VIRTIO_CONSOLE_TX_QUEUE)
+	if (vq == VIRTIO_CONSOLE_TX_QUEUE) {
 		thread_pool__init_job(&cdev.jobs[vq], kvm, virtio_console_handle_callback, queue);
-	else if (vq == VIRTIO_CONSOLE_RX_QUEUE)
+	} else if (vq == VIRTIO_CONSOLE_RX_QUEUE) {
 		thread_pool__init_job(&cdev.jobs[vq], kvm, virtio_console__inject_interrupt_callback, queue);
+		/* Tell the waiting poll thread that we're ready to go */
+		mutex_lock(&cdev.mutex);
+		cdev.vq_ready = 1;
+		pthread_cond_signal(&cdev.poll_cond);
+		mutex_unlock(&cdev.mutex);
+	}
 
 	return 0;
 }
@@ -192,6 +207,8 @@ int virtio_console__init(struct kvm *kvm)
 	if (kvm->cfg.active_console != CONSOLE_VIRTIO)
 		return 0;
 
+	pthread_cond_init(&cdev.poll_cond, NULL);
+
 	virtio_init(kvm, &cdev, &cdev.vdev, &con_dev_virtio_ops,
 		    VIRTIO_DEFAULT_TRANS, PCI_DEVICE_ID_VIRTIO_CONSOLE,
 		    VIRTIO_ID_CONSOLE, PCI_CLASS_CONSOLE);
-- 
1.7.9.5