Re: [Qemu-devel] [PATCHv2] [RFC 2/7] aio / timers: qemu-timer.c utility functions and add list of clocks

[Stefan Hajnoczi]

On Thu, Jul 25, 2013 at 10:46:18AM +0100, Alex Bligh wrote:
> >>@@ -61,6 +71,15 @@ int64_t cpu_get_ticks(void);
> >> void cpu_enable_ticks(void);
> >> void cpu_disable_ticks(void);
> >>
> >>+static inline int64_t qemu_soonest_timeout(int64_t timeout1, int64_t
> >>timeout2) +{
> >>+    /* we can abuse the fact that -1 (which means infinite) is a maximal
> >>+     * value when cast to unsigned. As this is disgusting, it's kept in
> >>+     * one inline function.
> >>+     */
> >>+    return ((uint64_t) timeout1 < (uint64_t) timeout2) ? timeout1 :
> >>timeout2;
> >
> >The straightforward version isn't much longer than the commented casting
> >trick:
> >
> >if (timeout1 == -1) {
> >    return timeout2;
> >} else if (timeout2 == -1) {
> >    return timeout1;
> >} else {
> >    return timeout1 < timeout2 ? timeout1 : timeout2;
> >}
> 
> Well, it should be (timeout1 < 0) for consistency. It may be a micro
> optimisation but I'm pretty sure the casting trick will produce better
> code. With the comment, it's arguably more readable too.

Seems like a compiler could be smart enough to use unsigned
instructions.  Seems like a ">> 9" vs "/ 512" micro-optimization to me.

> >>+void qemu_free_clock(QEMUClock *clock)
> >>+{
> >>+    QLIST_REMOVE(clock, list);
> >>+    g_free(clock);
> >
> >assert that there are no timers left?
> 
> Yes I wasn't quite sure of the right semantics here as no clocks are
> currently ever destroyed. I'm not quite sure how we know all timers
> are destroyed when an AioContext is destroyed. Should I go and manually
> free them or assert the right way?

It is not possible to free them since their owner still holds a pointer.
That is why I'd use an assert.  The code needs to be written so that
timers are destroyed before the clock is destroyed.

Stefan