On 05/11/16 00:50, Bill Fischofer wrote:
On Tue, May 10, 2016 at 11:19 AM, Maxim Uvarov
<[email protected] <mailto:[email protected]>> wrote:
Ola, can you please review this patch?
Bill, Bala,
I am not sure that this change is correct.
There is 2 things:
1. Align on 16 bytes:
The entire struct will be aligned on a 16 byte boundary if
ODP_ATOMIC_U128 is defined. Otherwise it is 8 byte aligned because
of the definition of odp_atomic_u64_t:
struct odp_atomic_u64_s {
uint64_t v; /**< Actual storage for the atomic variable */
#if __GCC_ATOMIC_LLONG_LOCK_FREE < 2
/* Some architectures do not support lock-free operations on 64-bit
* data types. We use a spin lock to ensure atomicity. */
char lock; /**< Spin lock (if needed) used to ensure atomic access */
#endif
} ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */;
typedef struct tick_buf_s {
odp_atomic_u64_t exp_tck;/* Expiration tick or TMO_xxx */
odp_buffer_t tmo_buf;/* ODP_BUFFER_INVALID if timer not active */
#ifdef TB_NEEDS_PAD
uint32_t pad;/* Need to be able to access padding for
successful CAS */
#endif
} tick_buf_t
#ifdef ODP_ATOMIC_U128
ODP_ALIGNED(16) /* 16-byte atomic operations need properly aligned
addresses */
#endif
;
2. Static assert that there is exactly 16 bytes:
ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) ==
16");
This is the bug. When __GCC_ATOMIC_LLONG_LOCK_FREE < 2 is true an
extra char is inserted into the struct which pushes the total length
to be > 16 bytes. The original bug arises because this is false when
compiling with GCC but true when compiling with clang. In fact, when
compiling with clang on Ubuntu 32-bit sizeof(tick_buf_t) == 24.
Yes, I but I calculated 21, maybe missed something:
struct odp_atomic_u64_s {
uint64_t v; // 8 bytes
#if __GCC_ATOMIC_LLONG_LOCK_FREE < 2
char lock; // 9 bytes
#endif
} ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */;
typedef struct tick_buf_s {
odp_atomic_u64_t exp_tck; // 8 or 9 bytes
odp_buffer_t tmo_buf; // 8 bytes due to it's pointer
#ifdef TB_NEEDS_PAD
uint32_t pad; // 4 bytes
#endif
} tick_buf_t
#ifdef ODP_ATOMIC_U128
ODP_ALIGNED(16)
#endif
;
So maximum sizeof(tick_buf_t) is 8 + 9 + 4 = 21 bytes. So usage of 128
bit cmp operation is not valid.
_odp_atomic_u128_cmp_xchg_mm() function is not defined anywhere so this
branch inside ifdef looks like dead :)
So we should not go to that code anyhow so that removing static_assert
might works for us.
So I think that right fix what we can do now is:
1. remove static_assert completely.
2. remove currently dead code under #ifdef ODP_ATOMIC_U128
Do you agree?
Maxim.
As I understand from code, Ola did that to use u128 functions for
atomic exchange and
compare. For example here:
tick_buf_t new, old;
_odp_atomic_u128_xchg_mm((_odp_atomic_u128_t *)tb,
(_uint128_t *)&new,
(_uint128_t *)&old,
_ODP_MEMMODEL_ACQ_RLS);
That assumes that this structure has to be exactly or less (in
that case adding zero padding in the end) 16 bytes,
I.e. 16 bytes * 8 bits = 1 u128_t
No, it simply requires that the struct be at least 16 bytes long,
which is why it works with clang if the ODP_STATIC_ASSERT is corrected.
By modifying this assert you hide problem that tail of function
can not be copied/compared, makes code
unpredictable and this static assert useless. Of course we can
fall to memcpy() if struct is lager than 16 bytes,
but gain of current optimisation can be lost. So we have to think
about real fix here...
The tail processing is identical if TB_NEEDS_PAD is true. Under clang
this in fact is false so there is no tail and the copy code disappears.
I can't say I'm completely happy with the code even before this fix,
but the fix is needed because you cannot force this struct as written
to be exactly 16 bytes in all cases. This was the simplest fix and the
result is that the timer tests pass in both 32-bit and 64-bit
environments after it is applied. If there is a problem that the
timer tests aren't catching then that's a bug against the timer tests,
not the implementation, but I don't see any timer failures.
Thanks,
Maxim.
On 05/10/16 09:00, Bala Manoharan wrote:
Reviewed-by: Balasubramanian Manoharan
<[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>>
On 10 May 2016 at 07:25, Bill Fischofer
<[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>> wrote:
The tick_buf_t struct may be larger than 16 bytes when a
lock char is
needed so correct the ODP_STATIC_ASSERT to reflect this. This
addresses
bug https://bugs.linaro.org/show_bug.cgi?id=2211 when
compiling with
clang.
Signed-off-by: Bill Fischofer <[email protected]
<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>>
---
platform/linux-generic/odp_timer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/platform/linux-generic/odp_timer.c
b/platform/linux-generic/odp_timer.c
index f4fb1f6..89ec5f5 100644
--- a/platform/linux-generic/odp_timer.c
+++ b/platform/linux-generic/odp_timer.c
@@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic
operations
need properly aligned addresses */
#endif
;
-ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16,
"sizeof(tick_buf_t)
== 16");
+ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16,
"sizeof(tick_buf_t)
>= 16");
typedef struct odp_timer_s {
void *user_ptr;
--
2.7.4
_______________________________________________
lng-odp mailing list
[email protected] <mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected] <mailto:[email protected]>
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected] <mailto:[email protected]>
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
