From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3kBi-0002Kt-Mt for qemu-devel@nongnu.org; Tue, 01 Dec 2015 07:37:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3kBh-0004EY-PE for qemu-devel@nongnu.org; Tue, 01 Dec 2015 07:37:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58247) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3kBh-0004EP-JZ for qemu-devel@nongnu.org; Tue, 01 Dec 2015 07:37:41 -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 3C37F120482 for ; Tue, 1 Dec 2015 12:37:41 +0000 (UTC) References: <1448883140-20249-1-git-send-email-peterx@redhat.com> <1448883140-20249-11-git-send-email-peterx@redhat.com> <565C478A.8080106@redhat.com> <20151201035755.GI21032@pxdev.xzpeter.org> <565D6E68.1060303@redhat.com> <20151201123250.GA31109@pxdev.xzpeter.org> From: Paolo Bonzini Message-ID: <565D9491.5000909@redhat.com> Date: Tue, 1 Dec 2015 13:37:37 +0100 MIME-Version: 1.0 In-Reply-To: <20151201123250.GA31109@pxdev.xzpeter.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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: Peter Xu Cc: drjones@redhat.com, lersek@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, famz@redhat.com, lcapitulino@redhat.com On 01/12/2015 13:32, Peter Xu wrote: > On Tue, Dec 01, 2015 at 10:54:48AM +0100, Paolo Bonzini wrote: >> >> >> On 01/12/2015 04:57, Peter Xu wrote: >>>>> 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] >> >> Yes, it's possible but you need to use atomic_mb_read/atomic_mb_set to >> write ->status. Otherwise a CPU can see the write to ->status before >> some of the final writes to ->written_size. > > Hi, Paolo, > > Thanks to point out. However, would it be confusing to use > atomic_mb_{read|set} rather than directly use smp_rmb() and > smp_wmb()? Like: > > In dump thread: > > - inc written_size > - inc written_size > - ... > - inc written_size > - smp_wmb() > - atomic_set(status, COMPLETED|FAILED) > > In main thread: > > - atomic_read(status) > - smp_rmb() > - read written_size > > What I understand from the doc (seems written by you, thanks :) ) is > that: atomic_mb_{read|set} is the pair of helper functions for _one_ > specific variable, to make sure its operations are always in order > as long as we are using atomic_mb_* functions to access it all the > time. However, in the dump thread case, it's related to read/write > order of two variables (status and written_size). atomic_mb_{read,set} does order accesses to the variable against all other accesses. In this case I'd prefer it to smp_wmb/rmb, because the writes to written_size are far from the writes to status. Compare with thread-pool.c: req->ret = ret; /* Write ret before state. */ smp_wmb(); req->state = THREAD_DONE; /* Read state before ret. */ smp_rmb(); /* Schedule ourselves in case elem->common.cb() calls aio_poll() to * wait for another request that completed at the same time. */ qemu_bh_schedule(pool->completion_bh); elem->common.cb(elem->common.opaque, elem->ret); It's a matter of taste though. What you wrote above is certainly okay as well. Paolo