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.aus...@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...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
