From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34869) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2HAz-0005fK-I8 for qemu-devel@nongnu.org; Fri, 27 Nov 2015 06:26:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a2HAw-00030g-S3 for qemu-devel@nongnu.org; Fri, 27 Nov 2015 06:26:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59454) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2HAw-00030U-Me for qemu-devel@nongnu.org; Fri, 27 Nov 2015 06:26:50 -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 452E4C062C96 for ; Fri, 27 Nov 2015 11:26:50 +0000 (UTC) Date: Fri, 27 Nov 2015 19:26:45 +0800 From: Peter Xu Message-ID: <20151127112643.GB10191@pxdev.xzpeter.org> References: <1448592497-2462-1-git-send-email-peterx@redhat.com> <1448592497-2462-4-git-send-email-peterx@redhat.com> <565830E4.90301@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <565830E4.90301@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On Fri, Nov 27, 2015 at 11:31:00AM +0100, Paolo Bonzini wrote: > [snip] > > + > > +static GlobalDumpState *dump_state_get_global(void) > > +{ > > + static bool init = false; > > + static GlobalDumpState global_dump_state; > > + if (unlikely(!init)) { > > + qemu_mutex_init(&global_dump_state.gds_mutex); > > The mutex is not necessary. The structure is always created in the main > thread and released by the dump thread (of which there is just one). [1] > > You can then just make a global DumpState (not a pointer!), and separate > the fields between: > > - those that are protected by a mutex (most likely the DumpResult and > written_size, possibly others) Hi, Paolo, So mutex is still necessary, right? (refer to [1]) Since "dump-query" will read several fields of it, while the dump thread might be modifying them as well? > > - those that are only written before the thread is started, and thus do > not need to be protected by a mutex > > - those that are only accessed by the thread, and thus do not need to be > protected by a mutex either. > > See for example this extract from migration/block.c: > > typedef struct BlkMigState { > /* Written during setup phase. Can be read without a lock. */ > int blk_enable; > int shared_base; > QSIMPLEQ_HEAD(bmds_list, BlkMigDevState) bmds_list; > int64_t total_sector_sum; > bool zero_blocks; > > /* Protected by lock. */ > QSIMPLEQ_HEAD(blk_list, BlkMigBlock) blk_list; > int submitted; > int read_done; > > /* Only used by migration thread. Does not need a lock. */ > int transferred; > int prev_progress; > int bulk_completed; > > /* The lock. */ > QemuMutex lock; > } BlkMigState; > > static BlkMigState block_mig_state; Ok, I think I can remove the global state and make a static DumpState. When I was drafting the patch, I just tried to keep the old logic (malloc/free) and avoid introducing bugs. Maybe I was wrong. I should better not introduce new struct if not necessary. Will try to follow this example in v3. Thanks. Peter > > Paolo >