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:
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");
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
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...
Thanks,
Maxim.
On 05/10/16 09:00, Bala Manoharan wrote:
Reviewed-by: Balasubramanian Manoharan <bala.manoha...@linaro.org
<mailto:bala.manoha...@linaro.org>>
On 10 May 2016 at 07:25, Bill Fischofer <bill.fischo...@linaro.org
<mailto:bill.fischo...@linaro.org>> 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 <bill.fischo...@linaro.org
<mailto:bill.fischo...@linaro.org>>
---
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
lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
