| Message ID | 1381347758-5016-1-git-send-email-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 > > >
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 >> >> >> >
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.
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
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>
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
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);
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(-)