[Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Hans de Goede]

Now that we no longer have MIN_REARM_TIMER_NS a bug in the audio subsys has
clearly shown it self by trying to make a timer fire every nano second.

Note we have a similar problem in 1.6, 1.5 and older but there
MIN_REARM_TIMER_NS limits the wakeups caused by audio being active to
4000 times / second. This still causes a host cpu load of 50 % for simply
playing audio, where as with this patch git master is at 13%, so we should
backport this to 1.5 and 1.6 too.

Note this will not apply to 1.5 and 1.6 as is.

Cc: qemu-stable@nongnu.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 audio/audio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/audio/audio.c b/audio/audio.c
index af4cdf6..b3db679 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1124,7 +1124,8 @@ static int audio_is_timer_needed (void)
 static void audio_reset_timer (AudioState *s)
 {
     if (audio_is_timer_needed ()) {
-        timer_mod (s->ts, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1);
+        timer_mod (s->ts,
+            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
     }
     else {
         timer_del (s->ts);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] audio: Lower default wakeup rate to 100 times / second

[Hans de Goede]

This is more then plenty to keep audio card fifos filles / emptied.

This drops host cpu-load for audio playback inside a linux vm from
13% to 9%.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 audio/audio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/audio/audio.c b/audio/audio.c
index b3db679..fc77511 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -95,7 +95,7 @@ static struct {
         }
     },
 
-    .period = { .hertz = 250 },
+    .period = { .hertz = 100 },
     .plive = 0,
     .log_to_monitor = 0,
     .try_poll_in = 1,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Alex Bligh]

On 9 Oct 2013, at 20:42, Hans de Goede wrote:

> Now that we no longer have MIN_REARM_TIMER_NS a bug in the audio subsys has
> clearly shown it self by trying to make a timer fire every nano second.
> 
> Note we have a similar problem in 1.6, 1.5 and older but there
> MIN_REARM_TIMER_NS limits the wakeups caused by audio being active to
> 4000 times / second. This still causes a host cpu load of 50 % for simply
> playing audio, where as with this patch git master is at 13%, so we should
> backport this to 1.5 and 1.6 too.
> 
> Note this will not apply to 1.5 and 1.6 as is.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> audio/audio.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index af4cdf6..b3db679 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1124,7 +1124,8 @@ static int audio_is_timer_needed (void)
> static void audio_reset_timer (AudioState *s)
> {
>     if (audio_is_timer_needed ()) {
> -        timer_mod (s->ts, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1);
> +        timer_mod (s->ts,
> +            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);

This assumes conf.period.ticks is in nanoseconds. That seems wrong.
Suggest multiplying by SCALE_US or SCALE_MS.

Alex

>     }
>     else {
>         timer_del (s->ts);
> -- 
> 1.8.3.1
> 
> 
> 

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Hans de Goede]

Hi,

On 10/10/2013 08:31 AM, Alex Bligh wrote:
>
> On 9 Oct 2013, at 20:42, Hans de Goede wrote:
>
>> Now that we no longer have MIN_REARM_TIMER_NS a bug in the audio subsys has
>> clearly shown it self by trying to make a timer fire every nano second.
>>
>> Note we have a similar problem in 1.6, 1.5 and older but there
>> MIN_REARM_TIMER_NS limits the wakeups caused by audio being active to
>> 4000 times / second. This still causes a host cpu load of 50 % for simply
>> playing audio, where as with this patch git master is at 13%, so we should
>> backport this to 1.5 and 1.6 too.
>>
>> Note this will not apply to 1.5 and 1.6 as is.
>>
>> Cc: qemu-stable@nongnu.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> audio/audio.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/audio/audio.c b/audio/audio.c
>> index af4cdf6..b3db679 100644
>> --- a/audio/audio.c
>> +++ b/audio/audio.c
>> @@ -1124,7 +1124,8 @@ static int audio_is_timer_needed (void)
>> static void audio_reset_timer (AudioState *s)
>> {
>>      if (audio_is_timer_needed ()) {
>> -        timer_mod (s->ts, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1);
>> +        timer_mod (s->ts,
>> +            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
>
> This assumes conf.period.ticks is in nanoseconds. That seems wrong.
> Suggest multiplying by SCALE_US or SCALE_MS.

Which it is, quoting from higher up in the same file:

         conf.period.ticks =
             muldiv64 (1, get_ticks_per_sec (), conf.period.hertz);

And get_ticks_per_sec () returns ns .

Regards,

Hans



>
> Alex
>
>>      }
>>      else {
>>          timer_del (s->ts);
>> --
>> 1.8.3.1
>>
>>
>>
>

Re: [Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Alex Bligh]

On 10 Oct 2013, at 07:58, Hans de Goede wrote:

> Which it is, quoting from higher up in the same file:
> 
>        conf.period.ticks =
>            muldiv64 (1, get_ticks_per_sec (), conf.period.hertz);
> 
> And get_ticks_per_sec () returns ns .

Doh! I confused .hertz & .ticks.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Hans de Goede]

Hi,

On 9 Oct 2013, at 20:42, Hans de Goede wrote:
>
> Now that we no longer have MIN_REARM_TIMER_NS a bug in the audio subsys has
> clearly shown it self by trying to make a timer fire every nano second.
>
> Note we have a similar problem in 1.6, 1.5 and older but there
> MIN_REARM_TIMER_NS limits the wakeups caused by audio being active to
> 4000 times / second. This still causes a host cpu load of 50 % for simply
> playing audio, where as with this patch git master is at 13%, so we should
> backport this to 1.5 and 1.6 too.

I'm still not sure when this actually started happening, but looking at
RHEL-6 qemu sources to see if that has the issue too, I've learned how
this problem was introduced, the audio_timer callback used to do this:

     qemu_mod_timer (s->ts, qemu_get_clock (vm_clock) + conf.period.ticks);

instead of calling audio_reset_timer(), so in the past there were 2 mod_timer
calls, one from audio_reset_timer(), which scheduled the callback to run
ASAP, and one from the audio_timer callback honering conf.period.hertz.

Then at some point the qemu_mod_timer call in audio_timer was replaced
with calling audio_reset_timer() and we got the problem my patch fixes.

Regards,

Hans

Re: [Qemu-devel] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Paolo Bonzini]

Il 10/10/2013 11:23, Hans de Goede ha scritto:
> Hi,
> 
> On 9 Oct 2013, at 20:42, Hans de Goede wrote:
>>
>> Now that we no longer have MIN_REARM_TIMER_NS a bug in the audio
>> subsys has
>> clearly shown it self by trying to make a timer fire every nano second.
>>
>> Note we have a similar problem in 1.6, 1.5 and older but there
>> MIN_REARM_TIMER_NS limits the wakeups caused by audio being active to
>> 4000 times / second. This still causes a host cpu load of 50 % for simply
>> playing audio, where as with this patch git master is at 13%, so we
>> should
>> backport this to 1.5 and 1.6 too.
> 
> I'm still not sure when this actually started happening, but looking at
> RHEL-6 qemu sources to see if that has the issue too, I've learned how
> this problem was introduced, the audio_timer callback used to do this:
> 
>     qemu_mod_timer (s->ts, qemu_get_clock (vm_clock) + conf.period.ticks);
> 
> instead of calling audio_reset_timer(), so in the past there were 2
> mod_timer
> calls, one from audio_reset_timer(), which scheduled the callback to run
> ASAP, and one from the audio_timer callback honering conf.period.hertz.
> 
> Then at some point the qemu_mod_timer call in audio_timer was replaced
> with calling audio_reset_timer() and we got the problem my patch fixes.

The first broken version seems to be 0.14.0:

commit 39deb1e496de81957167daebf5cf5d1fbd5e47c2
Author: malc <av1474@comtv.ru>
Date:   Thu Nov 18 14:30:12 2010 +0300

    audio: Only use audio timer when necessary

    Originally proposed by Gerd Hoffmann.

    Signed-off-by: malc <av1474@comtv.ru>
    Acked-by: Gerd Hoffmann <kraxel@redhat.com>

Re: [Qemu-devel] [Qemu-stable] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Michael Roth]

Quoting Hans de Goede (2013-10-09 14:42:37)
> Now that we no longer have MIN_REARM_TIMER_NS a bug in the audio subsys has
> clearly shown it self by trying to make a timer fire every nano second.
> 
> Note we have a similar problem in 1.6, 1.5 and older but there
> MIN_REARM_TIMER_NS limits the wakeups caused by audio being active to
> 4000 times / second. This still causes a host cpu load of 50 % for simply
> playing audio, where as with this patch git master is at 13%, so we should
> backport this to 1.5 and 1.6 too.
> 
> Note this will not apply to 1.5 and 1.6 as is.

What needs to be changed? Wouldn't this patch also restore the 250hz
frequency for 1.6, as it was pre-0.14?

> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  audio/audio.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index af4cdf6..b3db679 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1124,7 +1124,8 @@ static int audio_is_timer_needed (void)
>  static void audio_reset_timer (AudioState *s)
>  {
>      if (audio_is_timer_needed ()) {
> -        timer_mod (s->ts, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 1);
> +        timer_mod (s->ts,
> +            qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + conf.period.ticks);
>      }
>      else {
>          timer_del (s->ts);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [Qemu-stable] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Alex Bligh]

--On 3 December 2013 14:03:04 -0600 Michael Roth 
<mdroth@linux.vnet.ibm.com> wrote:

>> Note this will not apply to 1.5 and 1.6 as is.
>
> What needs to be changed? Wouldn't this patch also restore the 250hz
> frequency for 1.6, as it was pre-0.14?

The function names have changed as have the clock names.
At a guess (cut and paste diff, completely untested), you
need something like this:

diff --git a/audio/audio.c b/audio/audio.c
index 02bb886..45d146e 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1124,7 +1124,8 @@ static int audio_is_timer_needed (void)
 static void audio_reset_timer (AudioState *s)
 {
     if (audio_is_timer_needed ()) {
-        qemu_mod_timer (s->ts, qemu_get_clock_ns (vm_clock) + 1);
+        qemu_mod_timer (s->ts, qemu_get_clock_ns (vm_clock) +
+                        conf.period.ticks);
     }
     else {
         qemu_del_timer (s->ts);

-- 
Alex Bligh

Re: [Qemu-devel] [Qemu-stable] [PATCH 1/2] audio: honor QEMU_AUDIO_TIMER_PERIOD instead of waking up every *nano* second

[Michael Roth]

Quoting Alex Bligh (2013-12-03 15:17:11)
> --On 3 December 2013 14:03:04 -0600 Michael Roth 
> <mdroth@linux.vnet.ibm.com> wrote:
> 
> >> Note this will not apply to 1.5 and 1.6 as is.
> >
> > What needs to be changed? Wouldn't this patch also restore the 250hz
> > frequency for 1.6, as it was pre-0.14?
> 
> The function names have changed as have the clock names.
> At a guess (cut and paste diff, completely untested), you
> need something like this:
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index 02bb886..45d146e 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1124,7 +1124,8 @@ static int audio_is_timer_needed (void)
>  static void audio_reset_timer (AudioState *s)
>  {
>      if (audio_is_timer_needed ()) {
> -        qemu_mod_timer (s->ts, qemu_get_clock_ns (vm_clock) + 1);
> +        qemu_mod_timer (s->ts, qemu_get_clock_ns (vm_clock) +
> +                        conf.period.ticks);
>      }
>      else {
>          qemu_del_timer (s->ts);

Ah, thanks, this looks correct. Kept staring at 1.6/1.7 versions and wasn't
seeing a difference for some reason...

> 
> -- 
> Alex Bligh