On Tue, May 10, 2016 at 11:19 AM, Maxim Uvarov <maxim.uva...@linaro.org>
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.
>
> 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 <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
>
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
