Re: [Qemu-devel] [PATCH 1.1?] qemu_rearm_alarm_timer: do not call rearm if the next deadline is INT64_MAX

[Stefan Weil]

Am 29.05.2012 19:23, schrieb Stefano Stabellini:
> On Tue, 29 May 2012, Stefan Weil wrote:
> > Am 29.05.2012 15:35, schrieb Stefano Stabellini:
> > > qemu_rearm_alarm_timer partially duplicates the code in
> > > qemu_next_alarm_deadline to figure out if it needs to rearm the timer.
> > > If it calls qemu_next_alarm_deadline, it always rearms the timer even if
> > > the next deadline is INT64_MAX.
> > >
> > > This patch simplifies the behavior of qemu_rearm_alarm_timer and removes
> > > the duplicated code, always calling qemu_next_alarm_deadline and only
> > > rearming the timer if the deadline is less than INT64_MAX.
> > >
> > > Signed-off-by: Stefano Stabellini<address@hidden>
> > >
> > > diff --git a/qemu-timer.c b/qemu-timer.c
> > > index de98977..81ff824 100644
> > > --- a/qemu-timer.c
> > > +++ b/qemu-timer.c
> > > @@ -112,14 +112,10 @@ static int64_t qemu_next_alarm_deadline(void)
> > >
> > >    static void qemu_rearm_alarm_timer(struct qemu_alarm_timer *t)
> > >    {
> > > -    int64_t nearest_delta_ns;
> > > -    if (!rt_clock->active_timers&&
> > > -        !vm_clock->active_timers&&
> > > -        !host_clock->active_timers) {
> > > -        return;
> > > +    int64_t nearest_delta_ns = qemu_next_alarm_deadline();
> > > +    if (nearest_delta_ns<   INT64_MAX) {
> > > +        t->rearm(t, nearest_delta_ns);
> > >        }
> > > -    nearest_delta_ns = qemu_next_alarm_deadline();
> > > -    t->rearm(t, nearest_delta_ns);
> > >    }
> > >
> > >    /* TODO: MIN_TIMER_REARM_NS should be optimized */
> >
> > Reviewed-by: Stefan Weil<address@hidden>
>
> thanks
>
> > This patch clearly improves the current code and fixes
> > an abort on Darwin (reported by Andreas Färber) and maybe
> > other hosts. Therefore I changed the subject and suggest
> > to consider this patch for QEMU 1.1.
> >
> > There remain issues which can be fixed after 1.1:
> >
> > nearest_delta_ns also gets negative values (rtdelta<  0,
> > maybe because the expiration time already expired).
> > I did not check whether all different timers handle
> > a negative time gracefully.
> >
> > nearest_delta_ns should also be limited to INT32_MAX
> > seconds, because some timers assign the seconds
> > to a long (see setitimer) or UINT value. On 32 bit
> > Linux and on all variants of Windows, long is less
> > or equal INT32_MAX. If we limit nearest_delta_ns
> > to 1000000 seconds (or some other limit which allows
> > ULONG milliseconds), we could further simplify the code
> > because most timers would no longer have to test the
> > upper limit.
>
> If that's the issue we could limit nearest_delta_ns to LONG_MAX.
>
> However I got the feeling that Darwin has an undocumented limit
> for tv_sec, lower than INT32_MAX.

Yes, we could set the upper limit to LONG_MAX seconds for some
timers, but I did not want to have a dependency of the
upper limit on sizeof(long). The function win32_rearm_timer
only allows 4294967 seconds. Is there any reason why we
should allow timers which expire after more than 11 days
(1000000 seconds are about 11 days)? If there is none,
1000000 seconds would be a good upper limit for most timers
(maybe also for Darwin). mm_rearm_timer is the only timer
which then still needs its own upper limit.

Regards,
Stefan W.