[Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Jan Kiszka]

Not only clean up enabled voices but any registered one. Backends like
pulsaudio rely on unconditional fini handler invocations.

This fixes "Memory pool destroyed but not all memory blocks freed!"
warnings on VM shutdowns when pa is used.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 audio/audio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index bd9237e..4b6e06c 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1775,7 +1775,7 @@ static void audio_atexit (void)
     HWVoiceOut *hwo = NULL;
     HWVoiceIn *hwi = NULL;
 
-    while ((hwo = audio_pcm_hw_find_any_enabled_out (hwo))) {
+    while ((hwo = audio_pcm_hw_find_any_out (hwo))) {
         SWVoiceCap *sc;
 
         hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);
@@ -1791,7 +1791,7 @@ static void audio_atexit (void)
         }
     }
 
-    while ((hwi = audio_pcm_hw_find_any_enabled_in (hwi))) {
+    while ((hwi = audio_pcm_hw_find_any_in (hwi))) {
         hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);
         hwi->pcm_ops->fini_in (hwi);
     }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[malc]

On Thu, 3 May 2012, Jan Kiszka wrote:

> Not only clean up enabled voices but any registered one. Backends like
> pulsaudio rely on unconditional fini handler invocations.
> 
> This fixes "Memory pool destroyed but not all memory blocks freed!"
> warnings on VM shutdowns when pa is used.

Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Jan Kiszka]

On 2012-05-03 16:32, malc wrote:
> On Thu, 3 May 2012, Jan Kiszka wrote:
> 
>> Not only clean up enabled voices but any registered one. Backends like
>> pulsaudio rely on unconditional fini handler invocations.
>>
>> This fixes "Memory pool destroyed but not all memory blocks freed!"
>> warnings on VM shutdowns when pa is used.
> 
> Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?

This might be some additional issue (that pa is not supporting
enable/disable). In any case, it is unrelated to this one: fini
corresponds to init. And as we initialized the voice, we also have to
finalize it on shutdown. That's what this patch is fixing.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[malc]

On Thu, 3 May 2012, Jan Kiszka wrote:

> On 2012-05-03 16:32, malc wrote:
> > On Thu, 3 May 2012, Jan Kiszka wrote:
> > 
> >> Not only clean up enabled voices but any registered one. Backends like
> >> pulsaudio rely on unconditional fini handler invocations.
> >>
> >> This fixes "Memory pool destroyed but not all memory blocks freed!"
> >> warnings on VM shutdowns when pa is used.
> > 
> > Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?
> 
> This might be some additional issue (that pa is not supporting
> enable/disable). In any case, it is unrelated to this one: fini
> corresponds to init. And as we initialized the voice, we also have to
> finalize it on shutdown. That's what this patch is fixing.
> 

The issue is that i don't remember exactly why it iterates only over
enabled voices, maybe there was a reason, maybe there wasn't, need to
think it over. 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Jan Kiszka]

On 2012-05-03 20:51, malc wrote:
> On Thu, 3 May 2012, Jan Kiszka wrote:
> 
>> On 2012-05-03 16:32, malc wrote:
>>> On Thu, 3 May 2012, Jan Kiszka wrote:
>>>
>>>> Not only clean up enabled voices but any registered one. Backends like
>>>> pulsaudio rely on unconditional fini handler invocations.
>>>>
>>>> This fixes "Memory pool destroyed but not all memory blocks freed!"
>>>> warnings on VM shutdowns when pa is used.
>>>
>>> Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?
>>
>> This might be some additional issue (that pa is not supporting
>> enable/disable). In any case, it is unrelated to this one: fini
>> corresponds to init. And as we initialized the voice, we also have to
>> finalize it on shutdown. That's what this patch is fixing.
>>
> 
> The issue is that i don't remember exactly why it iterates only over
> enabled voices, maybe there was a reason, maybe there wasn't, need to
> think it over. 

Any news on this?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[malc]

On Wed, 9 May 2012, Jan Kiszka wrote:

> On 2012-05-03 20:51, malc wrote:
> > On Thu, 3 May 2012, Jan Kiszka wrote:
> > 
> >> On 2012-05-03 16:32, malc wrote:
> >>> On Thu, 3 May 2012, Jan Kiszka wrote:
> >>>
> >>>> Not only clean up enabled voices but any registered one. Backends like
> >>>> pulsaudio rely on unconditional fini handler invocations.
> >>>>
> >>>> This fixes "Memory pool destroyed but not all memory blocks freed!"
> >>>> warnings on VM shutdowns when pa is used.
> >>>
> >>> Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?
> >>
> >> This might be some additional issue (that pa is not supporting
> >> enable/disable). In any case, it is unrelated to this one: fini
> >> corresponds to init. And as we initialized the voice, we also have to
> >> finalize it on shutdown. That's what this patch is fixing.
> >>
> > 
> > The issue is that i don't remember exactly why it iterates only over
> > enabled voices, maybe there was a reason, maybe there wasn't, need to
> > think it over. 
> 
> Any news on this?
> 

Nope.

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Jan Kiszka]

On 2012-05-09 16:33, malc wrote:
> On Wed, 9 May 2012, Jan Kiszka wrote:
> 
>> On 2012-05-03 20:51, malc wrote:
>>> On Thu, 3 May 2012, Jan Kiszka wrote:
>>>
>>>> On 2012-05-03 16:32, malc wrote:
>>>>> On Thu, 3 May 2012, Jan Kiszka wrote:
>>>>>
>>>>>> Not only clean up enabled voices but any registered one. Backends like
>>>>>> pulsaudio rely on unconditional fini handler invocations.
>>>>>>
>>>>>> This fixes "Memory pool destroyed but not all memory blocks freed!"
>>>>>> warnings on VM shutdowns when pa is used.
>>>>>
>>>>> Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?
>>>>
>>>> This might be some additional issue (that pa is not supporting
>>>> enable/disable). In any case, it is unrelated to this one: fini
>>>> corresponds to init. And as we initialized the voice, we also have to
>>>> finalize it on shutdown. That's what this patch is fixing.
>>>>
>>>
>>> The issue is that i don't remember exactly why it iterates only over
>>> enabled voices, maybe there was a reason, maybe there wasn't, need to
>>> think it over. 
>>
>> Any news on this?
>>
> 
> Nope.

Can we please get this or similar fix in 1.1.? It not only cures the
annoying warning, I just noticed that, without it, the qemu process
hangs like this on exit when audio was in use by the guest:

(gdb) bt
#0  0x00007f4c2f01b5f0 in sem_wait () from /lib64/libpthread.so.0
#1  0x00007f4c2a695f58 in pa_semaphore_wait () from
/usr/lib64/libpulsecommon-0.9.22.so
#2  0x00007f4c2a67d0a4 in ?? () from /usr/lib64/libpulsecommon-0.9.22.so
#3  0x00007f4c2a67d1ab in ?? () from /usr/lib64/libpulsecommon-0.9.22.so
#4  0x00007f4c2a67fa50 in pa_memimport_free () from
/usr/lib64/libpulsecommon-0.9.22.so
#5  0x00007f4c2a688930 in pa_pstream_unlink () from
/usr/lib64/libpulsecommon-0.9.22.so
#6  0x00007f4c2de1a3ea in ?? () from /usr/lib64/libpulse.so.0
#7  0x00007f4c2de1ad58 in ?? () from /usr/lib64/libpulse.so.0
#8  0x00007f4c30be1aa4 in qpa_audio_fini (opaque=0x7f4c311a98e0) at
/data/qemu-kvm/audio/paaudio.c:875
#9  0x00007f4c2b9fc5a1 in __run_exit_handlers () from /lib64/libc.so.6
#10 0x00007f4c2b9fc5f5 in exit () from /lib64/libc.so.6
#11 0x00007f4c2b9e5c04 in __libc_start_main () from /lib64/libc.so.6
#12 0x00007f4c30bd5219 in _start () at ../sysdeps/x86_64/elf/start.S:103

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Gerd Hoffmann]

On 05/03/12 21:32, malc wrote:
> On Thu, 3 May 2012, Jan Kiszka wrote:
> 
>> Not only clean up enabled voices but any registered one. Backends like
>> pulsaudio rely on unconditional fini handler invocations.
>>
>> This fixes "Memory pool destroyed but not all memory blocks freed!"
>> warnings on VM shutdowns when pa is used.
> 
> Perhaps it's better to actually handle VOICE_DISABLE in pa's ctl_[in|out]?
> 
> [..snip..]

Waded through the source.  Have to agree with Jan here: Handling this
via VOICE_{ENABLE,DISABLE} isn't going to fly.  It would mean to move
the complete pulseaudio setup (connect to daemon, create streams, create
worker thread, ...).from init() to VOICE_ENABLE so we can cleanup in
VOICE_DISABLE and don't need fini().  This isn't how it is supposed to
work ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 1.1] audio: Always call fini on exit

[Gerd Hoffmann]

On 05/03/12 20:56, Jan Kiszka wrote:
> Not only clean up enabled voices but any registered one. Backends like
> pulsaudio rely on unconditional fini handler invocations.
> 
> This fixes "Memory pool destroyed but not all memory blocks freed!"
> warnings on VM shutdowns when pa is used.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  audio/audio.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/audio/audio.c b/audio/audio.c
> index bd9237e..4b6e06c 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1775,7 +1775,7 @@ static void audio_atexit (void)
>      HWVoiceOut *hwo = NULL;
>      HWVoiceIn *hwi = NULL;
>  
> -    while ((hwo = audio_pcm_hw_find_any_enabled_out (hwo))) {
> +    while ((hwo = audio_pcm_hw_find_any_out (hwo))) {
>          SWVoiceCap *sc;
>  
>          hwo->pcm_ops->ctl_out (hwo, VOICE_DISABLE);

I think you better call VOICE_DISABLE only in case the voice is actually
enabled.

> @@ -1791,7 +1791,7 @@ static void audio_atexit (void)
>          }
>      }
>  
> -    while ((hwi = audio_pcm_hw_find_any_enabled_in (hwi))) {
> +    while ((hwi = audio_pcm_hw_find_any_in (hwi))) {
>          hwi->pcm_ops->ctl_in (hwi, VOICE_DISABLE);

Likewise.

>          hwi->pcm_ops->fini_in (hwi);
>      }

cheers,
  Gerd