Re: [Xen-devel] [PATCH v2 4/5] asm/atomic.h: common prototyping (add xen/atomic.h)

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel