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 >