Re: [RFC] Paravirt timer for KVM

Glauber de Oliveira Costa
2007-10-15T17:15:40Z

On 10/12/07, Jeremy Fitzhardinge <jeremy-TSDbQ3PG+2Y@public.gmane.org> wrote:
> > * 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.

Yes, we can't be precise. But my point here is just being _more_
accurate whenever possible. Yes, it might have been preempted, we
don't know how much time it spent out, but at least this time has
elapsed. It's a win in some scenarios, without too much overhead but
an initial measure and a summation.

Even with tsc adjustments to wallclock, and clock read, I'm using it
regardless of the host having accurate tsc. My main point behind it is
that it will, accurate or not, be _more_ accurate than the simple
version.

Of course, we can discuss if it will really make a difference.

> > + * 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?

I have to think further on that.

But for all cases , I think that the host knows what happened, and can
adjust the timer accordingly. About sync tsc, this code shouldn't be
making this assumption, but right now... it is. I'll think further.

> So this returns a tsc here? ---->
[ .. snip .. ]
> ---> But returns ns here?
Yes. But if you look below, (unless I've really forgot to include in
the patch I fired out), I change the multiplier in case we're using a
tsc value. We multiply by one if using nanosecond timer, and by a host
provided multiplier if tsc timer.

>
> > + }
> > +
> > + 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?

Yes, thanks.

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

You are right, it might be better using a version number like xen does, then.
humm... Rusty's approach of checking against seconds, and then using
nanoseconds only if it has not yet changed may work... but maybe not
as well, due to the tsc adjustments. Right now, I'm more inclined to
the first option.

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

Since the host and guest times are synchronized by assumption, it
might work. I'll give it a try.

> > +#ifdef CONFIG_KVM_CLOCK
> > + kvmclock_init();
> > +#endif
> >
>
> Why is this necessary? Can't you hook one of the existing pvops?

Well, as you all have seen, I've just arrived from weekend, just took
an overlook over all the answers, but I think anthony already answered
this one: Someone has to override the pvops.

However, I think I can do better on this, doing what I did for vsmp on
x86_64, by using ARCH_INIT to check for availability. To be honest, I
considered both approaches, but saw no clear benefit in any, against
the other, and did this way just because vmi is already there.

But yeah, it seems to have some special requirements for placement,
that we do not. If you feel more confortable with it, I can move it to
ARCH_INIT.

>
> J
>

--
Glauber de Oliveira Costa.
"Free as in Freedom"
http://glommer.net

"The less confident you are, the more serious you have to act."