Hi On Thu, Dec 6, 2018 at 1:38 PM Markus Armbruster <arm...@redhat.com> wrote: > > Marc-André Lureau <marcandre.lur...@redhat.com> writes: > > > Hi > > > > On Thu, Dec 6, 2018 at 1:13 PM Markus Armbruster <arm...@redhat.com> wrote: > >> > >> Marc-André Lureau <marcandre.lur...@redhat.com> writes: > >> > >> > Hi > >> > On Thu, Dec 6, 2018 at 10:08 AM Markus Armbruster <arm...@redhat.com> > >> > wrote: > >> >> > >> >> One more question... > >> >> > >> >> Marc-André Lureau <marcandre.lur...@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