Re: [PATCH 0/4] Rework alarm timer infrastrucure - take2

[Anthony Liguori <anthony-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>]

I think this is a really nice and important patch set.  Just a couple
things:

On Sun, 2007-08-19 at 00:02 +0200, Luca Tettamanti wrote:

> > In this case the dyn-tick minimum res will be 1msec. I believe it should
> > work ok since this is the case without any dyn-tick.
> 
> Actually minimum resolution depends on host HZ setting, but - yes -
> essentially you have the same behaviour of the "unix" timer, plus the
> overhead of reprogramming the timer. 

Is this significant?  At a high guest HZ, this is could be quite a lot
of additional syscalls right?

> Add support for dynamic ticks.
> 
> If the the dynticks alarm timer is used qemu does not attempt to generate
> SIGALRM at a constant rate. Rather, the system timer is set to generate SIGALRM
> only when it is needed. Dynticks timer reduces the number of SIGALRMs sent to
> idle dynamic-ticked guests.
> Original patch from Dan Kenigsberg <dank-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
> 
> Signed-off-by: Luca Tettamanti <kronos.it-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---
>  vl.c |  178 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 170 insertions(+), 8 deletions(-)
> 
> Index: qemu/vl.c
> ===================================================================
> --- qemu.orig/vl.c	2007-08-18 23:23:47.000000000 +0200
> +++ qemu/vl.c	2007-08-18 23:23:53.000000000 +0200
> @@ -784,12 +784,31 @@
>  
>  struct qemu_alarm_timer {
>      char const *name;
> +    unsigned int flags;
>  
>      int (*start)(struct qemu_alarm_timer *t);
>      void (*stop)(struct qemu_alarm_timer *t);
> +    void (*rearm)(struct qemu_alarm_timer *t);
>      void *priv;
>  };
>  
> +#define ALARM_FLAG_DYNTICKS  0x1
> +
> +static inline int alarm_has_dynticks(struct qemu_alarm_timer *t)
> +{
> +    return t->flags & ALARM_FLAG_DYNTICKS;
> +}
> +
> +static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t) {

The '{' should be on the next line.

The rest looks fine.

Regards,

Anthony Liguori

> +    if (!alarm_has_dynticks(t))
> +        return;
> +
> +    t->rearm(t);
> +}
> +
> +/* TODO: MIN_TIMER_REARM_US should be optimized */
> +#define MIN_TIMER_REARM_US 250
> +
>  static struct qemu_alarm_timer *alarm_timer;
>  
>  #ifdef _WIN32
> @@ -802,12 +821,17 @@
>  
>  static int win32_start_timer(struct qemu_alarm_timer *t);
>  static void win32_stop_timer(struct qemu_alarm_timer *t);
> +static void win32_rearm_timer(struct qemu_alarm_timer *t);
>  
>  #else
>  
>  static int unix_start_timer(struct qemu_alarm_timer *t);
>  static void unix_stop_timer(struct qemu_alarm_timer *t);
>  
> +static int dynticks_start_timer(struct qemu_alarm_timer *t);
> +static void dynticks_stop_timer(struct qemu_alarm_timer *t);
> +static void dynticks_rearm_timer(struct qemu_alarm_timer *t);
> +
>  #ifdef __linux__
>  
>  static int hpet_start_timer(struct qemu_alarm_timer *t);
> @@ -816,21 +840,23 @@
>  static int rtc_start_timer(struct qemu_alarm_timer *t);
>  static void rtc_stop_timer(struct qemu_alarm_timer *t);
>  
> -#endif
> +#endif /* __linux__ */
>  
>  #endif /* _WIN32 */
>  
>  static struct qemu_alarm_timer alarm_timers[] = {
> +#ifndef _WIN32
> +    {"dynticks", ALARM_FLAG_DYNTICKS, dynticks_start_timer, dynticks_stop_timer, dynticks_rearm_timer, NULL},
>  #ifdef __linux__
>      /* HPET - if available - is preferred */
> -    {"hpet", hpet_start_timer, hpet_stop_timer, NULL},
> +    {"hpet", 0, hpet_start_timer, hpet_stop_timer, NULL, NULL},
>      /* ...otherwise try RTC */
> -    {"rtc", rtc_start_timer, rtc_stop_timer, NULL},
> +    {"rtc", 0, rtc_start_timer, rtc_stop_timer, NULL, NULL},
>  #endif
> -#ifndef _WIN32
> -    {"unix", unix_start_timer, unix_stop_timer, NULL},
> +    {"unix", 0, unix_start_timer, unix_stop_timer, NULL, NULL},
>  #else
> -    {"win32", win32_start_timer, win32_stop_timer, &alarm_win32_data},
> +    {"dynticks", ALARM_FLAG_DYNTICKS, win32_start_timer, win32_stop_timer, win32_rearm_timer, &alarm_win32_data},
> +    {"win32", 0, win32_start_timer, win32_stop_timer, NULL, &alarm_win32_data},
>  #endif
>      {NULL, }
>  };
> @@ -949,6 +975,8 @@
>          }
>          pt = &t->next;
>      }
> +
> +    qemu_rearm_alarm_timer(alarm_timer);
>  }
>  
>  /* modify the current timer so that it will be fired when current_time
> @@ -1008,6 +1036,7 @@
>          /* run the callback (the timer list can be modified) */
>          ts->cb(ts->opaque);
>      }
> +    qemu_rearm_alarm_timer(alarm_timer);
>  }
>  
>  int64_t qemu_get_clock(QEMUClock *clock)
> @@ -1115,7 +1144,8 @@
>          last_clock = ti;
>      }
>  #endif
> -    if (qemu_timer_expired(active_timers[QEMU_TIMER_VIRTUAL],
> +    if (alarm_has_dynticks(alarm_timer) ||
> +        qemu_timer_expired(active_timers[QEMU_TIMER_VIRTUAL],
>                             qemu_get_clock(vm_clock)) ||
>          qemu_timer_expired(active_timers[QEMU_TIMER_REALTIME],
>                             qemu_get_clock(rt_clock))) {
> @@ -1136,6 +1166,27 @@
>      }
>  }
>  
> +static uint64_t qemu_next_deadline(void) {
> +    uint64_t nearest_delta_us = ULLONG_MAX;
> +    uint64_t vmdelta_us;
> +
> +    if (active_timers[QEMU_TIMER_REALTIME])
> +        nearest_delta_us = (active_timers[QEMU_TIMER_REALTIME]->expire_time - qemu_get_clock(rt_clock))*1000;
> +
> +    if (active_timers[QEMU_TIMER_VIRTUAL]) {
> +        /* round up */
> +        vmdelta_us = (active_timers[QEMU_TIMER_VIRTUAL]->expire_time - qemu_get_clock(vm_clock)+999)/1000;
> +        if (vmdelta_us < nearest_delta_us)
> +            nearest_delta_us = vmdelta_us;
> +    }
> +
> +    /* Avoid arming the timer to negative, zero, or too low values */
> +    if (nearest_delta_us <= MIN_TIMER_REARM_US)
> +        nearest_delta_us = MIN_TIMER_REARM_US;
> +
> +    return nearest_delta_us;
> +}
> +
>  #ifndef _WIN32
>  
>  #if defined(__linux__)
> @@ -1243,6 +1294,80 @@
>  
>  #endif /* !defined(__linux__) */
>  
> +static int dynticks_start_timer(struct qemu_alarm_timer *t)
> +{
> +    struct sigevent ev;
> +    timer_t host_timer;
> +    struct sigaction act;
> +
> +    sigfillset(&act.sa_mask);
> +    act.sa_flags = 0;
> +#if defined(TARGET_I386) && defined(USE_CODE_COPY)
> +    act.sa_flags |= SA_ONSTACK;
> +#endif
> +    act.sa_handler = host_alarm_handler;
> +
> +    sigaction(SIGALRM, &act, NULL);
> +
> +    ev.sigev_value.sival_int = 0;
> +    ev.sigev_notify = SIGEV_SIGNAL;
> +    ev.sigev_signo = SIGALRM;
> +
> +    if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) {
> +        perror("timer_create");
> +
> +        /* disable dynticks */
> +        fprintf(stderr, "Dynamic Ticks disabled\n");
> +
> +        return -1;
> +    }
> +
> +    t->priv = (void *)host_timer;
> +
> +    return 0;
> +}
> +
> +static void dynticks_stop_timer(struct qemu_alarm_timer *t)
> +{
> +    timer_t host_timer = (timer_t)t->priv;
> +
> +    timer_delete(host_timer);
> +}
> +
> +static void dynticks_rearm_timer(struct qemu_alarm_timer *t)
> +{
> +    timer_t host_timer = (timer_t)t->priv;
> +    struct itimerspec timeout;
> +    int64_t nearest_delta_us = INT64_MAX;
> +    int64_t current_us;
> +
> +    if (!active_timers[QEMU_TIMER_REALTIME] &&
> +                !active_timers[QEMU_TIMER_VIRTUAL])
> +            return;
> +
> +    nearest_delta_us = qemu_next_deadline();
> +
> +    /* check whether a timer is already running */
> +    if (timer_gettime(host_timer, &timeout)) {
> +        perror("gettime");
> +        fprintf(stderr, "Internal timer error: aborting\n");
> +        exit(1);
> +    }
> +    current_us = timeout.it_value.tv_sec * 1000000 + timeout.it_value.tv_nsec/1000;
> +    if (current_us && current_us <= nearest_delta_us)
> +        return;
> +
> +    timeout.it_interval.tv_sec = 0;
> +    timeout.it_interval.tv_nsec = 0; /* 0 for one-shot timer */
> +    timeout.it_value.tv_sec =  nearest_delta_us / 1000000;
> +    timeout.it_value.tv_nsec = (nearest_delta_us % 1000000) * 1000;
> +    if (timer_settime(host_timer, 0 /* RELATIVE */, &timeout, NULL)) {
> +        perror("settime");
> +        fprintf(stderr, "Internal timer error: aborting\n");
> +        exit(1);
> +    }
> +}
> +
>  static int unix_start_timer(struct qemu_alarm_timer *t)
>  {
>      struct sigaction act;
> @@ -1288,6 +1413,7 @@
>  {
>      TIMECAPS tc;
>      struct qemu_alarm_win32 *data = t->priv;
> +    UINT flags;
>  
>      data->host_alarm = CreateEvent(NULL, FALSE, FALSE, NULL);
>      if (!data->host_alarm) {
> @@ -1304,11 +1430,17 @@
>  
>      timeBeginPeriod(data->period);
>  
> +    flags = TIME_CALLBACK_FUNCTION;
> +    if (alarm_has_dynticks(t))
> +        flags |= TIME_ONESHOT;
> +    else
> +        flags |= TIME_PERIODIC;
> +
>      data->timerId = timeSetEvent(1,         // interval (ms)
>                          data->period,       // resolution
>                          host_alarm_handler, // function
>                          (DWORD)t,           // parameter
> -                        TIME_PERIODIC | TIME_CALLBACK_FUNCTION);
> +                        flags);
>  
>      if (!data->timerId) {
>          perror("Failed to initialize win32 alarm timer");
> @@ -1333,6 +1465,35 @@
>      CloseHandle(data->host_alarm);
>  }
>  
> +static void win32_rearm_timer(struct qemu_alarm_timer *t)
> +{
> +    struct qemu_alarm_win32 *data = t->priv;
> +    uint64_t nearest_delta_us;
> +
> +    if (!active_timers[QEMU_TIMER_REALTIME] &&
> +                !active_timers[QEMU_TIMER_VIRTUAL])
> +            return;
> +
> +    nearest_delta_us = qemu_next_deadline();
> +    nearest_delta_us /= 1000;
> +
> +    timeKillEvent(data->timerId);
> +
> +    data->timerId = timeSetEvent(1,
> +                        data->period,
> +                        host_alarm_handler,
> +                        (DWORD)t,
> +                        TIME_ONESHOT | TIME_PERIODIC);
> +
> +    if (!data->timerId) {
> +        perror("Failed to re-arm win32 alarm timer");
> +
> +        timeEndPeriod(data->period);
> +        CloseHandle(data->host_alarm);
> +        exit(1);
> +    }
> +}
> +
>  #endif /* _WIN32 */
>  
>  static void init_timer_alarm(void)
> @@ -6491,6 +6652,7 @@
>          cpu_enable_ticks();
>          vm_running = 1;
>          vm_state_notify(1);
> +        qemu_rearm_alarm_timer(alarm_timer);
>      }
>  }
>  
> 
> Luca


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