From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:38843)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1V2dMX-0006Cn-Iq
	for qemu-devel@nongnu.org; Fri, 26 Jul 2013 04:27:00 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1V2dMV-0006ts-O7
	for qemu-devel@nongnu.org; Fri, 26 Jul 2013 04:26:57 -0400
Received: from mail-ee0-x232.google.com ([2a00:1450:4013:c00::232]:48436)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1V2dMV-0006tS-Gw
	for qemu-devel@nongnu.org; Fri, 26 Jul 2013 04:26:55 -0400
Received: by mail-ee0-f50.google.com with SMTP id d49so1429094eek.9
	for <qemu-devel@nongnu.org>; Fri, 26 Jul 2013 01:26:54 -0700 (PDT)
Date: Fri, 26 Jul 2013 10:26:51 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130726082651.GC31438@stefanha-thinkpad.redhat.com>
References: <1E8E204.8000201@redhat.com>
	<1374343603-29183-1-git-send-email-alex@alex.org.uk>
	<1374343603-29183-3-git-send-email-alex@alex.org.uk>
	<20130725091929.GD21033@stefanha-thinkpad.redhat.com>
	<660041A85B474BE86E0EA054@nimrod.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <660041A85B474BE86E0EA054@nimrod.local>
Subject: Re: [Qemu-devel] [PATCHv2] [RFC 2/7] aio / timers: qemu-timer.c
 utility functions and add list of clocks
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alex Bligh <alex@alex.org.uk>
Cc: Kevin Wolf <kwolf@redhat.com>, Anthony Liguori <aliguori@us.ibm.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, rth@twiddle.net

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