From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58852) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwlZO-0000xa-7N for qemu-devel@nongnu.org; Wed, 19 Oct 2016 03:45:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwlZK-0004Hu-HV for qemu-devel@nongnu.org; Wed, 19 Oct 2016 03:45:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57972) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwlZK-0004HO-91 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 03:45:46 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 91032C04B311 for ; Wed, 19 Oct 2016 07:45:44 +0000 (UTC) Date: Wed, 19 Oct 2016 08:45:40 +0100 From: "Daniel P. Berrange" Message-ID: <20161019074540.GD11194@redhat.com> Reply-To: "Daniel P. Berrange" References: <20161012191502.GC16187@work-vm> <20161018100409.GH4349@redhat.com> <20161018113202.GE2190@work-vm> <20161018120121.GN4349@redhat.com> <20161018132524.GG2190@work-vm> <20161018133528.GD12728@redhat.com> <20161018135213.GI2190@work-vm> <20161018140141.GF12728@redhat.com> <20161018185351.GB22395@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161018185351.GB22395@work-vm> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] chardev's and fd's in monitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: qemu-devel@nongnu.org, armbru@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 wrot= e: > > > * 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 Gilb= ert 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 i= s that > > > > > > > > > they both want fd's or FILE*'s; editline takes either b= ut > > > > > > > > > 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 p= articular > > > > > > > > > tls, mux, ringbuffers etc. > > > > > > > > >=20 > > > > > > > > > If we could get away with just a FILE* then we could us= e fopencookie, > > > > > > > > > but that's GNU only. > > > > > > > > >=20 > > > > > > > > > Is there any sane way of shepherding all chardev's into= having an > > > > > > > > > fd? > > > > > > > >=20 > > > > > > > > The entire chardev abstraction model exists precisely bec= ause we cannot > > > > > > > > make all chardevs look like a single fd. Even those which= are fd based > > > > > > > > may have separate FDs for input and output. > > > > > > >=20 > > > > > > > Note that editline takes separate in/out streams, but it do= es want those streams > > > > > > > to be FILE*'s. > > > > > > >=20 > > > > > > > > IMHO the only viable approach would be to enhance linenoi= se/editline to > > > > > > > > not assume use of fd* or FILE * abstractions. > > > > > > >=20 > > > > > > > I think if it came to that then we'd probably end up sticki= ng with what we > > > > > > > had for a very long time; I'd assume it would take a long t= ime before > > > > > > > any mods we made to the libraries would come around to be g= enerally useful. > > > > > > >=20 > > > > > > > > BTW, what is the actual thread issue you are facing ? Cha= rdevs at least > > > > > > > > ought to be usable from a separate thread, as long as eac= h distinct > > > > > > > > chardev object instance was only used from one thread at = a time ? > > > > > > >=20 > > > > > > > Marc-Andr=C3=A9 pointed that out; I hadn't realised they we= re 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? > > > > > >=20 > > > > > > 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. > > > > >=20 > > > > > 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 a= nd the monitor > > > > > is also. What about something like an error_report from a diff= erent thread > > > > > while something is happening in the monitor? > > > >=20 > > > > If we moved execution of monitor commands to separate thread from= the > > > > thread handling monitor I/O, then we'd have to modify error_repor= t so > > > > that it queued the text in some manner, such that it was only the= n > > > > fed back to the client once the command thread completed. Alterna= tively > > > > we'd have to introduced locking in the Monitor object, that seria= lized > > > > access to the underling CharDriverState I/O funcs. > > >=20 > > > I already use error_report's in places in migration threads of vari= ous > > > types; I'm not sure if that's a problem. > >=20 > > 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. >=20 > Hmm that's going to be interesting to fix; I certainly use error_repor= t > all over in postcopy, and the postcopy code uses device load code in it= s > 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 --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr= / :|