Re: [Qemu-devel] [PATCH v3 1/2] add a header file for atomic operations

[Richard Henderson <rth@twiddle.net>]

On 06/19/2013 01:39 PM, Torvald Riegel wrote:
> On Thu, 2013-06-20 at 04:59 +0800, Liu Ping Fan wrote:
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index fbabf99..28abe1e 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -16,6 +16,7 @@
>>  #include <sys/ioctl.h>
>>  #include "hw/virtio/vhost.h"
>>  #include "hw/hw.h"
>> +#include "qemu/atomic.h"
>>  #include "qemu/range.h"
>>  #include <linux/vhost.h>
>>  #include "exec/address-spaces.h"
>> @@ -47,11 +48,9 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>>              addr += VHOST_LOG_CHUNK;
>>              continue;
>>          }
>> -        /* Data must be read atomically. We don't really
>> -         * need the barrier semantics of __sync
>> -         * builtins, but it's easier to use them than
>> -         * roll our own. */
>> -        log = __sync_fetch_and_and(from, 0);
>> +        /* Data must be read atomically. We don't really need barrier semantics
>> +         * but it's easier to use atomic_* than roll our own. */
>> +        log = atomic_xchg(from, 0);
> 
> If you really don't need any ordering guarantees / barriers here, then
> using a relaxed load should be fine.  But my gut feeling tells me you
> probably do need some barriers; either you are "re-using" another
> barrier (and then the comment should probably point out which), or it
> must be a case where it's either fine to read any value someone (else)
> wrote or there's no concurrent store after all.
> 

There is a store here, before and after.  Read the value, store zero.

I suppose what the comment is saying is that the atomic operation doesn't need
to be ordered with respect to the rest of the surrounding code, as the object
being synchronized is just that one integer.


r~