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

Anthony Liguori
2007-08-18T23:58:25Z

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