From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49175) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X7LrD-0007mX-J9 for qemu-devel@nongnu.org; Wed, 16 Jul 2014 05:50:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X7Lr7-0005pR-EL for qemu-devel@nongnu.org; Wed, 16 Jul 2014 05:50:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30360) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X7Lr7-0005pC-4O for qemu-devel@nongnu.org; Wed, 16 Jul 2014 05:50:33 -0400 Message-ID: <53C64AE1.8070805@redhat.com> Date: Wed, 16 Jul 2014 11:50:25 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1404495717-4239-1-git-send-email-dgilbert@redhat.com> <1404495717-4239-8-git-send-email-dgilbert@redhat.com> <53B7CE18.9020907@redhat.com> <20140716093742.GE2514@work-vm> In-Reply-To: <20140716093742.GE2514@work-vm> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/46] Return path: Open a return path on QEMUFile for sockets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, qemu-devel@nongnu.org, lilei@linux.vnet.ibm.com Il 16/07/2014 11:37, Dr. David Alan Gilbert ha scritto: >>> >>> + >>> + /* If it's already open, return it */ >>> + if (qfs->file->return_path) { >>> + return qfs->file->return_path; >> >> Wouldn't this leave a dangling file descriptor if you call >> socket_dup_return_path twice, and then close the original QEMUFile? > > Hmm - how? The problem is that there is no reference count on QEMUFile, so if you do f1 = open_return_path(f0); f2 = open_return_path(f0); /* now f1 == f2 */ qemu_fclose(f1); /* now f2 is dangling */ The remark about "closing the original QEMUFile" is also related to this part: if (result) { /* We are the reverse path of our reverse path (although I don't expect this to be used, it would stop another dup if it was / result->return_path = qfs->file; which has a similar bug f1 = open_return_path(f0); f2 = open_return_path(f1); /* now f2 == f0 */ qemu_fclose(f0); /* now f2 is dangling */ If this is correct, the simplest fix is to drop the optimization. > > Source side > Forward path - written by migration thread > : It's OK for this to be blocking, but we end up with it being > non-blocking, and modify the socket code to emulate blocking. This likely has a performance impact though. The first migration thread code drop from Juan already improved throughput a lot, even if it kept the iothread all the time and only converted from nonblocking writes to blocking. > Return path - opened by main thread, read by fd_handler on main thread > : Must be non-blocking so as not to block the main thread while > waiting for a partially sent command. Why can't you handle this in the migration thread (or a new postcopy thread on the source side)? Then it can stay blocking. > Destination side > Forward path - read by main thread This must be nonblocking so that the monitor keeps responding. > Return path - opened by main thread, written by main thread AND postcopy > thread (protected by rp_mutex) When does the main thread needs to write? If it doesn't need that, you can just switch to blocking when you process the listen command (i.e. when the postcopy thread starts). Paolo > I think I'm OK with both these being blocking.