Re: [Qemu-devel] [PATCH v3 10/12] Dump: add qmp command "query-dump"

[Peter Xu]

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;