From: Marc-André Lureau <marcandre.lureau@redhat.com>

Closing and opening a stream too quickly during reconfiguration create
issues with Spice.

Note: the issue with Spice has been there before and still is. When the
audio stream is recreated, for example when using
`out.mixing-engine=false`.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2639
Fixes: 6d6e23361f ("hw/audio/hda: fix memory leak on audio setup")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/audio/hda-codec.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index bc661504cf..b734a5c718 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -502,7 +502,15 @@ static void hda_audio_setup(HDAAudioStream *st)
     trace_hda_audio_format(st->node->name, st->as.nchannels,
                            fmt2name[st->as.fmt], st->as.freq);
 
-    hda_close_stream(st->state, st);
+    /*
+     * Do not hda_close_stream(st->state, st), AUD_open_() handles the logic for
+     * fixed_settings, and same format. This helps prevent race issues in Spice
+     * server & client code too. (see #2639)
+     */
+    if (use_timer) {
+        timer_free(st->buft);
+        st->buft = NULL;
+    }
     if (st->output) {
         if (use_timer) {
             cb = hda_audio_output_cb;
-- 
2.47.0

On 11/5/24 09:32, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Closing and opening a stream too quickly during reconfiguration create
> issues with Spice.
> 
> Note: the issue with Spice has been there before and still is. When the
> audio stream is recreated, for example when using
> `out.mixing-engine=false`.
> 
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2639
> Fixes: 6d6e23361f ("hw/audio/hda: fix memory leak on audio setup")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/audio/hda-codec.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index bc661504cf..b734a5c718 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -502,7 +502,15 @@ static void hda_audio_setup(HDAAudioStream *st)
>       trace_hda_audio_format(st->node->name, st->as.nchannels,
>                              fmt2name[st->as.fmt], st->as.freq);
>   
> -    hda_close_stream(st->state, st);
> +    /*
> +     * Do not hda_close_stream(st->state, st), AUD_open_() handles the logic for
> +     * fixed_settings, and same format. This helps prevent race issues in Spice
> +     * server & client code too. (see #2639)
> +     */
> +    if (use_timer) {
> +        timer_free(st->buft);
> +        st->buft = NULL;
> +    }

Looks like we raced on sending a fix.  I don't like freeing and 
recreating the timer...  all you need is timer_del(), the callback 
cannot change and the timer can be created close to where st->output is set.

Paolo

Hi Paolo

Did you see that patch? What do you say about it?

On Tue, Nov 5, 2024 at 12:33 PM <marcandre.lureau@redhat.com> wrote:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Closing and opening a stream too quickly during reconfiguration create
> issues with Spice.
>
> Note: the issue with Spice has been there before and still is. When the
> audio stream is recreated, for example when using
> `out.mixing-engine=false`.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/2639
> Fixes: 6d6e23361f ("hw/audio/hda: fix memory leak on audio setup")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/audio/hda-codec.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
> index bc661504cf..b734a5c718 100644
> --- a/hw/audio/hda-codec.c
> +++ b/hw/audio/hda-codec.c
> @@ -502,7 +502,15 @@ static void hda_audio_setup(HDAAudioStream *st)
>      trace_hda_audio_format(st->node->name, st->as.nchannels,
>                             fmt2name[st->as.fmt], st->as.freq);
>
> -    hda_close_stream(st->state, st);
> +    /*
> +     * Do not hda_close_stream(st->state, st), AUD_open_() handles the
> logic for
> +     * fixed_settings, and same format. This helps prevent race issues in
> Spice
> +     * server & client code too. (see #2639)
> +     */
> +    if (use_timer) {
> +        timer_free(st->buft);
> +        st->buft = NULL;
> +    }
>      if (st->output) {
>          if (use_timer) {
>              cb = hda_audio_output_cb;
> --
> 2.47.0
>
>
>

-- 
Marc-André Lureau

On Thu, 14 Nov 2024 at 12:57, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi Paolo
>
> Did you see that patch? What do you say about it?

I think Paolo's patch seems like a cleaner fix for
the leak than this one.

Incidentally, to recap something I said on IRC: it seems
to me like part of the problem here is that our AUD_*
API doesn't match up well with what the Spice audio
backend in QEMU needs. Specifically, the Spice channel
(if I read the code correctly) always uses a fixed
sample format and frequency. So even if the guest
reconfigures the audio device with a different frequency
there's no need to tear down the Spice audio channel.

Awkwardly, our AUD_ layer APIs provide no way for the hda
device model to say "this is just a change of the config
parameters" -- all it can do is AUD_close-then-AUD_open.
And at the Spice backend layer there's no way to tell
"this is an AUD_close because we're really finished with
audio" from "this is an AUD_close that's about to be
followed by an AUD_open because it's just a config change".

Even if the spice client end ought to cope better
with the server end closing the audio connection, it's
definitely not ideal to have a close-and-reconnect
happening multiple times in less than a second
(which is what you see in one of the logs provided
in the bug report).

thanks
-- PMM