From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTypq-0003ZS-6A for qemu-devel@nongnu.org; Wed, 09 Oct 2013 14:50:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VTypk-0007M3-5s for qemu-devel@nongnu.org; Wed, 09 Oct 2013 14:50:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:65117) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VTypj-0007LT-SP for qemu-devel@nongnu.org; Wed, 09 Oct 2013 14:50:08 -0400 Message-ID: <5255A554.1070105@redhat.com> Date: Wed, 09 Oct 2013 20:49:56 +0200 From: Hans de Goede MIME-Version: 1.0 References: <1381259403-7386-1-git-send-email-hdegoede@redhat.com> <52545950.5070403@redhat.com> <52545B44.70005@redhat.com> <28EE4224-856B-4DC1-8159-A0C274BD269E@alex.org.uk> <52546000.6070308@redhat.com> <6CD4D4EE-341E-48D0-98F4-D55C0D3922D4@alex.org.uk> <52546832.9040900@redhat.com> <5254701B.5070804@redhat.com> <525552D9.90407@redhat.com> <2451DF8D-DD3E-4875-B6D6-CEF66A763D7E@alex.org.uk> <52559A8A.2070201@redhat.com> <30CAC247-C396-4991-804B-DD0A0FE68674@alex.org.uk> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] main-loop: Don't lock starve io-threads when main_loop_tlg has pending events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh , Paolo Bonzini Cc: qemu-devel@nongnu.org Hi, On 10/09/2013 08:36 PM, Alex Bligh wrote: > > On 9 Oct 2013, at 19:28, Alex Bligh wrote: > >>> >>> static void audio_reset_timer (AudioState *s) >>> { >>> if (audio_is_timer_needed ()) { >>> timer_mod (s->ts, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1); >>> } >>> else { >>> timer_del (s->ts); >>> } >>> } >>> >>> static void audio_timer (void *opaque) >>> { >>> audio_run ("timer"); >>> audio_reset_timer (opaque); >>> } >>> >>> Note how that is using a timer which expires every freaking nano second, >>> I think it is very likely that is the culprit. >> >> Indeed. I am hoping that it is not my automated perl conversion code that >> did that, because if it is, it may have broken other stuff :-/ >> >> Thanks for finding this - let me see whether the bug existed before >> my automated changes commit. > > I think this was broken prior to my changes. Here's audio/audio.c before > my changes: > > static void audio_reset_timer (AudioState *s) > { > if (audio_is_timer_needed ()) { > qemu_mod_timer (s->ts, qemu_get_clock_ns (vm_clock) + 1); > } > else { > qemu_del_timer (s->ts); > } > } > > > Now qemu_get_clock_ns can only return something in nanoseconds, so it's > adding 1 nanosecond. This is thus either broken because: > 1. ts->scale is in nanoseconds, in which case that timer expires in > one nano-second's time; or > 2. ts->scale is not in nanoseconds, in which case nanosecond VM clock > is going to be a huge time in the future, and it's never going > to expire. > > I wonder whether it's meant to be 1 millisecond or 1 microsecond? Maybe once it was 1 ms, this code just exists to keep the buffers of a soundcard filled / emptied in time. 100 times / second is more then plenty for that, so that is what I'm going to use in the patch I'm about to submit. Regards, Hans p.s. I still think we should unlock the io-thread in the main_loop_wait when called with nonblocking == 0, even if there are expired timers. Any suggestions on how to best do that ?