Re: [Qemu-devel] chardev's and fd's in monitors

["Daniel P. Berrange" <berrange@redhat.com>]

On Tue, Oct 18, 2016 at 07:53:51PM +0100, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrange (berrange@redhat.com) wrote:
> > On Tue, Oct 18, 2016 at 02:52:13PM +0100, Dr. David Alan Gilbert wrote:
> > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > On Tue, Oct 18, 2016 at 02:25:25PM +0100, Dr. David Alan Gilbert wrote:
> > > > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > > > On Tue, Oct 18, 2016 at 12:32:02PM +0100, Dr. David Alan Gilbert wrote:
> > > > > > > * Daniel P. Berrange (berrange@redhat.com) wrote:
> > > > > > > > On Wed, Oct 12, 2016 at 08:15:02PM +0100, Dr. David Alan Gilbert wrote:
> > > > > > > > > Hi,
> > > > > > > > >   I had a look at a couple of readline like libraries;
> > > > > > > > > editline and linenoise.  A difficulty with using them is that
> > > > > > > > > they both want fd's or FILE*'s; editline takes either but
> > > > > > > > > from a brief look I think it's expecting to extract the fd.
> > > > > > > > > That makes them tricky to integrate into qemu, where
> > > > > > > > > the chardev's hide a whole bunch of non-fd things; in particular
> > > > > > > > > tls, mux, ringbuffers etc.
> > > > > > > > > 
> > > > > > > > > If we could get away with just a FILE* then we could use fopencookie,
> > > > > > > > > but that's GNU only.
> > > > > > > > > 
> > > > > > > > > Is there any sane way of shepherding all chardev's into having an
> > > > > > > > > fd?
> > > > > > > > 
> > > > > > > > The entire chardev abstraction model exists precisely because we cannot
> > > > > > > > make all chardevs look like a single fd. Even those which are fd based
> > > > > > > > may have separate FDs for input and output.
> > > > > > > 
> > > > > > > Note that editline takes separate in/out streams, but it does want those streams
> > > > > > > to be FILE*'s.
> > > > > > > 
> > > > > > > > IMHO the only viable approach would be to enhance linenoise/editline to
> > > > > > > > not assume use of fd* or FILE * abstractions.
> > > > > > > 
> > > > > > > I think if it came to that then we'd probably end up sticking with what we
> > > > > > > had for a very long time; I'd assume it would take a long time before
> > > > > > > any mods we made to the libraries would come around to be generally useful.
> > > > > > > 
> > > > > > > > BTW, what is the actual thread issue you are facing ? Chardevs at least
> > > > > > > > ought to be usable from a separate thread, as long as each distinct
> > > > > > > > chardev object instance was only used from one thread at a time ?
> > > > > > > 
> > > > > > > Marc-André pointed that out; I hadn't realised they were thread safe.
> > > > > > > But what are the rules? You say 'only used from one thread at a time' -
> > > > > > > what happens if we have a mux and the different streams to the mux come
> > > > > > > from different threads?
> > > > > > 
> > > > > > Well there is no mutex locking on the CharDriverState objects, so the
> > > > > > exact rule is "you mustn't do anything from multiple threads that will
> > > > > > race on contents of CharDriverState". That's too fuzzy to be useful to
> > > > > > developers though, so I think the only sensible option right now is to
> > > > > > say any "top level" CharDriverState should only be touch from one thread
> > > > > > at a time. IOW, if you have a mux, that that rule would apply to the
> > > > > > mux itself and the various children it owns as if they were a single
> > > > > > unnit.
> > > > > 
> > > > > OK; I think we're probably saved by the big lock at the moment, so that
> > > > > all device emulation that outputs text is probably holding it and the monitor
> > > > > is also.  What about something like an error_report from a different thread
> > > > > while something is happening in the monitor?
> > > > 
> > > > If we moved execution of monitor commands to separate thread from the
> > > > thread handling monitor I/O, then we'd have to modify error_report so
> > > > that it queued the text in some manner, such that it was only then
> > > > fed back to the client once the command thread completed. Alternatively
> > > > we'd have to introduced locking in the Monitor object, that serialized
> > > > access to the underling CharDriverState I/O funcs.
> > > 
> > > I already use error_report's in places in migration threads of various
> > > types; I'm not sure if that's a problem.
> > 
> > Unless those places are protected by the big qemu lock, that sounds
> > not good. error_report calls into error_vprintf which checks the
> > 'cur_mon' global "Monitor" pointer. This variable is updated at
> > runtime - eg in qmp_human_monitor_command(), monitor_qmp_read(),
> > monitor_read(), etc. So if migration threads outside the BQL are
> > calling error_report() that could well cause problems. If you
> > are lucky messages will merely end up going to stderr instead of
> > the monitor, but in worst case I wouldn't be surprised if there
> > is a crash possibility in some race conditions.
> 
> Hmm that's going to be interesting to fix;  I certainly use error_report
> all over in postcopy, and the postcopy code uses device load code in its
> threads that are shared by the normal load paths.   I doubt any of the
> rest of the threaded code is clean from them either; does block code
> used in the iothreads ever end up with an error_report?

One approach might be to turn the 'cur_mon' variable in to a
thread-local instead of a regular global variable. That would mean
any error_report() from a non-eventloop thread ends up going to
stderr instead of the to the monitor, but at least that is then
unambigously threadsafe.

> Can't we take the bql in the inside of error_report?

We could probably do that, or we could associate a dedicated mutex
with the cur_mon variable to have more fine grained locking.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|