Re: [PATCH V2 3/5] atomic: convert to uatomic

[Chandan Babu R <chandan.babu@oracle.com>]

On 25 Sep 2021 at 03:43, Eric Sandeen wrote:
> On 9/24/21 9:09 AM, Chandan Babu R wrote:
>> From: Dave Chinner <dchinner@redhat.com>
>> Now we have liburcu, we can make use of it's atomic variable
>> implementation. It is almost identical to the kernel API - it's just
>> got a "uatomic" prefix. liburcu also provides all the same aomtic
>> variable memory barriers as the kernel, so if we pull memory barrier
>> dependent kernel code across, it will just work with the right
>> barrier wrappers.
>> This is preparation the addition of more extensive atomic operations
>> the that kernel buffer cache requires to function correctly.
>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
>> [chandan.babu@oracle.com: Swap order of arguments provided to atomic[64]_[add|sub]()]
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>   include/atomic.h | 65 ++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 54 insertions(+), 11 deletions(-)
>> diff --git a/include/atomic.h b/include/atomic.h
>> index e0e1ba84..99cb85d3 100644
>> --- a/include/atomic.h
>> +++ b/include/atomic.h
>> @@ -7,21 +7,64 @@
>>   #define __ATOMIC_H__
>>     /*
>> - * Warning: These are not really atomic at all. They are wrappers around the
>> - * kernel atomic variable interface. If we do need these variables to be atomic
>> - * (due to multithreading of the code that uses them) we need to add some
>> - * pthreads magic here.
>> + * Atomics are provided by liburcu.
>> + *
>> + * API and guidelines for which operations provide memory barriers is here:
>> + *
>> + * https://github.com/urcu/userspace-rcu/blob/master/doc/uatomic-api.md
>> + *
>> + * Unlike the kernel, the same interface supports 32 and 64 bit atomic integers.
>
> Given this, anyone have any objection to putting the #defines together at the
> top, rather than hiding the 64 variants at the end of the file?
>

I don't see any issue in doing that.

>>    */
>> +#include <urcu/uatomic.h>
>> +#include "spinlock.h"
>> +
>>   typedef	int32_t	atomic_t;
>>   typedef	int64_t	atomic64_t;
>>   -#define atomic_inc_return(x)	(++(*(x)))
>> -#define atomic_dec_return(x)	(--(*(x)))
>> +#define atomic_read(a)		uatomic_read(a)
>> +#define atomic_set(a, v)	uatomic_set(a, v)
>> +#define atomic_add(v, a)	uatomic_add(a, v)
>> +#define atomic_sub(v, a)	uatomic_sub(a, v)
>> +#define atomic_inc(a)		uatomic_inc(a)
>> +#define atomic_dec(a)		uatomic_dec(a)
>> +#define atomic_inc_return(a)	uatomic_add_return(a, 1)
>> +#define atomic_dec_return(a)	uatomic_sub_return(a, 1)
>> +#define atomic_dec_and_test(a)	(atomic_dec_return(a) == 0)
>> +#define cmpxchg(a, o, n)        uatomic_cmpxchg(a, o, n);
>
> and I'll fix this whitespace.
>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks for the reviews.

-- 
chandan