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

[Marc Zyngier <marc.zyngier@arm.com>]

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...