[PATCH] complib/cl_timer.c: fixing cl_timer calculation

[PATCH] complib/cl_timer.c: fixing cl_timer calculation

[Yevgeny Kliteynik]

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).

Signed-off-by:  Yevgeny Kliteynik <kliteyn-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>

---
 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;

 	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;

 	/* 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] complib/cl_timer.c: fixing cl_timer calculation

[Sasha Khapyorsky]

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().

See a comments about the patch below.

> 
> Signed-off-by:  Yevgeny Kliteynik <kliteyn-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
> 
> ---
>  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);

?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] complib/cl_timer.c: fixing cl_timer calculation

[Yevgeny Kliteynik]

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().
> 
> See a comments about the patch below.
> 
>>
>> Signed-off-by:  Yevgeny Kliteynik<kliteyn-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>>
>> ---
>>   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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] complib/cl_timer.c: fixing cl_timer calculation

[Yevgeny Kliteynik]

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<kliteyn-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>>>
>>> ---
>>>    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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] complib/cl_timer.c: fixing cl_timer calculation

[Sasha Khapyorsky]

On 15:01 Tue 24 Aug     , Yevgeny Kliteynik wrote:
> 
> 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;} */

Have no idea. This is from day zero of git history.

Sasha
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] complib/cl_timer.c: fixing cl_timer calculation

[Yevgeny Kliteynik]

On 25-Aug-10 7:07 PM, Sasha Khapyorsky wrote:
> On 15:01 Tue 24 Aug     , Yevgeny Kliteynik wrote:
>>
>> 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;} */
> 
> Have no idea. This is from day zero of git history.

Well, it's not there any more.

-- Yevgeny

> Sasha
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html