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

[Markus Armbruster <armbru@redhat.com>]

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?