On 24-Aug-10 2:55 PM, Yevgeny Kliteynik wrote:
> On 23-Aug-10 9:55 PM, Sasha Khapyorsky wrote:
>> On 17:38 Mon 23 Aug , Yevgeny Kliteynik wrote:
>>> When calculating p_timer->timeout.tv_sec and p_timer->timeout.tv_nsec,
>>> the carry was ignored, resulting in wrong value in p_timer->timeout.tv_sec,
>>> and value> 10^9 in p_timer->timeout.tv_nsec (illegal value).
>>
>> Nice catch. And I also see a similar issue in cl_timer_trim().
BTW, any idea what are the (obviously historical)
reasons for these lines in the code?
323: /* do not do 0 wait ! */
324: /* if (delta_time < 1000.0) {delta_time = 1000;} */
-- Yevgeny
>> See a comments about the patch below.
>>
>>>
>>> Signed-off-by: Yevgeny Kliteynik<[email protected]>
>>>
>>> ---
>>> opensm/complib/cl_timer.c | 8 +++++---
>>> 1 files changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/opensm/complib/cl_timer.c b/opensm/complib/cl_timer.c
>>> index 2acdb51..09a5584 100644
>>> --- a/opensm/complib/cl_timer.c
>>> +++ b/opensm/complib/cl_timer.c
>>> @@ -299,7 +299,8 @@ cl_status_t cl_timer_start(IN cl_timer_t * const
>>> p_timer,
>>> {
>>> struct timeval curtime;
>>> cl_list_item_t *p_list_item;
>>> - uint32_t delta_time = time_ms;
>>> + uint32_t delta_time_sec = time_ms / 1000;
>>> + uint32_t delta_time_usec = (time_ms % 1000) * 1000;
>>
>> In order to prevent compatibility and porting issues would it be better
>> to use struct timeval's types here:
>>
>> time_t delta_time_sec;
>> suseconds_t delta_time_usec_t delta_time_usec;
>>
>> , instead of fixed size integers?
>>
>>>
>>> CL_ASSERT(p_timer);
>>> CL_ASSERT(p_timer->state == CL_INITIALIZED);
>>> @@ -324,9 +325,10 @@ cl_status_t cl_timer_start(IN cl_timer_t * const
>>> p_timer,
>>> /* if (delta_time< 1000.0) {delta_time = 1000;} */
>>>
>>> /* Calculate the timeout. */
>>> - p_timer->timeout.tv_sec = curtime.tv_sec + (delta_time / 1000);
>>> + p_timer->timeout.tv_sec = curtime.tv_sec + delta_time_sec +
>>> + ((curtime.tv_usec + delta_time_usec) / 1000000);
>>> p_timer->timeout.tv_nsec =
>>> - (curtime.tv_usec + ((delta_time % 1000) * 1000)) * 1000;
>>> + ((curtime.tv_usec + delta_time_usec) % 1000000) * 1000;
>>
>> Or even something like this:
>>
>> strcut timeval curtime, deltatime;
>>
>> gettimeofday(&curtime, NULL);
>>
>> addtime.tv_sec = time_ms / 1000;
>> addtime.tv_usec = (time_ms % 1000) * 1000;
>>
>> timeradd(&curtime,&addtime,&timer->timeout);
>>
>> ?
>
> Sure, why not. It won't be that clean, because p_timer->timeout
> and curtime/deltatime are different types, but I get the idea.
>
> -- Yevgeny
>
>> Sasha
>>
>>>
>>> /* Add the timer to the queue. */
>>> if (cl_is_qlist_empty(&gp_timer_prov->queue)) {
>>> --
>>> 1.6.2.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
