On Thu, Sep 4, 2014 at 8:42 AM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> Small nits below, otherwise looks good to me (even though I’m not versed in
> MSVC),
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
>
> On Sep 3, 2014, at 1:08 PM, Gurucharan Shetty <shet...@nicira.com> wrote:
>> +
>> +/* 64 bit writes are atomic on i586 if 64 bit aligned. */
>> +#define atomic_store64(DST, SRC, ORDER) \
>> + if (((size_t) (DST) & (sizeof *(DST) - 1)) == 0) { \
>> + if (ORDER == memory_order_seq_cst) { \
>> + InterlockedExchange64((int64_t volatile *) (DST), \
>> + (int64_t) (SRC)); \
>> + } else { \
>> + *(DST) = (SRC); \
>> + } \
>> + } else { \
>> + abort(); \
>> + }
>> +
>
> How about this instead:
>
> /* 64 bit writes are atomic on i586 if 64 bit aligned. */
> #define atomic_store64(DST, SRC, ORDER) \
> if (((size_t) (DST) & (sizeof *(DST) - 1)) \
> || ORDER == memory_order_seq_cst) { \
> InterlockedExchange64((int64_t volatile *) (DST), \
> (int64_t) (SRC)); \
> } else { \
> *(DST) = (SRC); \
> } \
>
> However, you should probably keep the version in your patch if you know that
> the alignment check can be computed at compile-time and/or there is some
> documentation that MSVC will not align 64-bit units on at 4-byte alignment.
Ben pointed out as a reply that MSVC will not align 64-bit units on
4-byte alignment for x86. (Thanks Ben. I looked for that but hadn't
found that page).
So that means that your suggestion would actually work because it
calls InterlockedExchange64() when the data is not 64 bit aligned. So
I will use your version. (I suppose I understood the implications
correctly?)
>> +/* Arithmetic subtraction calls. */
>> +
>> +#define atomic_sub(RMW, ARG, ORIG) \
>> + atomic_add_explicit(RMW, (0 - ARG), ORIG, memory_order_seq_cst)
>
> ARG should be in parenthesis: -(ARG). For example, if ARG was “x - 5” we
> should add -x + 5, not -x - 5.
Oops.
>
>> +
>> +#define atomic_sub_explicit(RMW, ARG, ORIG, ORDER) \
>> + atomic_add_explicit(RMW, (0 - ARG), ORIG, ORDER)
>
> Same here.
Thanks. I will make the change.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
