[LGUEST] updated nohz/hrtimer WIP patches (v02)

[LGUEST] updated nohz/hrtimer WIP patches (v02)

[James Morris]

At http://namei.org/misc/lguest/patches/time/v02/

Current status:
- Resync to recent upstream lguest patch queue
- Rudimentary clock event device is working, but with a significant 
  performance hit
- Old TSC code included, still needs to be modified to handle freq change

Next:
- Check for pending interrupts after they're enabled again per suggestion 
  from Rusty.



-- 
James Morris
<jmorris@namei.org>

[LGUEST] updated nohz/hrtimer WIP patches (v04)

[James Morris]

At http://namei.org/misc/lguest/patches/time/v04/

Just a resync to the latest upstream lguest patch queue, after some fun 
with bare metal bugs and assorted churn.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [LGUEST] updated nohz/hrtimer WIP patches (v04)

[Jeremy Fitzhardinge]

James Morris wrote:
> At http://namei.org/misc/lguest/patches/time/v04/
>
> Just a resync to the latest upstream lguest patch queue, after some fun 
> with bare metal bugs and assorted churn.
>
>
>
> - James
>   

+void guest_clockevent(struct lguest *lg, const ktime_t __user *u)
+{
+	ktime_t kdelta;
+
+	lgread(lg, &kdelta, (u32)u, sizeof(kdelta));
+
+	if (ktime_to_ns(kdelta) < LG_CLOCK_MIN_DELTA) {
+		if (printk_ratelimit())
+			printk(KERN_DEBUG "%s: guest %u small delta %Ld ns\n",
+			       __FUNCTION__, lg->guestid, ktime_to_ns(kdelta));
+
+		/* kick guest timer immediately */
+		set_bit(0, lg->irqs_pending);
+	} else
+		hrtimer_start(&lg->hrt, kdelta, HRTIMER_MODE_REL);
+}


Two things:

   1. It's probably better to make this interface specified in absolute
      rather than relative time.  Asking for a timeout "X ns from _now_"
      is a bit vague if the guest can be preempted and _now_ can be
      arbitrarily deferred.  The tricky part about using an absolute
      time is that the guest needs to work out how to convert from a
      guest time into hypervisor time...
   2. Rather than kicking the timer immediately for too-short (or
      negative) timeouts, it should have the option to return -ETIME, to
      match the clockevents set_next_event API.


+static int lguest_clockevent_set_next_event(unsigned long delta,
+					    struct clock_event_device *evt)
+{
+	ktime_t kdelta = ktime_sub(evt->next_event, ktime_get());
+	hcall(LHCALL_CLOCKEVENT, __pa(&kdelta), 0, 0);
+	return 0;
+}


Why compute kdelta?  Why not just use "delta"?

    J

Re: [LGUEST] updated nohz/hrtimer WIP patches (v04)

[James Morris]

On Thu, 29 Mar 2007, Jeremy Fitzhardinge wrote:

> Two things:
> 
>    1. It's probably better to make this interface specified in absolute
>       rather than relative time.  Asking for a timeout "X ns from _now_"
>       is a bit vague if the guest can be preempted and _now_ can be
>       arbitrarily deferred.  The tricky part about using an absolute
>       time is that the guest needs to work out how to convert from a
>       guest time into hypervisor time...

Thanks for the review!

Yep, it's as simple as possible now, and absolute time something to 
investigate along with time synchronization between the host and the 
guest (which we effectively avoid at this point).

>    2. Rather than kicking the timer immediately for too-short (or
>       negative) timeouts, it should have the option to return -ETIME, to
>       match the clockevents set_next_event API.

Ok.

> +static int lguest_clockevent_set_next_event(unsigned long delta,
> +					    struct clock_event_device *evt)
> +{
> +	ktime_t kdelta = ktime_sub(evt->next_event, ktime_get());
> +	hcall(LHCALL_CLOCKEVENT, __pa(&kdelta), 0, 0);
> +	return 0;
> +}
> 
> 
> Why compute kdelta?  Why not just use "delta"?

'delta' is limited to 2^32 nanoseconds on 32-bit, which is not enough to 
be useful (I really don't understand why the API is like this).  So, 
instead, the 64-bit ktime_t delta is derived.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [LGUEST] updated nohz/hrtimer WIP patches (v04)

[Jeremy Fitzhardinge]

James Morris wrote:
> Yep, it's as simple as possible now, and absolute time something to 
> investigate along with time synchronization between the host and the 
> guest (which we effectively avoid at this point).
>   

There doesn't need to be any time synchronization; you just need a way
to get at the hypervisor's current clock.  You end up doing the
hypervisor_clock() + delta yourself anyway, but at least its under your
control.

> 'delta' is limited to 2^32 nanoseconds on 32-bit, which is not enough to 
> be useful (I really don't understand why the API is like this).  So, 
> instead, the 64-bit ktime_t delta is derived.

It means the max delta between timer interrupts is ~4 seconds, but
that's not too bad.  The clock infrastructure will sort out resetting
the timer to do longer delays.  It probably isn't terribly useful to do
longer delays than that, because you need to deal with drift between the
kernel's clocksource and the timer.

    J

Re: [LGUEST] updated nohz/hrtimer WIP patches (v04)

[Rusty Russell]

On Thu, 2007-03-29 at 11:53 -0400, James Morris wrote:
> At http://namei.org/misc/lguest/patches/time/v04/
> 
> Just a resync to the latest upstream lguest patch queue, after some fun 
> with bare metal bugs and assorted churn.

Hi James!

	Thanks for the patch!  Nothing major to add, just some questions
mainly...

> diff --git a/drivers/lguest/core.c b/drivers/lguest/core.c
> index d8f136a..344b455 100644
> --- a/drivers/lguest/core.c
> +++ b/drivers/lguest/core.c
> @@ -393,6 +393,23 @@ int find_free_guest(void)
>  	return -1;
>  }
>  
> +void guest_clockevent(struct lguest *lg, const ktime_t __user *u)

interrupts_and_traps.c might be a better place for this?  Similarly the
code currently in lguest_user.c, which is mainly for code dealing
with /dev/lguest.

> +	case LHCALL_CLOCKEVENT:
> +		guest_clockevent(lg, (ktime_t __user *)regs->edx);
> +		break;

Perhaps LHCALL_SET_CLOCKEVENT is a better name? Or were you thinking of
extending it?

> --- a/drivers/lguest/lguest.c
> +++ b/drivers/lguest/lguest.c
> @@ -62,6 +64,7 @@ #include <asm/e820.h>
>  #include <asm/pda.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/mce.h>
> +#include "lg.h"

Hmm, this implies we've knotted the headers somehow.  "lg.h" is supposed
to be the internal header for lg.ko.  Perhaps something needs to be
moved to linux/lguest.h?

Thanks!
Rusty.