| Message ID | 1332606390-3605-3-git-send-email-lee.essen@nowonline.co.uk |
|---|---|
| State | New |
| Headers | show |
Il 24/03/2012 17:26, Lee Essen ha scritto: > Dynticks was limited to linux. This patch adds Solaris support > and ensures a CLOCK_HIGHRES clock is used which is the optimal > setup for Solaris systems. Looks good, but I would prefer if you tested for timer_create in configure and use #ifdef CONFIG_RT_TIMER instead. > +#if defined(__sun__) > + if (timer_create(CLOCK_HIGHRES, &ev, &host_timer)) { > +#else > if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) { > +#endif This should be #ifdef CLOCK_HIGHRES. Paolo
On 2012-03-27 17:01, Paolo Bonzini wrote: > Il 24/03/2012 17:26, Lee Essen ha scritto: >> Dynticks was limited to linux. This patch adds Solaris support >> and ensures a CLOCK_HIGHRES clock is used which is the optimal >> setup for Solaris systems. > > Looks good, but I would prefer if you tested for timer_create in > configure and use #ifdef CONFIG_RT_TIMER instead. > >> +#if defined(__sun__) >> + if (timer_create(CLOCK_HIGHRES, &ev, &host_timer)) { >> +#else >> if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) { >> +#endif > > This should be #ifdef CLOCK_HIGHRES. Are we sure about this is and will remain equivalent and correct? Also, I found some man page that says CLOCK_HIGHRES is non-adjustable while CLOCK_REALTIME is. That should make a difference in QEMU. Jan
Il 27/03/2012 17:08, Jan Kiszka ha scritto: >>> +#if defined(__sun__) >>> + if (timer_create(CLOCK_HIGHRES, &ev, &host_timer)) { >>> +#else >>> if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) { >>> +#endif >> >> This should be #ifdef CLOCK_HIGHRES. > > Are we sure about this is and will remain equivalent and correct? > > Also, I found some man page that says CLOCK_HIGHRES is non-adjustable > while CLOCK_REALTIME is. That should make a difference in QEMU. Right, that's why I CCed you but then I forgot to ask the question. Does QEMU rely on CLOCK_REALTIME when "-rtc clock=host" is in use? A monotonic clock would work better when CLOCK_REALTIME jumps backwards (DST->solar). If the jump goes unnoticed, the alarm timer would have no timeout for an hour or so. Of course the opposite is true when going from solar time to DST; you move the realtime clock one hour forward and, with CLOCK_MONOTONIC, a host_clock timer to trigger an hour too late. Paolo
On 2012-03-27 17:52, Paolo Bonzini wrote: > Il 27/03/2012 17:08, Jan Kiszka ha scritto: >>>> +#if defined(__sun__) >>>> + if (timer_create(CLOCK_HIGHRES, &ev, &host_timer)) { >>>> +#else >>>> if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) { >>>> +#endif >>> >>> This should be #ifdef CLOCK_HIGHRES. >> >> Are we sure about this is and will remain equivalent and correct? >> >> Also, I found some man page that says CLOCK_HIGHRES is non-adjustable >> while CLOCK_REALTIME is. That should make a difference in QEMU. > > Right, that's why I CCed you but then I forgot to ask the question. > > Does QEMU rely on CLOCK_REALTIME when "-rtc clock=host" is in use? A > monotonic clock would work better when CLOCK_REALTIME jumps backwards > (DST->solar). If the jump goes unnoticed, the alarm timer would have no > timeout for an hour or so. > > Of course the opposite is true when going from solar time to DST; you > move the realtime clock one hour forward and, with CLOCK_MONOTONIC, a > host_clock timer to trigger an hour too late. But the latter would be fixed up when we actually check for expired timers against the proper clocks. Hmm, I'm not sure anymore if we really need CLOCK_REALTIME for the timer here. This also has no telling history. Maybe the contributor of dynticks just didn't think about this aspect of REALTIME vs. MONOTONIC. Jan
There exists a simple patch that changes the wait loop timer to use CLOCK_MONOTONIC. See https://github.com/portante/qemu/commit/35c92daa9784882153c6d8b0e15e8c8f181d6e8a . -peter On Tue, Mar 27, 2012 at 12:00 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2012-03-27 17:52, Paolo Bonzini wrote: > > Il 27/03/2012 17:08, Jan Kiszka ha scritto: > >>>> +#if defined(__sun__) > >>>> + if (timer_create(CLOCK_HIGHRES, &ev, &host_timer)) { > >>>> +#else > >>>> if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) { > >>>> +#endif > >>> > >>> This should be #ifdef CLOCK_HIGHRES. > >> > >> Are we sure about this is and will remain equivalent and correct? > >> > >> Also, I found some man page that says CLOCK_HIGHRES is non-adjustable > >> while CLOCK_REALTIME is. That should make a difference in QEMU. > > > > Right, that's why I CCed you but then I forgot to ask the question. > > > > Does QEMU rely on CLOCK_REALTIME when "-rtc clock=host" is in use? A > > monotonic clock would work better when CLOCK_REALTIME jumps backwards > > (DST->solar). If the jump goes unnoticed, the alarm timer would have no > > timeout for an hour or so. > > > > Of course the opposite is true when going from solar time to DST; you > > move the realtime clock one hour forward and, with CLOCK_MONOTONIC, a > > host_clock timer to trigger an hour too late. > > But the latter would be fixed up when we actually check for expired > timers against the proper clocks. Hmm, I'm not sure anymore if we really > need CLOCK_REALTIME for the timer here. This also has no telling > history. Maybe the contributor of dynticks just didn't think about this > aspect of REALTIME vs. MONOTONIC. > > Jan > > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux > >
diff --git a/qemu-timer.c b/qemu-timer.c index d7f56e5..48817c9 100644 --- a/qemu-timer.c +++ b/qemu-timer.c @@ -77,7 +77,7 @@ struct qemu_alarm_timer { int (*start)(struct qemu_alarm_timer *t); void (*stop)(struct qemu_alarm_timer *t); void (*rearm)(struct qemu_alarm_timer *t, int64_t nearest_delta_ns); -#if defined(__linux__) +#if defined(__linux__) || defined(__sun__) int fd; timer_t timer; #elif defined(_WIN32) @@ -165,7 +165,7 @@ static int unix_start_timer(struct qemu_alarm_timer *t); static void unix_stop_timer(struct qemu_alarm_timer *t); static void unix_rearm_timer(struct qemu_alarm_timer *t, int64_t delta); -#ifdef __linux__ +#if defined(__linux__) || defined(__sun__) static int dynticks_start_timer(struct qemu_alarm_timer *t); static void dynticks_stop_timer(struct qemu_alarm_timer *t); @@ -177,7 +177,7 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t, int64_t delta); static struct qemu_alarm_timer alarm_timers[] = { #ifndef _WIN32 -#ifdef __linux__ +#if defined(__linux__) || defined(__sun__) {"dynticks", dynticks_start_timer, dynticks_stop_timer, dynticks_rearm_timer}, #endif @@ -502,7 +502,7 @@ static void host_alarm_handler(int host_signum) } } -#if defined(__linux__) +#if defined(__linux__) || defined(__sun__) #include "compatfd.h" @@ -533,7 +533,11 @@ static int dynticks_start_timer(struct qemu_alarm_timer *t) #endif /* SIGEV_THREAD_ID */ ev.sigev_signo = SIGALRM; +#if defined(__sun__) + if (timer_create(CLOCK_HIGHRES, &ev, &host_timer)) { +#else if (timer_create(CLOCK_REALTIME, &ev, &host_timer)) { +#endif perror("timer_create"); /* disable dynticks */ @@ -585,7 +589,7 @@ static void dynticks_rearm_timer(struct qemu_alarm_timer *t, } } -#endif /* defined(__linux__) */ +#endif /* defined(__linux__) || defined(__sun__) */ #if !defined(_WIN32)
Dynticks was limited to linux. This patch adds Solaris support and ensures a CLOCK_HIGHRES clock is used which is the optimal setup for Solaris systems. Signed-off-by: Lee Essen <lee.essen@nowonline.co.uk> --- qemu-timer.c | 14 +++++++++----- 1 files changed, 9 insertions(+), 5 deletions(-)