[Qemu-devel] [PATCH for-4.0 v4 0/7] monitor: misc fixes

[Qemu-devel] [PATCH for-4.0 v4 0/7] monitor: misc fixes

[Marc-André Lureau]

Hi,

Here is a small series of fixes for the monitor, mostly related to
threading issues.

v4:
 - added "colo: check chardev can switch context"
 - replaced an assert_not_reached() with a better assert
 - commit message updates & rb tags

v3:
 - replaced an error_report() with an assert()
 - simplify a mon->use_io_thread condition, removing needless QMP check
 - modify/add some code comments
 - update commit messages

v2 bis:
 - update comments/commit messages
 - add Peter r-b

v2: after Peter review
 - patch 2: fix resuming with oob=off
 - patch 4: keep MUX case explicit, improve commit message

Marc-André Lureau (7):
  monitor: inline ambiguous helper functions
  monitor: accept chardev input from iothread
  char: add a QEMU_CHAR_FEATURE_GCONTEXT flag
  monitor: check if chardev can switch gcontext for OOB
  colo: check chardev can switch context
  monitor: prevent inserting new monitors after cleanup
  monitor: avoid potential dead-lock when cleaning up

 include/chardev/char.h |  3 ++
 chardev/char.c         | 11 +++++++
 monitor.c              | 71 ++++++++++++++++++++++++++----------------
 net/colo-compare.c     |  6 ++++
 4 files changed, 65 insertions(+), 26 deletions(-)

-- 
2.20.0.rc1

[Qemu-devel] [PATCH for-4.0 v4 1/7] monitor: inline ambiguous helper functions

[Marc-André Lureau]

The function were not named with "mon_iothread", or following the AIO
vs GMainContext distinction. Inline them instead.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/monitor.c b/monitor.c
index d39390c2f2..d531e8ccc9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4453,16 +4453,6 @@ static void sortcmdlist(void)
     qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
 }
 
-static GMainContext *monitor_get_io_context(void)
-{
-    return iothread_get_g_main_context(mon_iothread);
-}
-
-static AioContext *monitor_get_aio_context(void)
-{
-    return iothread_get_aio_context(mon_iothread);
-}
-
 static void monitor_iothread_init(void)
 {
     mon_iothread = iothread_create("mon_iothread", &error_abort);
@@ -4549,7 +4539,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     GMainContext *context;
 
     assert(mon->use_io_thread);
-    context = monitor_get_io_context();
+    context = iothread_get_g_main_context(mon_iothread);
     assert(context);
     qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
                              monitor_qmp_event, NULL, mon, context, true);
@@ -4601,7 +4591,7 @@ void monitor_init(Chardev *chr, int flags)
              * since chardev might be running in the monitor I/O
              * thread.  Schedule a bottom half.
              */
-            aio_bh_schedule_oneshot(monitor_get_aio_context(),
+            aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
                                     monitor_qmp_setup_handlers_bh, mon);
             /* The bottom half will add @mon to @mon_list */
             return;
-- 
2.20.0.rc1

[Qemu-devel] [PATCH for-4.0 v4 2/7] monitor: accept chardev input from iothread

[Marc-André Lureau]

Chardev backends may not handle safely IO events from concurrent
threads (may not handle I/O events from concurrent threads safely,
only the write path is since commit >
9005b2a7589540a3733b3abdcfbccfe7746cd1a1). 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.

Clean up control flow not to rely on mon->use_io_thread implying
monitor_is_qmp(mon).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index d531e8ccc9..79afe99079 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4297,7 +4297,7 @@ int monitor_suspend(Monitor *mon)
 
     atomic_inc(&mon->suspend_cnt);
 
-    if (monitor_is_qmp(mon) && mon->use_io_thread) {
+    if (mon->use_io_thread) {
         /*
          * Kick I/O thread to make sure this takes effect.  It'll be
          * evaluated again in prepare() of the watch object.
@@ -4309,6 +4309,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)) {
@@ -4316,20 +4323,22 @@ void monitor_resume(Monitor *mon)
     }
 
     if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
-        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));
-            }
+        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);
         }
-        qemu_chr_fe_accept_input(&mon->chr);
+
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
     }
+
     trace_monitor_suspend(mon, -1);
 }
 
-- 
2.20.0.rc1

[Qemu-devel] [PATCH for-4.0 v4 3/7] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Marc-André Lureau]

QEMU_CHAR_FEATURE_GCONTEXT declares the character device can switch
GMainContext.

Assert we don't switch context when the character device doesn't
provide this feature.  Character device users must not violate this
restriction.  In particular, user configurations that violate them
must be rejected.

Existing frontend that rely on context switching would now assert() if
the backend doesn't allow it (instead of silently producing undesired
events in the default context). Following patches improve the
situation by reporting an error earlier instead, on the frontend side.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char.h |  3 +++
 chardev/char.c         | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 7becd8c80c..014566c3de 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -47,6 +47,9 @@ typedef enum {
     QEMU_CHAR_FEATURE_FD_PASS,
     /* Whether replay or record mode is enabled */
     QEMU_CHAR_FEATURE_REPLAY,
+    /* Whether the gcontext can be changed after calling
+     * qemu_chr_be_update_read_handlers() */
+    QEMU_CHAR_FEATURE_GCONTEXT,
 
     QEMU_CHAR_FEATURE_LAST,
 } ChardevFeature;
diff --git a/chardev/char.c b/chardev/char.c
index 152dde5327..ccba36bafb 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -193,6 +193,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
 {
     ChardevClass *cc = CHARDEV_GET_CLASS(s);
 
+    assert(qemu_chr_has_feature(s, QEMU_CHAR_FEATURE_GCONTEXT)
+           || !context);
     s->gcontext = context;
     if (cc->chr_update_read_handler) {
         cc->chr_update_read_handler(s);
@@ -240,6 +242,15 @@ static void char_init(Object *obj)
 
     chr->logfd = -1;
     qemu_mutex_init(&chr->chr_write_lock);
+
+    /*
+     * Assume if chr_update_read_handler is implemented it will
+     * take the updated gcontext into account.
+     */
+    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
+        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
+    }
+
 }
 
 static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
-- 
2.20.0.rc1

Re: [Qemu-devel] [PATCH for-4.0 v4 3/7] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Markus Armbruster]

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

> QEMU_CHAR_FEATURE_GCONTEXT declares the character device can switch
> GMainContext.
>
> Assert we don't switch context when the character device doesn't
> provide this feature.  Character device users must not violate this
> restriction.  In particular, user configurations that violate them
> must be rejected.
>
> Existing frontend that rely on context switching would now assert() if
> the backend doesn't allow it (instead of silently producing undesired
> events in the default context). Following patches improve the
> situation by reporting an error earlier instead, on the frontend side.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Not all backends are able to switch gcontext. Those backends cannot
drive a OOB monitor (the monitor would then be blocking on main
thread).

For example, ringbuf, spice, or more esoteric input chardevs like
braille or MUX.

We currently forbid MUX because not all frontends are ready to run
outside main loop. Extend to add a context-switching feature check.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 79afe99079..25cf4223e8 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
     bool use_oob = flags & MONITOR_USE_OOB;
 
     if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {
+        if (CHARDEV_IS_MUX(chr) ||
+            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
             error_report("Monitor out-of-band is not supported with "
-                         "MUX typed chardev backend");
+                         "%s typed chardev backend",
+                         object_get_typename(OBJECT(chr)));
             exit(1);
         }
         if (use_readline) {
-- 
2.20.0.rc1

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Markus Armbruster]

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

> Not all backends are able to switch gcontext. Those backends cannot
> drive a OOB monitor (the monitor would then be blocking on main
> thread).
>
> For example, ringbuf, spice, or more esoteric input chardevs like
> braille or MUX.
>
> We currently forbid MUX because not all frontends are ready to run
> outside main loop. Extend to add a context-switching feature check.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Markus Armbruster]

One more question...

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

> Not all backends are able to switch gcontext. Those backends cannot
> drive a OOB monitor (the monitor would then be blocking on main
> thread).
>
> For example, ringbuf, spice, or more esoteric input chardevs like
> braille or MUX.

These chardevs don't provide QEMU_CHAR_FEATURE_GCONTEXT.

> We currently forbid MUX because not all frontends are ready to run
> outside main loop. Extend to add a context-switching feature check.

Why check CHARDEV_IS_MUX() when chardev-mux already fails the
qemu_char_feature_gcontext(chr, QEMU_CHAR_FEATURE_GCONTEXT) check?

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 79afe99079..25cf4223e8 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
>      bool use_oob = flags & MONITOR_USE_OOB;
>  
>      if (use_oob) {
> -        if (CHARDEV_IS_MUX(chr)) {
> +        if (CHARDEV_IS_MUX(chr) ||
> +            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
>              error_report("Monitor out-of-band is not supported with "
> -                         "MUX typed chardev backend");
> +                         "%s typed chardev backend",
> +                         object_get_typename(OBJECT(chr)));
>              exit(1);
>          }
>          if (use_readline) {

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Hi
On Thu, Dec 6, 2018 at 10:08 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> One more question...
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Not all backends are able to switch gcontext. Those backends cannot
> > drive a OOB monitor (the monitor would then be blocking on main
> > thread).
> >
> > For example, ringbuf, spice, or more esoteric input chardevs like
> > braille or MUX.
>
> These chardevs don't provide QEMU_CHAR_FEATURE_GCONTEXT.
>
> > We currently forbid MUX because not all frontends are ready to run
> > outside main loop. Extend to add a context-switching feature check.
>
> Why check CHARDEV_IS_MUX() when chardev-mux already fails the
> qemu_char_feature_gcontext(chr, QEMU_CHAR_FEATURE_GCONTEXT) check?
>


It currently fails, but with "[PATCH 4/9] char: update the mux
hanlders in class callback", it won't.

But the main reason to keep an explicit check on mux is that the
monitor frontend doesn't know if other mux frontends can be called
from any context (when you set a context, it is set on the backend
side, events are dispatched by the backend).

We may want to mix this extra frontend-side capability limitation with
FEATURE_GCONTEXT flag, but they are fundamentally different: to be
able to set a backend context VS attached mux frontends can be
dispatched from any context.


> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 79afe99079..25cf4223e8 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4562,9 +4562,11 @@ void monitor_init(Chardev *chr, int flags)
> >      bool use_oob = flags & MONITOR_USE_OOB;
> >
> >      if (use_oob) {
> > -        if (CHARDEV_IS_MUX(chr)) {
> > +        if (CHARDEV_IS_MUX(chr) ||
> > +            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> >              error_report("Monitor out-of-band is not supported with "
> > -                         "MUX typed chardev backend");
> > +                         "%s typed chardev backend",
> > +                         object_get_typename(OBJECT(chr)));
> >              exit(1);
> >          }
> >          if (use_readline) {

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Markus Armbruster]

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

> Hi
> On Thu, Dec 6, 2018 at 10:08 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> One more question...
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Not all backends are able to switch gcontext. Those backends cannot
>> > drive a OOB monitor (the monitor would then be blocking on main
>> > thread).
>> >
>> > For example, ringbuf, spice, or more esoteric input chardevs like
>> > braille or MUX.
>>
>> These chardevs don't provide QEMU_CHAR_FEATURE_GCONTEXT.
>>
>> > We currently forbid MUX because not all frontends are ready to run
>> > outside main loop. Extend to add a context-switching feature check.
>>
>> Why check CHARDEV_IS_MUX() when chardev-mux already fails the
>> qemu_char_feature_gcontext(chr, QEMU_CHAR_FEATURE_GCONTEXT) check?
>>
>
>
> It currently fails, but with "[PATCH 4/9] char: update the mux
> hanlders in class callback", it won't.

That's because it makes chardev-mux implement chr_update_read_handler(),
and "[PATCH 3/7] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag" assumes
that a chardev implementing that "will take the updated gcontext into
account".

Sounds to me as if "[PATCH 4/9] char: update the mux hanlders in class
callback" violates that assumption.  Why am I wrong?

> But the main reason to keep an explicit check on mux is that the
> monitor frontend doesn't know if other mux frontends can be called
> from any context (when you set a context, it is set on the backend
> side, events are dispatched by the backend).
>
> We may want to mix this extra frontend-side capability limitation with
> FEATURE_GCONTEXT flag, but they are fundamentally different: to be
> able to set a backend context VS attached mux frontends can be
> dispatched from any context.

I'm afraid I can't yet see the full picture.

The goal of this series PATCH 3-5 is to catch certain thread-related
badness in chardevs before it can happen.

Apparently, there are two separate kinds of badness:

* The chardev backend may fail to cope with changed gcontext.  I don't
  understand how exactly the backends screw up, but I doubt I have to
  right now.

* The chardev frontend may fail to... what exactly?  And why is only
  chardev-mux affected?

[...]

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Hi

On Thu, Dec 6, 2018 at 1:13 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi
> > On Thu, Dec 6, 2018 at 10:08 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> One more question...
> >>
> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >>
> >> > Not all backends are able to switch gcontext. Those backends cannot
> >> > drive a OOB monitor (the monitor would then be blocking on main
> >> > thread).
> >> >
> >> > For example, ringbuf, spice, or more esoteric input chardevs like
> >> > braille or MUX.
> >>
> >> These chardevs don't provide QEMU_CHAR_FEATURE_GCONTEXT.
> >>
> >> > We currently forbid MUX because not all frontends are ready to run
> >> > outside main loop. Extend to add a context-switching feature check.
> >>
> >> Why check CHARDEV_IS_MUX() when chardev-mux already fails the
> >> qemu_char_feature_gcontext(chr, QEMU_CHAR_FEATURE_GCONTEXT) check?
> >>
> >
> >
> > It currently fails, but with "[PATCH 4/9] char: update the mux
> > hanlders in class callback", it won't.
>
> That's because it makes chardev-mux implement chr_update_read_handler(),
> and "[PATCH 3/7] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag" assumes
> that a chardev implementing that "will take the updated gcontext into
> account".
>
> Sounds to me as if "[PATCH 4/9] char: update the mux hanlders in class
> callback" violates that assumption.  Why am I wrong?

The mux should be gcontext-feature neutral, or it should in fact
reflects the backend capability, since it is entirely driven by it.

For now, it is simpler to keep it mark as unsupport, and I'll probably
update the aforementioned patch when resubmitting.

> > But the main reason to keep an explicit check on mux is that the
> > monitor frontend doesn't know if other mux frontends can be called
> > from any context (when you set a context, it is set on the backend
> > side, events are dispatched by the backend).
> >
> > We may want to mix this extra frontend-side capability limitation with
> > FEATURE_GCONTEXT flag, but they are fundamentally different: to be
> > able to set a backend context VS attached mux frontends can be
> > dispatched from any context.
>
> I'm afraid I can't yet see the full picture.
>
> The goal of this series PATCH 3-5 is to catch certain thread-related
> badness in chardevs before it can happen.

Yes, as the context is associated with a thread. If a backend is not
able to switch context, it will keep dispatching in the default
context, which may have undesirable results for the frontend.

>
> Apparently, there are two separate kinds of badness:
>
> * The chardev backend may fail to cope with changed gcontext.  I don't
>   understand how exactly the backends screw up, but I doubt I have to
>   right now.
>
> * The chardev frontend may fail to... what exactly?  And why is only
>   chardev-mux affected?

For some reason, the chardev API let the frontend decide which context
should be used for the dispatch.

This is quite fine when you have a one-to-one relationship between
backend and frontend (as long as the backend complies with context
switching, ie FEATURE_GCONTEXT).

But in a one-to-many, as is the case with MUX, things get more
complicated, because one frontend may want to switch the context
(typically an oob monitor, moving dispatch to the iothread) while
another frontend (typically, a serial device) may not expect to be
dispatched from a different thread than the default thread.

As you can see, MUX has two problems wrt context switching: backend
and frontends. I think it would be safer to mark MUX as
!FEATURE_GCONTEXT (although in fact, you could use it if you really
now what you do with backend & frontends)

>
> [...]

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Markus Armbruster]

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

> Hi
>
> On Thu, Dec 6, 2018 at 1:13 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Hi
>> > On Thu, Dec 6, 2018 at 10:08 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >>
>> >> One more question...
>> >>
>> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>> >>
>> >> > Not all backends are able to switch gcontext. Those backends cannot
>> >> > drive a OOB monitor (the monitor would then be blocking on main
>> >> > thread).
>> >> >
>> >> > For example, ringbuf, spice, or more esoteric input chardevs like
>> >> > braille or MUX.
>> >>
>> >> These chardevs don't provide QEMU_CHAR_FEATURE_GCONTEXT.
>> >>
>> >> > We currently forbid MUX because not all frontends are ready to run
>> >> > outside main loop. Extend to add a context-switching feature check.
>> >>
>> >> Why check CHARDEV_IS_MUX() when chardev-mux already fails the
>> >> qemu_char_feature_gcontext(chr, QEMU_CHAR_FEATURE_GCONTEXT) check?
>> >>
>> >
>> >
>> > It currently fails, but with "[PATCH 4/9] char: update the mux
>> > hanlders in class callback", it won't.
>>
>> That's because it makes chardev-mux implement chr_update_read_handler(),
>> and "[PATCH 3/7] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag" assumes
>> that a chardev implementing that "will take the updated gcontext into
>> account".
>>
>> Sounds to me as if "[PATCH 4/9] char: update the mux hanlders in class
>> callback" violates that assumption.  Why am I wrong?
>
> The mux should be gcontext-feature neutral, or it should in fact
> reflects the backend capability, since it is entirely driven by it.

Yes, that makes sense.

> For now, it is simpler to keep it mark as unsupport, and I'll probably
> update the aforementioned patch when resubmitting.

Okay.

>> > But the main reason to keep an explicit check on mux is that the
>> > monitor frontend doesn't know if other mux frontends can be called
>> > from any context (when you set a context, it is set on the backend
>> > side, events are dispatched by the backend).
>> >
>> > We may want to mix this extra frontend-side capability limitation with
>> > FEATURE_GCONTEXT flag, but they are fundamentally different: to be
>> > able to set a backend context VS attached mux frontends can be
>> > dispatched from any context.
>>
>> I'm afraid I can't yet see the full picture.
>>
>> The goal of this series PATCH 3-5 is to catch certain thread-related
>> badness in chardevs before it can happen.
>
> Yes, as the context is associated with a thread. If a backend is not
> able to switch context, it will keep dispatching in the default
> context, which may have undesirable results for the frontend.
>
>>
>> Apparently, there are two separate kinds of badness:
>>
>> * The chardev backend may fail to cope with changed gcontext.  I don't
>>   understand how exactly the backends screw up, but I doubt I have to
>>   right now.
>>
>> * The chardev frontend may fail to... what exactly?  And why is only
>>   chardev-mux affected?
>
> For some reason, the chardev API let the frontend decide which context
> should be used for the dispatch.
>
> This is quite fine when you have a one-to-one relationship between
> backend and frontend (as long as the backend complies with context
> switching, ie FEATURE_GCONTEXT).
>
> But in a one-to-many, as is the case with MUX, things get more
> complicated, because one frontend may want to switch the context
> (typically an oob monitor, moving dispatch to the iothread) while
> another frontend (typically, a serial device) may not expect to be
> dispatched from a different thread than the default thread.
>
> As you can see, MUX has two problems wrt context switching: backend
> and frontends.

Thanks, that helped some.

>                I think it would be safer to mark MUX as
> !FEATURE_GCONTEXT (although in fact, you could use it if you really
> now what you do with backend & frontends)

There's no pressing need for a smarter chardev-mux that provides
FEATURE_GCONTEXT in cases where it's safe.  Simply not providing it at
all is good enough.

Testing CHARDEV_IS_MUX() in addition to FEATURE_GCONTEXT is then
redundant.

This makes me think we should drop the CHARDEV_IS_MUX() check from
monitor_init(), and update the commit message to say

    We already forbid MUX because not all frontends are ready to run outside
    main loop.  Replace that by a context-switching feature check.

What do you think?

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Hi
On Thu, Dec 6, 2018 at 1:38 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Hi
> >
> > On Thu, Dec 6, 2018 at 1:13 PM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >>
> >> > Hi
> >> > On Thu, Dec 6, 2018 at 10:08 AM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>
> >> >> One more question...
> >> >>
> >> >> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> >> >>
> >> >> > Not all backends are able to switch gcontext. Those backends cannot
> >> >> > drive a OOB monitor (the monitor would then be blocking on main
> >> >> > thread).
> >> >> >
> >> >> > For example, ringbuf, spice, or more esoteric input chardevs like
> >> >> > braille or MUX.
> >> >>
> >> >> These chardevs don't provide QEMU_CHAR_FEATURE_GCONTEXT.
> >> >>
> >> >> > We currently forbid MUX because not all frontends are ready to run
> >> >> > outside main loop. Extend to add a context-switching feature check.
> >> >>
> >> >> Why check CHARDEV_IS_MUX() when chardev-mux already fails the
> >> >> qemu_char_feature_gcontext(chr, QEMU_CHAR_FEATURE_GCONTEXT) check?
> >> >>
> >> >
> >> >
> >> > It currently fails, but with "[PATCH 4/9] char: update the mux
> >> > hanlders in class callback", it won't.
> >>
> >> That's because it makes chardev-mux implement chr_update_read_handler(),
> >> and "[PATCH 3/7] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag" assumes
> >> that a chardev implementing that "will take the updated gcontext into
> >> account".
> >>
> >> Sounds to me as if "[PATCH 4/9] char: update the mux hanlders in class
> >> callback" violates that assumption.  Why am I wrong?
> >
> > The mux should be gcontext-feature neutral, or it should in fact
> > reflects the backend capability, since it is entirely driven by it.
>
> Yes, that makes sense.
>
> > For now, it is simpler to keep it mark as unsupport, and I'll probably
> > update the aforementioned patch when resubmitting.
>
> Okay.
>
> >> > But the main reason to keep an explicit check on mux is that the
> >> > monitor frontend doesn't know if other mux frontends can be called
> >> > from any context (when you set a context, it is set on the backend
> >> > side, events are dispatched by the backend).
> >> >
> >> > We may want to mix this extra frontend-side capability limitation with
> >> > FEATURE_GCONTEXT flag, but they are fundamentally different: to be
> >> > able to set a backend context VS attached mux frontends can be
> >> > dispatched from any context.
> >>
> >> I'm afraid I can't yet see the full picture.
> >>
> >> The goal of this series PATCH 3-5 is to catch certain thread-related
> >> badness in chardevs before it can happen.
> >
> > Yes, as the context is associated with a thread. If a backend is not
> > able to switch context, it will keep dispatching in the default
> > context, which may have undesirable results for the frontend.
> >
> >>
> >> Apparently, there are two separate kinds of badness:
> >>
> >> * The chardev backend may fail to cope with changed gcontext.  I don't
> >>   understand how exactly the backends screw up, but I doubt I have to
> >>   right now.
> >>
> >> * The chardev frontend may fail to... what exactly?  And why is only
> >>   chardev-mux affected?
> >
> > For some reason, the chardev API let the frontend decide which context
> > should be used for the dispatch.
> >
> > This is quite fine when you have a one-to-one relationship between
> > backend and frontend (as long as the backend complies with context
> > switching, ie FEATURE_GCONTEXT).
> >
> > But in a one-to-many, as is the case with MUX, things get more
> > complicated, because one frontend may want to switch the context
> > (typically an oob monitor, moving dispatch to the iothread) while
> > another frontend (typically, a serial device) may not expect to be
> > dispatched from a different thread than the default thread.
> >
> > As you can see, MUX has two problems wrt context switching: backend
> > and frontends.
>
> Thanks, that helped some.
>
> >                I think it would be safer to mark MUX as
> > !FEATURE_GCONTEXT (although in fact, you could use it if you really
> > now what you do with backend & frontends)
>
> There's no pressing need for a smarter chardev-mux that provides
> FEATURE_GCONTEXT in cases where it's safe.  Simply not providing it at
> all is good enough.
>
> Testing CHARDEV_IS_MUX() in addition to FEATURE_GCONTEXT is then
> redundant.
>
> This makes me think we should drop the CHARDEV_IS_MUX() check from
> monitor_init(), and update the commit message to say
>
>     We already forbid MUX because not all frontends are ready to run outside
>     main loop.  Replace that by a context-switching feature check.
>
> What do you think?

ack, can you do that on commit?

thanks

Re: [Qemu-devel] [PATCH for-4.0 v4 4/7] monitor: check if chardev can switch gcontext for OOB

[Markus Armbruster]

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

> Hi
> On Thu, Dec 6, 2018 at 1:38 PM Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> This makes me think we should drop the CHARDEV_IS_MUX() check from
>> monitor_init(), and update the commit message to say
>>
>>     We already forbid MUX because not all frontends are ready to run outside
>>     main loop.  Replace that by a context-switching feature check.
>>
>> What do you think?
>
> ack, can you do that on commit?
>
> thanks

Sure.

[Qemu-devel] [PATCH for-4.0 v4 5/7] colo: check chardev can switch context

[Marc-André Lureau]

COLO uses a worker context (iothread) to drive the chardev. All
backends are not able to switch the context, let's report an error in
this case.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/colo-compare.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index a39191d522..9156ab3349 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -957,6 +957,12 @@ static int find_and_check_chardev(Chardev **chr,
         return 1;
     }
 
+    if (!qemu_chr_has_feature(*chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
+        error_setg(errp, "chardev \"%s\" cannot switch context",
+                   chr_name);
+        return 1;
+    }
+
     return 0;
 }
 
-- 
2.20.0.rc1

Re: [Qemu-devel] [PATCH for-4.0 v4 5/7] colo: check chardev can switch context

[Markus Armbruster]

I'd like an Acked-by or Reviewed-by from Zhang Chen or Li Zhijian.

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

> COLO uses a worker context (iothread) to drive the chardev. All
> backends are not able to switch the context, let's report an error in
> this case.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  net/colo-compare.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index a39191d522..9156ab3349 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -957,6 +957,12 @@ static int find_and_check_chardev(Chardev **chr,
>          return 1;
>      }
>  
> +    if (!qemu_chr_has_feature(*chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> +        error_setg(errp, "chardev \"%s\" cannot switch context",
> +                   chr_name);
> +        return 1;
> +    }
> +
>      return 0;
>  }

Re: [Qemu-devel] [PATCH for-4.0 v4 5/7] colo: check chardev can switch context

[Li Zhijian]

On 12/06/2018 04:37 AM, Marc-André Lureau wrote:
> COLO uses a worker context (iothread) to drive the chardev. All
> backends are not able to switch the context, let's report an error in
> this case.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Li Zhijian <lizhiian@cn.fujitsu.com>


> ---
>   net/colo-compare.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index a39191d522..9156ab3349 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -957,6 +957,12 @@ static int find_and_check_chardev(Chardev **chr,
>           return 1;
>       }
>   
> +    if (!qemu_chr_has_feature(*chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> +        error_setg(errp, "chardev \"%s\" cannot switch context",
> +                   chr_name);
> +        return 1;
> +    }
> +
>       return 0;
>   }
>   
>

Re: [Qemu-devel] [PATCH for-4.0 v4 5/7] colo: check chardev can switch context

[Zhang Chen]

On Thu, Dec 6, 2018 at 4:38 AM Marc-André Lureau <
marcandre.lureau@redhat.com> wrote:

> COLO uses a worker context (iothread) to drive the chardev. All
> backends are not able to switch the context, let's report an error in
> this case.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>

Reviewed-by: Zhang Chen <zhangckid@gmail.com>


> ---
>  net/colo-compare.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index a39191d522..9156ab3349 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -957,6 +957,12 @@ static int find_and_check_chardev(Chardev **chr,
>          return 1;
>      }
>
> +    if (!qemu_chr_has_feature(*chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> +        error_setg(errp, "chardev \"%s\" cannot switch context",
> +                   chr_name);
> +        return 1;
> +    }
> +
>      return 0;
>  }
>
> --
> 2.20.0.rc1
>
>

[Qemu-devel] [PATCH for-4.0 v4 6/7] monitor: prevent inserting new monitors after cleanup

[Marc-André Lureau]

monitor_cleanup() is one of the last things main() calls before it
returns.  In the following patch, monitor_cleanup() will release the
monitor_lock during flushing. There may be pending commands to insert
new monitors, which would modify the mon_list during iteration, and
the clean-up could thus miss those new insertions.

Add a monitor_destroyed global to check if monitor_cleanup() has been
already called. In this case, don't insert the new monitor in the
list, but free it instead. A cleaner solution would involve the main
thread telling other threads to terminate, waiting for their
termination.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/monitor.c b/monitor.c
index 25cf4223e8..f0256bdec5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest;
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
-/* Protects mon_list, monitor_qapi_event_state.  */
+/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
 static QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
+static bool monitor_destroyed;
 
 /* Protects mon_fdsets */
 static QemuMutex mon_fdsets_lock;
@@ -4538,8 +4539,21 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
 static void monitor_list_append(Monitor *mon)
 {
     qemu_mutex_lock(&monitor_lock);
-    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+    /*
+     * This prevents inserting new monitors during monitor_cleanup().
+     * A cleaner solution would involve the main thread telling other
+     * threads to terminate, waiting for their termination.
+     */
+    if (!monitor_destroyed) {
+        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+        mon = NULL;
+    }
     qemu_mutex_unlock(&monitor_lock);
+
+    if (mon) {
+        monitor_data_destroy(mon);
+        g_free(mon);
+    }
 }
 
 static void monitor_qmp_setup_handlers_bh(void *opaque)
@@ -4635,6 +4649,7 @@ void monitor_cleanup(void)
 
     /* Flush output buffers and destroy monitors */
     qemu_mutex_lock(&monitor_lock);
+    monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
         monitor_flush(mon);
-- 
2.20.0.rc1

[Qemu-devel] [PATCH for-4.0 v4 7/7] monitor: avoid potential dead-lock when cleaning up

[Marc-André Lureau]

When a monitor is connected to a Spice chardev, the monitor cleanup
can dead-lock:

 #0  0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0
 #1  0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0
 #2  0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 <monitor_lock>, file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66
 #3  0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645
 #4  0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 <error_abort>) at qapi/qapi-events-ui.c:149
 #5  0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235
 #6  0x00007f434560a6bb in reds_handle_channel_event (reds=<optimized out>, event=3, info=0x556ddad1b590) at reds.c:316
 #7  0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197
 #8  0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197
 #9  0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414
 #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388
 #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347
 #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0
 #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341
 #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241
 #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340
 #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280
 #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233
 #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786
 #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683
 #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660

Because spice code tries to emit a "disconnected" signal on the
monitors. Fix this dead-lock by releasing the monitor lock for
flush/destroy.

monitor_lock protects mon_list, monitor_qapi_event_state and
monitor_destroyed. monitor_flush() and monitor_data_destroy() don't
access any of those variables.

monitor_cleanup()'s loop is safe because it uses
QTAILQ_FOREACH_SAFE(), and no further monitor can be added after
calling monitor_cleanup() thanks to monitor_destroyed check in
monitor_list_append().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 monitor.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/monitor.c b/monitor.c
index f0256bdec5..936c040b2d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4652,8 +4652,11 @@ void monitor_cleanup(void)
     monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
+        /* Permit QAPI event emission from character frontend release */
+        qemu_mutex_unlock(&monitor_lock);
         monitor_flush(mon);
         monitor_data_destroy(mon);
+        qemu_mutex_lock(&monitor_lock);
         g_free(mon);
     }
     qemu_mutex_unlock(&monitor_lock);
-- 
2.20.0.rc1

Re: [Qemu-devel] [PATCH for-4.0 v4 0/7] monitor: misc fixes

[Markus Armbruster]

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

> Hi,
>
> Here is a small series of fixes for the monitor, mostly related to
> threading issues.

Queued, thanks!