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

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>

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) {

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) {

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?

[...]

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)

>
> [...]

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?

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

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.