[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 4/5] asm/atomic.h: common prototyping (add xen/atomic.h)
On 7/13/2016 10:49 PM, Andrew Cooper wrote:
On 13/07/16 20:33, Corneliu ZUZU wrote:
On 7/13/2016 10:01 PM, Stefano Stabellini wrote:
On Wed, 13 Jul 2016, Corneliu ZUZU wrote:
Create a common-side <xen/atomic.h> to establish, among others,
prototypes of
atomic functions called from common-code. Done to avoid introducing
inconsistencies between arch-side <asm/atomic.h> headers when we
make subtle
changes to one of them.
Some arm-side macros had to be turned into inline functions in the
process.
Removed outdated comment ("NB. I've [...]").
Signed-off-by: Corneliu ZUZU <czuzu@xxxxxxxxxxxxxxx>
Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
Changed since v1:
* removed comments that were duplicate between asm-x86/atomic.h and
xen/atomic.h
* remove outdated comment ("NB. [...]")
* add atomic_cmpxchg doc-comment
* don't use yoda condition
---
xen/include/asm-arm/atomic.h | 45 ++++++++----
xen/include/asm-x86/atomic.h | 103 +-------------------------
xen/include/xen/atomic.h | 171
+++++++++++++++++++++++++++++++++++++++++++
3 files changed, 202 insertions(+), 117 deletions(-)
create mode 100644 xen/include/xen/atomic.h
diff --git a/xen/include/asm-arm/atomic.h
b/xen/include/asm-arm/atomic.h
index e8f7340..01af43b 100644
--- a/xen/include/asm-arm/atomic.h
+++ b/xen/include/asm-arm/atomic.h
@@ -2,6 +2,7 @@
#define __ARCH_ARM_ATOMIC__
#include <xen/config.h>
+#include <xen/atomic.h>
#include <xen/prefetch.h>
#include <asm/system.h>
@@ -95,15 +96,6 @@ void __bad_atomic_size(void);
default: __bad_atomic_size();
break; \
} \
})
-
-/*
- * NB. I've pushed the volatile qualifier into the operations. This
allows
- * fast accessors such as _atomic_read() and _atomic_set() which
don't give
- * the compiler a fit.
- */
-typedef struct { int counter; } atomic_t;
-
-#define ATOMIC_INIT(i) { (i) }
/*
* On ARM, ordinary assignment (str instruction) doesn't clear the
local
@@ -141,12 +133,35 @@ static inline void _atomic_set(atomic_t *v,
int i)
#define atomic_inc_return(v) (atomic_add_return(1, v))
#define atomic_dec_return(v) (atomic_sub_return(1, v))
What about atomic_inc_return and atomic_dec_return? Doesn't it make
sense to do this for all of them, since we are at it? I believe there
are also a couple more which are #define'd.
Those don't seem to be implemented for X86 and neither are they
referred from common code (probably because they really aren't defined
for X86).
Apologies - this is definitely turning into the rats nest I warned you
it might.
As far as I am concerned, I only think it is important to have the
static inline prototypes for the versions which are actually common
between architectures. Those are the ones we don't want to break
accidentally.
However, for the helpers which are not common, having them consistently
static inline would be a distinct improvement over mixed
inline/defines. (static inline functions are superior to macros in many
ways.)
~Andrew
Absolutely no problem! The rats nest you imagined before this series was
a different one :P . This really doesn't seem like it would require much
effort.
What I could also try to do is to actually implement them on X86 as
well. Would that be preferable?
I think the complete functions that are missing there are:
atomic_sub_return, atomic_{inc,dec}_return, atomic_xchg,
__atomic_add_unless. Please confirm this list.
As for how I'd implement them:
atomic_sub_return() - would "return arch_fetch_and_add(&v->counter,
-i) - i;" - similarly to what atomic_add_return() does - do?
atomic_{inc,dec}_return() - would "return
atomic_{add,sub}_return(1, v)" respectively as on ARM do?
atomic_xchg() - since xchg() is also available on X86, would
"return xchg(&((v)->counter), new);" as on ARM do?
Stefano, Julien: note that I'd have to
implement this for ARM64, so, since xchg is -also- available on ARM64,
would the above implementation also do in that case?
__atomic_add_unless() - again, everything this depends on in the
ARM64 version is already available on X86 too; would it also be advised
to rename it to atomic_add_unless()?
Why the built-in leading
underscores '__'? Is this function used anywhere?
Thanks,
Zuzu C.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
