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;