Re: [Qemu-devel] [RFC 0/3] chardev: introduce chardev contexts

[Peter Xu]

On Fri, Aug 24, 2018 at 03:46:03PM +0200, Marc-André Lureau wrote:
> Hi,
> 
> On Fri, Aug 24, 2018 at 11:10 AM Peter Xu <address@hidden> wrote:
> >
> > This is a RFC series.  It majorly did these things:
> >
> > (1) move the monitor iothread management from monitor code to chardev
> >     code somehow,
> >
> > (2) decide which context/iothread to use for the chardev before
> >     chardev creates, by parsing monitor options earlier (not init, but
> >     only parsing) since monitor is the only exception now,
> >
> > (2) disallow chardev context to change for the whole lifecycle.
> >
> > Basically this work moves the only chardev iothread (the monitor
> > iothread) into chardev's management, then it's easy to setup the
> > iothread even before the chardev creates, hence no context switch for
> > chardev is needed any more.  In the future if we want to let any
> > chardev to run on some other threads, we just define a new
> > ChardevContext, then do qemu_opt_set_number(...) for the chardev.  For
> > now, there is only a monitor context.
> >
> > It does not mean that this will let chardev depend on monitor code,
> > instead it'll totally remove the reverse dependency - before this
> > work, the chardev backend strangely depends on the frontend to setup
> > the context (which brought us many trouble).  Now this should be gone.
> >
> > This series should solve the potential issue raised here:
> >
> >   https://patchwork.kernel.org/patch/10122713/#22187395
> >
> > And also should let Marc-André's vhost-user reconnect series work
> > without breaking context switch of chardev (since it never switches
> > now hence no way to break):
> >
> >   http://patchwork.ozlabs.org/cover/961442/
> >
> > Only smoke test carried out with out-of-band, and make check.
> >
> > Please have a look on whether this is a workable solution,
> 
> Clever hack!
> 
> The code could be simplified somewhat:
> - it probably doesn't need ChardevContextMap

Is it because we only have monitor to use it?  If so, I suspect we
might still want it since I'm just thinking maybe we can add a second
one for COLO...

[2]

> - I would not typedef ChardevContext (took me a while to realize it was an 
> enum)

Maybe, ChardevContextType?

> - we should avoid the "context" option, perhaps during chardev
> parsing, check -mon usage.

[2]

> - more :)
> 
> Other issues:
> - blend monitor code in chardev

As I mentioned in the cover letter, my intention is not to blend
monitor code into chardev.  For example, we can define the name as
CHR_CONTEXT_1 rather than CHR_CONTEXT_MONITOR and comment with

  /* CONTEXT_1 should only be used by monitor */

but it's just not that direct.  I'd say it's not "blending monitor
codes in chardev", but a pure naming.  Instead, actually I see [1]
tries to blend monitor code in chardev, which I would prefer not.
That's why I used the QemuOpts to pass and decide what context to use.

> - chardev cleanup is done after monitor cleanup, this may race iothreads

Will it?  My plan is that we only let frontend (monitor) depends on
the frontend (chardev), then cleaning monitor then chardev seems the
correct order for me without race, no?

> - breaks chardev context switching (colo)

I just had a glance at COLO, I'm not sure but... I think it's not
really using the "dynamic context switching".  IMHO it only needs a
separate context.  If so, it should be able to live with this series,
we might just need to define one more for COLO at [1].

> - not a very dynamic solution (doesn't help to create oob monitor dynamically)

I never created monitors dynamically, but if we want that we might
need to expose this "context" property to user, then we can
dynamically create out-of-band monitors (as long as when we create the
chardev for the out-of-band monitor we pass in correct context
property).

> 
> I'd continue to look for options, we might come back to this one eventually :)
> 
> thanks!

Thanks for the very positive feedback! :) Yes, let's see whether we
have better alternatives.

Regards,

-- 
Peter Xu