On 01/12/2015 13:32, Peter Xu wrote: > On Tue, Dec 01, 2015 at 10:54:48AM +0100, Paolo Bonzini wrote: >> >> >> On 01/12/2015 04:57, Peter Xu wrote: >>>>> 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] >> >> Yes, it's possible but you need to use atomic_mb_read/atomic_mb_set to >> write ->status. Otherwise a CPU can see the write to ->status before >> some of the final writes to ->written_size. > > Hi, Paolo, > > Thanks to point out. However, would it be confusing to use > atomic_mb_{read|set} rather than directly use smp_rmb() and > smp_wmb()? Like: > > In dump thread: > > - inc written_size > - inc written_size > - ... > - inc written_size > - smp_wmb() > - atomic_set(status, COMPLETED|FAILED) > > In main thread: > > - atomic_read(status) > - smp_rmb() > - read written_size > > What I understand from the doc (seems written by you, thanks :) ) is > that: atomic_mb_{read|set} is the pair of helper functions for _one_ > specific variable, to make sure its operations are always in order > as long as we are using atomic_mb_* functions to access it all the > time. However, in the dump thread case, it's related to read/write > order of two variables (status and written_size).
atomic_mb_{read,set} does order accesses to the variable against all other accesses. In this case I'd prefer it to smp_wmb/rmb, because the writes to written_size are far from the writes to status. Compare with thread-pool.c: req->ret = ret; /* Write ret before state. */ smp_wmb(); req->state = THREAD_DONE; /* Read state before ret. */ smp_rmb(); /* Schedule ourselves in case elem->common.cb() calls aio_poll() to * wait for another request that completed at the same time. */ qemu_bh_schedule(pool->completion_bh); elem->common.cb(elem->common.opaque, elem->ret); It's a matter of taste though. What you wrote above is certainly okay as well. Paolo