Chardev backends may not handle safely IO events from concurrent
threads. Better to wake up the chardev from the monitor IO thread if
it's being used as the chardev context.
Unify code paths by using a BH in all cases.
Drop the now redundant aio_notify() call.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
monitor.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/monitor.c b/monitor.c
index 07712d89f9..511dd11d1c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
return 0;
}
+static void monitor_accept_input(void *opaque)
+{
+ Monitor *mon = opaque;
+
+ qemu_chr_fe_accept_input(&mon->chr);
+}
+
void monitor_resume(Monitor *mon)
{
if (monitor_is_hmp_non_interactive(mon)) {
@@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
}
if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
+ AioContext *ctx = qemu_get_aio_context();
+
if (monitor_is_qmp(mon)) {
/*
* For QMP monitors that are running in the I/O thread,
* let's kick the thread in case it's sleeping.
*/
if (mon->use_io_thread) {
- aio_notify(iothread_get_aio_context(mon_iothread));
+ ctx = iothread_get_aio_context(mon_iothread);
}
} else {
assert(mon->rs);
readline_show_prompt(mon->rs);
}
- qemu_chr_fe_accept_input(&mon->chr);
+
+ aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
}
+
trace_monitor_suspend(mon, -1);
}
--
2.19.0.271.gfe8321ec05
On Mon, Oct 29, 2018 at 04:57:29PM +0400, Marc-André Lureau wrote:
> Chardev backends may not handle safely IO events from concurrent
> threads. Better to wake up the chardev from the monitor IO thread if
> it's being used as the chardev context.
>
> Unify code paths by using a BH in all cases.
>
> Drop the now redundant aio_notify() call.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> monitor.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 07712d89f9..511dd11d1c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
> return 0;
> }
>
> +static void monitor_accept_input(void *opaque)
> +{
> + Monitor *mon = opaque;
> +
> + qemu_chr_fe_accept_input(&mon->chr);
> +}
> +
> void monitor_resume(Monitor *mon)
> {
> if (monitor_is_hmp_non_interactive(mon)) {
> @@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
> }
>
> if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> + AioContext *ctx = qemu_get_aio_context();
> +
> if (monitor_is_qmp(mon)) {
> /*
> * For QMP monitors that are running in the I/O thread,
> * let's kick the thread in case it's sleeping.
This comment seems stall, you may consider to touch it up, otherwise
it looks sane to me:
Reviewed-by: Peter Xu <peterx@redhat.com>
> */
> if (mon->use_io_thread) {
> - aio_notify(iothread_get_aio_context(mon_iothread));
> + ctx = iothread_get_aio_context(mon_iothread);
> }
> } else {
> assert(mon->rs);
> readline_show_prompt(mon->rs);
> }
> - qemu_chr_fe_accept_input(&mon->chr);
> +
> + aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
> }
> +
> trace_monitor_suspend(mon, -1);
> }
>
> --
> 2.19.0.271.gfe8321ec05
>
Regards,
--
Peter Xu
Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> Chardev backends may not handle safely IO events from concurrent
> threads.
What exactly could go wrong? Or is this a well-known fact that doesn't
need further elaboration?
"safely handle I/O events"
> Better to wake up the chardev from the monitor IO thread if
> it's being used as the chardev context.
>
> Unify code paths by using a BH in all cases.
>
> Drop the now redundant aio_notify() call.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> monitor.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 07712d89f9..511dd11d1c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
> return 0;
> }
>
> +static void monitor_accept_input(void *opaque)
> +{
> + Monitor *mon = opaque;
> +
> + qemu_chr_fe_accept_input(&mon->chr);
> +}
> +
> void monitor_resume(Monitor *mon)
> {
> if (monitor_is_hmp_non_interactive(mon)) {
> @@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
> }
>
> if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> + AioContext *ctx = qemu_get_aio_context();
> +
> if (monitor_is_qmp(mon)) {
> /*
> * For QMP monitors that are running in the I/O thread,
> * let's kick the thread in case it's sleeping.
> */
> if (mon->use_io_thread) {
> - aio_notify(iothread_get_aio_context(mon_iothread));
> + ctx = iothread_get_aio_context(mon_iothread);
> }
> } else {
> assert(mon->rs);
> readline_show_prompt(mon->rs);
> }
Correct, since mon->use_io_thread can only be true in a QMP monitor so
far. But there's no need to depend on that here:
AioContext *ctx;
if (mon->use_io_thread) {
ctx = iothread_get_aio_context(mon_iothread);
} else {
ctx = qemu_get_aio_context();
}
if (!monitor_is_qmp(mon)) {
assert(mon->rs);
readline_show_prompt(mon->rs);
}
Same in monitor_suspend(), by the way.
The dependence in monitor_init() is tolerable, because it's the place
where the dependence is established.
> - qemu_chr_fe_accept_input(&mon->chr);
> +
> + aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
> }
> +
> trace_monitor_suspend(mon, -1);
> }
Hi
On Mon, Dec 3, 2018 at 11:26 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Chardev backends may not handle safely IO events from concurrent
> > threads.
>
> What exactly could go wrong? Or is this a well-known fact that doesn't
> need further elaboration?
chardev are not thread-safe. Only the write path is, since commit
9005b2a7589540a3733b3abdcfbccfe7746cd1a1.
> "safely handle I/O events"
>
> > Better to wake up the chardev from the monitor IO thread if
> > it's being used as the chardev context.
> >
> > Unify code paths by using a BH in all cases.
> >
> > Drop the now redundant aio_notify() call.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > monitor.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 07712d89f9..511dd11d1c 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
> > return 0;
> > }
> >
> > +static void monitor_accept_input(void *opaque)
> > +{
> > + Monitor *mon = opaque;
> > +
> > + qemu_chr_fe_accept_input(&mon->chr);
> > +}
> > +
> > void monitor_resume(Monitor *mon)
> > {
> > if (monitor_is_hmp_non_interactive(mon)) {
> > @@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
> > }
> >
> > if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> > + AioContext *ctx = qemu_get_aio_context();
> > +
> > if (monitor_is_qmp(mon)) {
> > /*
> > * For QMP monitors that are running in the I/O thread,
> > * let's kick the thread in case it's sleeping.
> > */
> > if (mon->use_io_thread) {
> > - aio_notify(iothread_get_aio_context(mon_iothread));
> > + ctx = iothread_get_aio_context(mon_iothread);
> > }
> > } else {
> > assert(mon->rs);
> > readline_show_prompt(mon->rs);
> > }
>
> Correct, since mon->use_io_thread can only be true in a QMP monitor so
> far. But there's no need to depend on that here:
>
> AioContext *ctx;
>
> if (mon->use_io_thread) {
> ctx = iothread_get_aio_context(mon_iothread);
> } else {
> ctx = qemu_get_aio_context();
> }
>
> if (!monitor_is_qmp(mon)) {
> assert(mon->rs);
> readline_show_prompt(mon->rs);
> }
>
> Same in monitor_suspend(), by the way.
>
ok
> The dependence in monitor_init() is tolerable, because it's the place
> where the dependence is established.
>
> > - qemu_chr_fe_accept_input(&mon->chr);
> > +
> > + aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
> > }
> > +
> > trace_monitor_suspend(mon, -1);
> > }
>
--
Marc-André Lureau
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Dec 3, 2018 at 11:26 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes: >> >> > Chardev backends may not handle safely IO events from concurrent >> > threads. >> >> What exactly could go wrong? Or is this a well-known fact that doesn't >> need further elaboration? > > chardev are not thread-safe. Only the write path is, since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1. Add this to your commit message? Your call. >> "safely handle I/O events" >> >> > Better to wake up the chardev from the monitor IO thread if >> > it's being used as the chardev context. >> > >> > Unify code paths by using a BH in all cases. >> > >> > Drop the now redundant aio_notify() call. >> > >> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> [...]
© 2016 - 2025 Red Hat, Inc.