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

[Paolo Bonzini]

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