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

[PATCH 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

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 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 this substitution where required.

Signed-off-by: Jonathan Austin <jonathan.austin@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 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__arm_read_term'.

Signed-off-by: Jonathan Austin <jonathan.austin@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..935503f 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];
 
+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

[PATCH 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>
---
 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

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

[Pekka Enberg]

On 9/3/13 9:10 PM, Jonathan Austin wrote:
> 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
>
> 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(-)

Seems reasonable to me. Marc, Will?

                         Pekka

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

[Marc Zyngier]

Hi Jonny,

Just a couple of nits, see below:

On 03/09/13 19:10, Jonathan Austin 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__arm_read_term'.

s/kvm__arm_read_term/kvm__arch_read_term/

> Signed-off-by: Jonathan Austin <jonathan.austin@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..935503f 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];
>  
> +pthread_t term_poll_thread;
> +

Can't this be made static?

>  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);
> 

Otherwise, looks good to me.

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

On 04/09/13 10:23, Pekka Enberg wrote:

Hi Pekka,

> On 9/3/13 9:10 PM, Jonathan Austin wrote:
>> 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
>>
>> 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(-)
> 
> Seems reasonable to me. Marc, Will?

With the nits I mentioned earlier addressed, I'm happy to give my
"Acked-by: Marc Zyngier <marc.zyngier@arm.com>".

I must also mention than I've been using an earlier version of this
patch series, and that my test rig has been much happier since... ;-)

Cheers,

	M.
-- 
Jazz is not dead. It just smells funny...