From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41002) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3c52-0000NP-2J for qemu-devel@nongnu.org; Mon, 30 Nov 2015 22:58:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3c4y-0005td-1c for qemu-devel@nongnu.org; Mon, 30 Nov 2015 22:58:15 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51493) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3c4x-0005t7-S0 for qemu-devel@nongnu.org; Mon, 30 Nov 2015 22:58:11 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id E44DE935D8 for ; Tue, 1 Dec 2015 03:58:10 +0000 (UTC) Date: Tue, 1 Dec 2015 11:57:57 +0800 From: Peter Xu Message-ID: <20151201035755.GI21032@pxdev.xzpeter.org> References: <1448883140-20249-1-git-send-email-peterx@redhat.com> <1448883140-20249-11-git-send-email-peterx@redhat.com> <565C478A.8080106@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <565C478A.8080106@redhat.com> Subject: Re: [Qemu-devel] [PATCH v3 10/12] Dump: add qmp command "query-dump" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: drjones@redhat.com, lersek@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, famz@redhat.com, lcapitulino@redhat.com On Mon, Nov 30, 2015 at 01:56:42PM +0100, Paolo Bonzini wrote: > > > On 30/11/2015 12:32, Peter Xu wrote: > > +{ > > + DumpQueryResult *result = g_malloc0(sizeof(*result)); > > + DumpState *state = dump_state_get_global(); > > + result->status = state->status; > > + result->written_bytes = state->written_size; > > You need a mutex around the reads of ->status and ->written_size. Could I avoid using mutex here? Let me try to explain what I thought. The concurrency of this should only happen when: - detached dump thread is working (dump thread) - user queries dump status (main thread) What the dump thread is doing should be something like: - [start dumping] - inc written_size - inc written_size - ... - inc written_size - set ->status to COMPLETED|FAILED - [end dumping] Now if the main thread tries to fetch dump status during it's working, the worst thing is that, the ->written_size fetched by main thread is not exactly the one when it was fetching ->status. Or say, we might get some kind of inaccuracy (which should be really small) without the lock. Meanwhile, we could avoid a lock if we could allow the very small difference in written_size. Another thing could happen is when user queries duing it's finishing (or say, user query between dump thread modify written_size and status), we might got this: { "status": "active", "written": "100", "total": "100" } Rather than: { "status": "completed", "written": "100", "total": "100" } As long as we make sure we fetch "status" first rather than "written_size" (that's what I did in current codes). It should still be acceptable? Here, the reason I would like to avoid using lock is that: if I use lock here, I need to use it whenever dump thread increases the ->written_size. That's a operation very frequently happens in dump thread. I could enhance it though by updating ->written_bytes in periods, but it might be awkward comparing to directly drop the lock (if possible) by losing some kind of accuracy. Not sure whether I missed anything. Also, please let me know if you still suggest using lock here. (btw, if using lock, would spinlock be better?) Thanks! Peter > > Paolo > > > + result->total_bytes = state->total_size; > > + return result;