Re: [Qemu-devel] [PATCH v6 1/8] atomic: introduce atomic operations

[liu ping fan]

On Mon, Nov 12, 2012 at 5:54 PM, Paolo Bonzini <address@hidden> wrote:
> Il 05/11/2012 06:38, Liu Ping Fan ha scritto:
>> From: Liu Ping Fan <address@hidden>
>>
>> If out of global lock, we will be challenged by SMP in low level,
>> so need atomic ops.
>>
>> This file is a wrapper of GCC atomic builtin.
>
> I still object to this.
>
> I know it enforces type-safety, but it is incomplete.  It doesn't

Although it is incomplete, but the rest cases are rarely used.  Linux
faces such issue, and the "int" version is enough, so I think we can
borrow experience from there.

> provide neither atomic accesses to pointers, nor useful operations such
> as exchange.  It won't be used consistently, because in some places you
> just do not have an Atomic value (see both current uses of __sync_*
> builtins).
>
> If you can make it complete, and prove it by using it where __sync_* is

For current code, __sync_* is used rarely, I think except the
barriers, only two places use it. We can substitute it easily.

Regards,
Pingfan
> used now, or just use gcc builtins directly.
>
> Paolo
>
>>
>> Signed-off-by: Liu Ping Fan <address@hidden>
>> ---
>>  include/qemu/atomic.h |   63 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 63 insertions(+), 0 deletions(-)
>>  create mode 100644 include/qemu/atomic.h
>>
>> diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
>> new file mode 100644
>> index 0000000..a9e6d35
>> --- /dev/null
>> +++ b/include/qemu/atomic.h
>> @@ -0,0 +1,63 @@
>> +/*
>> + * Simple wrapper of gcc Atomic-Builtins
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +#ifndef __QEMU_ATOMIC_H
>> +#define __QEMU_ATOMIC_H
>> +
>> +typedef struct Atomic {
>> +    volatile int counter;
>> +} Atomic;
>> +
>> +static inline void atomic_set(Atomic *v, int i)
>> +{
>> +    v->counter = i;
>> +}
>> +
>> +static inline int atomic_read(Atomic *v)
>> +{
>> +    return v->counter;
>> +}
>> +
>> +static inline int atomic_return_and_add(int i, Atomic *v)
>> +{
>> +    int ret;
>> +
>> +    ret = __sync_fetch_and_add(&v->counter, i);
>> +    return ret;
>> +}
>> +
>> +static inline int atomic_return_and_sub(int i, Atomic *v)
>> +{
>> +    int ret;
>> +
>> +    ret = __sync_fetch_and_sub(&v->counter, i);
>> +    return ret;
>> +}
>> +
>> +/**
>> + *  * atomic_inc - increment atomic variable
>> + *   * @v: pointer of type Atomic
>> + *    *
>> + *     * Atomically increments @v by 1.
>> + *      */
>> +static inline void atomic_inc(Atomic *v)
>> +{
>> +    __sync_fetch_and_add(&v->counter, 1);
>> +}
>> +
>> +/**
>> + *  * atomic_dec - decrement atomic variable
>> + *   * @v: pointer of type Atomic
>> + *    *
>> + *     * Atomically decrements @v by 1.
>> + *      */
>> +static inline void atomic_dec(Atomic *v)
>> +{
>> +    __sync_fetch_and_sub(&v->counter, 1);
>> +}
>> +
>> +#endif
>>
>