Re: [RFC] Paravirt timer for KVM

[Jeremy Fitzhardinge <jeremy-TSDbQ3PG+2Y@public.gmane.org>]

Glauber de Oliveira Costa wrote:
> My next TODOs with it are:
> * Get SMP working
> * Try something for stolen time, as jeremy's last suggestion for anthony's patch
> * Measure the time it takes for a hypercall, and subtract this time
> for calculating the expiry time for the timer event.
>   

I don't think there's much point in trying to do stuff like this.  The
guest can be preempted at any time, so there's an arbitrary amount of
time between deciding to set a timeout, and the time the timeout
actually happens.

In theory you can mitigate this by using an absolute rather than
relative timeout value, but in practice I don't think it makes much
difference.

> +
> +/*
> + * This is our read_clock function. The host puts an tsc timestamp each time
> + * it updates a new time, and then we can use it to derive a slightly more
> + * precise notion of elapsed time, converted to nanoseconds.
> + *
> + * If the platform provides a stable tsc, we just use it, and there is no need
> + * for the host to update anything.


How would you deal with suspend/resume/migrate?  Also, do you assume
that stable_tsc also means synchronized tsc on an SMP host?

> + */
> +static cycle_t kvm_clock_read(void) {
> +
> +	u64 delta, last_tsc;
> +	struct timespec *now;
> +
> +	if (hv_clock.stable_tsc) {
> +		rdtscll(last_tsc);
> +		return last_tsc;
>   

So this returns a tsc here? ---->

> +	}
> +
> +	do {
> +		last_tsc = hv_clock.last_tsc;
> +		rmb();
> +		now = &hv_clock.now;
>   

Shouldn't this be taking a copy of now, rather than a pointer to it? 
Otherwise what's the point of this loop?

> +		rmb();
> +	} while (hv_clock.last_tsc != last_tsc);
>   

This won't be an atomic compare on 32-bit; it could get confused by
seeing a half-updated tsc value.

> +
> +	delta = native_read_tsc() - last_tsc;
> +	delta = (delta * hv_clock.tsc_mult) >> KVM_SCALE;
> +
> +	return (cycle_t)now->tv_sec * NSEC_PER_SEC + now->tv_nsec + delta;
>   

---> But returns ns here?

> +}
> +
> +static void kvm_timer_set_mode(enum clock_event_mode mode,
> +				struct clock_event_device *evt)
> +{
> +	WARN_ON(!irqs_disabled());
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		/* this is what we want */
> +		break;
> +	case CLOCK_EVT_MODE_RESUME:
> +		break;
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		WARN_ON(1);
> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +		kvm_hypercall0(KVM_HCALL_STOP_ONESHOT);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +/*
> + * Programming the next event is just a matter of asking the host
> + * to generate us an interrupt when the time expires. We pass the
> + * delta on, and hypervisor will do all remaining tricks. For a more
> + * precise timing, we can just subtract the time spent by the hypercall
>   

Not worthwhile.  It would be better to make the hypercall take an
absolute time, and pass it now+delta.  At least then if you get
preempted past the timeout period you can return -ETIME, and the clock
subsystem will know what to do.

> + */
> +static int kvm_timer_next_event(unsigned long delta,
> +				struct clock_event_device *evt)
> +{
> +	WARN_ON(evt->mode != CLOCK_EVT_MODE_ONESHOT);
> +	kvm_hypercall1(KVM_HCALL_SET_ALARM, delta);
> +	return 0;
> +}
> +
> diff --git a/arch/i386/kernel/setup.c b/arch/i386/kernel/setup.c
> index d474cd6..fd758f9 100644
> --- a/arch/i386/kernel/setup.c
> +++ b/arch/i386/kernel/setup.c
> @@ -46,6 +46,7 @@
>  #include <linux/crash_dump.h>
>  #include <linux/dmi.h>
>  #include <linux/pfn.h>
> +#include <linux/kvm_para.h>
>  
>  #include <video/edid.h>
>  
> @@ -579,6 +580,9 @@ void __init setup_arch(char **cmdline_p)
>  	vmi_init();
>  #endif
>  
> +#ifdef CONFIG_KVM_CLOCK
> +	kvmclock_init();
> +#endif
>   

Why is this necessary?  Can't you hook one of the existing pvops?


    J

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/