[Qemu-devel] paaudio race condition on close

[Qemu-devel] paaudio race condition on close

[Peter Maydell]

There seems to be a race condition in the pulseaudio backend on
closing of a voice...

There are two threads involved here. The first one is a worker
thread that just sits executing our qpa_thread_in() function
to get input audio from PA and feed it to us. The second thread
is QEMU itself (the cpu thread in the case of TCG), which calls
AUD_close_in():

#0  0x00007ffff054466b in pthread_join (threadid=140736709240576,
thread_return=0x7fffdfdcc130)
    at pthread_join.c:92
#1  0x00005555557def8f in audio_pt_join (p=0x7fffbc0195b8, arg=0x7fffdfdcc178,
    cap=0x555555b0dfd5 <__FUNCTION__.21892> "qpa_fini_in")
    at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_pt_int.c:166
#2  0x00005555557ddec7 in qpa_fini_in (hw=0x7fffbc019520)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/paaudio.c:689
#3  0x00005555557d3cab in audio_pcm_hw_gc_in (hwp=0x7fffbc032c28)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_template.h:196
#4  0x00005555557d432a in audio_close_in (sw=0x7fffbc032bd0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_template.h:375
#5  0x00005555557d43bc in AUD_close_in (card=0x555556a29a08, sw=0x7fffbc032bd0)
    at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_template.h:387

qpa_fini_in() basically sets a "done" flag in the PAVoiceIn struct,
and prods the worker thread to make sure it exits before continuing.
Usually this works. Unfortunately if the input thread is in the
middle of actually writing to the buffer then things can go pear
shaped, because the sample buffer itself is freed earlier, by
audio_pcm_hw_gc_in():

        glue (s->nb_hw_voices_, TYPE) += 1;
        glue (audio_pcm_hw_free_resources_ ,TYPE) (hw);
        glue (hw->pcm_ops->fini_, TYPE) (hw);

So we end up freeing hw resources (like the conv_buf) before we
give the pulseaudio backend a chance to actually shut down its
helper thread.

Does anybody know how this is supposed to work? Is the fix as
simple as just moving the free_resources call below the fini?

thanks
-- PMM

Re: [Qemu-devel] paaudio race condition on close

[Markus Armbruster]

If Gerd (cc'ed) doesn't know, then I'm afraid nobody around here does.

Peter Maydell <peter.maydell@linaro.org> writes:

> There seems to be a race condition in the pulseaudio backend on
> closing of a voice...
>
> There are two threads involved here. The first one is a worker
> thread that just sits executing our qpa_thread_in() function
> to get input audio from PA and feed it to us. The second thread
> is QEMU itself (the cpu thread in the case of TCG), which calls
> AUD_close_in():
>
> #0  0x00007ffff054466b in pthread_join (threadid=140736709240576,
> thread_return=0x7fffdfdcc130)
>     at pthread_join.c:92
> #1  0x00005555557def8f in audio_pt_join (p=0x7fffbc0195b8, arg=0x7fffdfdcc178,
>     cap=0x555555b0dfd5 <__FUNCTION__.21892> "qpa_fini_in")
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_pt_int.c:166
> #2  0x00005555557ddec7 in qpa_fini_in (hw=0x7fffbc019520)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/paaudio.c:689
> #3  0x00005555557d3cab in audio_pcm_hw_gc_in (hwp=0x7fffbc032c28)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_template.h:196
> #4  0x00005555557d432a in audio_close_in (sw=0x7fffbc032bd0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_template.h:375
> #5  0x00005555557d43bc in AUD_close_in (card=0x555556a29a08, sw=0x7fffbc032bd0)
>     at /home/petmay01/linaro/qemu-from-laptop/qemu/audio/audio_template.h:387
>
> qpa_fini_in() basically sets a "done" flag in the PAVoiceIn struct,
> and prods the worker thread to make sure it exits before continuing.
> Usually this works. Unfortunately if the input thread is in the
> middle of actually writing to the buffer then things can go pear
> shaped, because the sample buffer itself is freed earlier, by
> audio_pcm_hw_gc_in():
>
>         glue (s->nb_hw_voices_, TYPE) += 1;
>         glue (audio_pcm_hw_free_resources_ ,TYPE) (hw);
>         glue (hw->pcm_ops->fini_, TYPE) (hw);
>
> So we end up freeing hw resources (like the conv_buf) before we
> give the pulseaudio backend a chance to actually shut down its
> helper thread.
>
> Does anybody know how this is supposed to work? Is the fix as
> simple as just moving the free_resources call below the fini?
>
> thanks
> -- PMM

Re: [Qemu-devel] paaudio race condition on close

[Gerd Hoffmann]

  Hi,

> > Usually this works. Unfortunately if the input thread is in the
> > middle of actually writing to the buffer then things can go pear
> > shaped, because the sample buffer itself is freed earlier, by
> > audio_pcm_hw_gc_in():
> >
> >         glue (s->nb_hw_voices_, TYPE) += 1;
> >         glue (audio_pcm_hw_free_resources_ ,TYPE) (hw);
> >         glue (hw->pcm_ops->fini_, TYPE) (hw);
> >
> > So we end up freeing hw resources (like the conv_buf) before we
> > give the pulseaudio backend a chance to actually shut down its
> > helper thread.
> >
> > Does anybody know how this is supposed to work? Is the fix as
> > simple as just moving the free_resources call below the fini?

I suspect the code simply wasn't written with threads in mind and the
fix actually is that simple ...

The submitted patch looks fine to me.  Do you just commit it yourself?
I don't have any other audio stuff pending atm.

cheers,
  Gerd

Re: [Qemu-devel] paaudio race condition on close

[Peter Maydell]

On 15 December 2014 at 09:23, Gerd Hoffmann <kraxel@redhat.com> wrote:

>> > Does anybody know how this is supposed to work? Is the fix as
>> > simple as just moving the free_resources call below the fini?
>
> I suspect the code simply wasn't written with threads in mind and the
> fix actually is that simple ...
>
> The submitted patch looks fine to me.

Can I take that as a reviewed-by? :-)

> Do you just commit it yourself?
> I don't have any other audio stuff pending atm.

I'm happy to put it in via target-arm (the board where I
found this, musicpal, is an ARM one).

thanks
-- PMM

Re: [Qemu-devel] paaudio race condition on close

[Gerd Hoffmann]

On Mo, 2014-12-15 at 15:37 +0000, Peter Maydell wrote:
> On 15 December 2014 at 09:23, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >> > Does anybody know how this is supposed to work? Is the fix as
> >> > simple as just moving the free_resources call below the fini?
> >
> > I suspect the code simply wasn't written with threads in mind and the
> > fix actually is that simple ...
> >
> > The submitted patch looks fine to me.
> 
> Can I take that as a reviewed-by? :-)

Sure.

> 
> > Do you just commit it yourself?
> > I don't have any other audio stuff pending atm.
> 
> I'm happy to put it in via target-arm (the board where I
> found this, musicpal, is an ARM one).

Ok.

cheers,
  Gerd