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

[Torvald Riegel <triegel@redhat.com>]

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.