Re: [Qemu-devel] [PATCH v2 3/8] dump-guest-memory: add basic "detach" support.

[Peter Xu]

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
>