diff mbox series

[V2,3/5] atomic: convert to uatomic

Message ID 20210924140912.201481-4-chandan.babu@oracle.com (mailing list archive)
State Accepted
Headers show
Series xfsprogs: generic serialisation primitives | expand

Commit Message

Chandan Babu R Sept. 24, 2021, 2:09 p.m. UTC 
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(-)

Comments

Eric Sandeen Sept. 24, 2021, 10:13 p.m. UTC | #1 
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?

>    */
> +#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>
Chandan Babu R Sept. 25, 2021, 10:26 a.m. UTC | #2 
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.
Dave Chinner Sept. 25, 2021, 11:15 p.m. UTC | #3 
On Fri, Sep 24, 2021 at 05:13:30PM -0500, 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 wanted to keep the -APIs- separate, because all the kernel
atomic/atomic64 stuff is already separate and type checked. I don't
see any point in commingling the two different atomic type APIs
just because the implementation ends up being the same and that some
wrappers are defines and others are static inline code.

Ideally, the wrappers should all be static inlines so we get correct
atomic_t/atomic64_t type checking in userspace. Those are the types
we care about in terms of libxfs, so to typecheck the API properly
these should -all- be static inlines. The patch as it stands was a
"get it working properly" patch, not a "finalised, strictly correct
API" patch. That was somethign for "down the road" as I polished the
patchset ready for eventual review.....

Cheers,

Dave.
Eric Sandeen Sept. 25, 2021, 11:18 p.m. UTC | #4 
On 9/25/21 6:15 PM, Dave Chinner wrote:
> On Fri, Sep 24, 2021 at 05:13:30PM -0500, 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 wanted to keep the -APIs- separate, because all the kernel
> atomic/atomic64 stuff is already separate and type checked. I don't
> see any point in commingling the two different atomic type APIs
> just because the implementation ends up being the same and that some
> wrappers are defines and others are static inline code.
> 
> Ideally, the wrappers should all be static inlines so we get correct
> atomic_t/atomic64_t type checking in userspace. Those are the types
> we care about in terms of libxfs, so to typecheck the API properly
> these should -all- be static inlines. The patch as it stands was a
> "get it working properly" patch, not a "finalised, strictly correct
> API" patch. That was somethign for "down the road" as I polished the
> patchset ready for eventual review.....

Ok. Well, I was only talking about moving lines in your patch, nothing functional
at all. And ... that's why I had asked earlier (I think?) if your patch was
considered ready for review/merge, or just a demonstration of things to come.

So I guess changing it to a static inline as you suggest should be done before
merge.  Anything else like that that you don't actually consider quite ready,
in the first 3 patches?

Thanks,
-Eric
Dave Chinner Sept. 25, 2021, 11:49 p.m. UTC | #5 
On Sat, Sep 25, 2021 at 06:18:58PM -0500, Eric Sandeen wrote:
> On 9/25/21 6:15 PM, Dave Chinner wrote:
> > On Fri, Sep 24, 2021 at 05:13:30PM -0500, 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 wanted to keep the -APIs- separate, because all the kernel
> > atomic/atomic64 stuff is already separate and type checked. I don't
> > see any point in commingling the two different atomic type APIs
> > just because the implementation ends up being the same and that some
> > wrappers are defines and others are static inline code.
> > 
> > Ideally, the wrappers should all be static inlines so we get correct
> > atomic_t/atomic64_t type checking in userspace. Those are the types
> > we care about in terms of libxfs, so to typecheck the API properly
> > these should -all- be static inlines. The patch as it stands was a
> > "get it working properly" patch, not a "finalised, strictly correct
> > API" patch. That was somethign for "down the road" as I polished the
> > patchset ready for eventual review.....
> 
> Ok. Well, I was only talking about moving lines in your patch, nothing functional
> at all. And ... that's why I had asked earlier (I think?) if your patch was
> considered ready for review/merge, or just a demonstration of things to come.

I though you were asking "does it work/been tested enough to merge"
to which I answered "yes". I did point out that it was a quick
forward port, so stuff like variables the wrong way around in
wrappers I had to add for the forward port shouldn't be a surprise.

> So I guess changing it to a static inline as you suggest should be done before
> merge.

I don't see that as necessary before merging it. It's the direction
these wrappers need to move in so that we get better consistency in
libxfs between user and kernel space. We don't have to do everyone
at once and make code pristine perfect before merging it. Merging
functional, working code is far better than trying to polish off
every rough edge of every patch before considering them for merge..

> Anything else like that that you don't actually consider quite ready,
> in the first 3 patches?

Nope.

-Dave.
diff mbox series

Patch

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.
  */
+#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);
+
+static inline bool atomic_add_unless(atomic_t *a, int v, int u)
+{
+	int r = atomic_read(a);
+	int n, o;
+
+	do {
+		o = r;
+		if (o == u)
+			break;
+		n = o + v;
+		r = uatomic_cmpxchg(a, o, n);
+	} while (r != o);
+
+	return o != u;
+}
+
+static inline bool atomic_dec_and_lock(atomic_t *a, spinlock_t *lock)
+{
+	if (atomic_add_unless(a, -1, 1))
+		return 0;
+
+	spin_lock(lock);
+	if (atomic_dec_and_test(a))
+		return 1;
+	spin_unlock(lock);
+	return 0;
+}
 
-#define atomic64_read(x)	*(x)
-#define atomic64_set(x, v)	(*(x) = v)
-#define atomic64_add(v, x)	(*(x) += v)
-#define atomic64_inc(x)		((*(x))++)
-#define atomic64_dec(x)		((*(x))--)
+#define atomic64_read(x)	uatomic_read(x)
+#define atomic64_set(x, v)	uatomic_set(x, v)
+#define atomic64_add(v, a)	uatomic_add(a, v)
+#define atomic64_sub(v, a)	uatomic_sub(a, v)
+#define atomic64_inc(a)		uatomic_inc(a)
+#define atomic64_dec(a)		uatomic_dec(a)
 
 #endif /* __ATOMIC_H__ */