| Message ID | 20241105083203.2230983-1-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hw/audio/hda: avoid unnecessary re-open stream on reconfiguration | expand |
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 > > >
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
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
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;