[v2,3/5] linux-generic: odp_time: reutrn 0 if t2 = t1 instead of MAX for diff

It's better to describe by example:

cur = 15;
start = 15;

Petri,

What about this fix? It's similar to to CPU API.

On 11.09.15 13:04, Ivan Khoronzhuk wrote:
> It's better to describe by example:
>
> cur = 15;
> start = 15;
> diff = 2;
> while (odp_time_cycles_diff(start, cur) < diff) {
> 	cur = odp_time_cycles();
> }
>
> This example has to work. It's possible only when t2 - t1 = 0 if t2 = t1.
> The validation test on it will be added later.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>   platform/linux-generic/odp_time.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c
> index a08833d..a007d69 100644
> --- a/platform/linux-generic/odp_time.c
> +++ b/platform/linux-generic/odp_time.c
> @@ -14,7 +14,7 @@
>
>   uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2)
>   {
> -	if (odp_likely(t2 > t1))
> +	if (odp_likely(t2 >= t1))
>   		return t2 - t1;
>
>   	return t2 + (UINT64_MAX - t1);
>

But I have additional proposition. Maybe I'm wrong, but
one cycle can be lost here (equal as in CPU api, I'm ready to fix it also)

For instance:

start = MAX - 2;
cur = 1

res = MAX - MAX + 2 + 1 = 3;
It's correct. But in real it will be:

(MAX - 2)
           -> 1 cycle
(MAX - 1)
           -> 2 cycle
MAX
           -> 3 cycle
0
           -> 4 cycle
1

The function returns 3 cycles difference,
but due to 0, physically, timer counts 4 cycles.

Not sure, but I should send +1 patch that corrects it to:

return t2 + (UINT64_MAX - t1) + 1;

due to counter in continuous mode is reset to 0, then continues counting.

Can we apply this on cycle counter? (then I need correct CPU API implementation also)
Is it reseted to zero or wraps to 1 for all arches?

On 16.09.15 13:23, Ivan Khoronzhuk wrote:
> Petri,
>
> What about this fix? It's similar to to CPU API.
>
> On 11.09.15 13:04, Ivan Khoronzhuk wrote:
>> It's better to describe by example:
>>
>> cur = 15;
>> start = 15;
>> diff = 2;
>> while (odp_time_cycles_diff(start, cur) < diff) {
>>     cur = odp_time_cycles();
>> }
>>
>> This example has to work. It's possible only when t2 - t1 = 0 if t2 = t1.
>> The validation test on it will be added later.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>   platform/linux-generic/odp_time.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/platform/linux-generic/odp_time.c b/platform/linux-generic/odp_time.c
>> index a08833d..a007d69 100644
>> --- a/platform/linux-generic/odp_time.c
>> +++ b/platform/linux-generic/odp_time.c
>> @@ -14,7 +14,7 @@
>>
>>   uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2)
>>   {
>> -    if (odp_likely(t2 > t1))
>> +    if (odp_likely(t2 >= t1))
>>           return t2 - t1;
>>
>>       return t2 + (UINT64_MAX - t1);
>>
>
> But I have additional proposition. Maybe I'm wrong, but
> one cycle can be lost here (equal as in CPU api, I'm ready to fix it also)
>
> For instance:
>
> start = MAX - 2;
> cur = 1
>
> res = MAX - MAX + 2 + 1 = 3;
> It's correct. But in real it will be:
>
> (MAX - 2)
>            -> 1 cycle
> (MAX - 1)
>            -> 2 cycle
> MAX
>            -> 3 cycle
> 0
>            -> 4 cycle
> 1
>
> The function returns 3 cycles difference,
> but due to 0, physically, timer counts 4 cycles.
>
> Not sure, but I should send +1 patch that corrects it to:
>
> return t2 + (UINT64_MAX - t1) + 1;
>
> due to counter in continuous mode is reset to 0, then continues counting.
>
> Can we apply this on cycle counter? (then I need correct CPU API implementation also)
> Is it reseted to zero or wraps to 1 for all arches?
>
>

For instance, from here http://download.intel.com/design/intelxscale/27347302.pdf,
(Intel XScale® Core) CCNT behaves like:

"When CCNT reaches its maximum value 0xFFFF,FFFF, the next clock
cycle will cause it to roll over to zero"

But I'm not sure about other arches hidden under linux-generic.
I tend to believe that it's applicable for all cases.

> -----Original Message-----
> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org]
> Sent: Wednesday, September 16, 2015 1:47 PM
> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo)
> Subject: Re: [lng-odp] [PATCH v2 3/5] linux-generic: odp_time: reutrn 0
> if t2 = t1 instead of MAX for diff
> 
> On 16.09.15 13:23, Ivan Khoronzhuk wrote:
> > Petri,
> >
> > What about this fix? It's similar to to CPU API.
> >
> > On 11.09.15 13:04, Ivan Khoronzhuk wrote:
> >> It's better to describe by example:
> >>
> >> cur = 15;
> >> start = 15;
> >> diff = 2;
> >> while (odp_time_cycles_diff(start, cur) < diff) {
> >>     cur = odp_time_cycles();
> >> }
> >>
> >> This example has to work. It's possible only when t2 - t1 = 0 if t2
> = t1.
> >> The validation test on it will be added later.
> >>
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> ---
> >>   platform/linux-generic/odp_time.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/platform/linux-generic/odp_time.c b/platform/linux-
> generic/odp_time.c
> >> index a08833d..a007d69 100644
> >> --- a/platform/linux-generic/odp_time.c
> >> +++ b/platform/linux-generic/odp_time.c
> >> @@ -14,7 +14,7 @@
> >>
> >>   uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2)
> >>   {
> >> -    if (odp_likely(t2 > t1))
> >> +    if (odp_likely(t2 >= t1))
> >>           return t2 - t1;
> >>
> >>       return t2 + (UINT64_MAX - t1);
> >>
> >
> > But I have additional proposition. Maybe I'm wrong, but
> > one cycle can be lost here (equal as in CPU api, I'm ready to fix it
> also)
> >
> > For instance:
> >
> > start = MAX - 2;
> > cur = 1
> >
> > res = MAX - MAX + 2 + 1 = 3;
> > It's correct. But in real it will be:
> >
> > (MAX - 2)
> >            -> 1 cycle
> > (MAX - 1)
> >            -> 2 cycle
> > MAX
> >            -> 3 cycle
> > 0
> >            -> 4 cycle
> > 1
> >
> > The function returns 3 cycles difference,
> > but due to 0, physically, timer counts 4 cycles.
> >
> > Not sure, but I should send +1 patch that corrects it to:
> >
> > return t2 + (UINT64_MAX - t1) + 1;
> >
> > due to counter in continuous mode is reset to 0, then continues
> counting.
> >
> > Can we apply this on cycle counter? (then I need correct CPU API
> implementation also)
> > Is it reseted to zero or wraps to 1 for all arches?
> >
> >
> 
> For instance, from here
> http://download.intel.com/design/intelxscale/27347302.pdf,
> (Intel XScale(r) Core) CCNT behaves like:
> 
> "When CCNT reaches its maximum value 0xFFFF,FFFF, the next clock
> cycle will cause it to roll over to zero"
> 
> But I'm not sure about other arches hidden under linux-generic.
> I tend to believe that it's applicable for all cases.
> 
> --
> Regards,
> Ivan Khoronzhuk


uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2)
{
	if (odp_unlikely(t2 == t1))
		return 0;

	if (odp_likely(t2 > t1))
		return t2 - t1;

       return t2 + (UINT64_MAX - t1) + 1;
}


t1 = MAX

t2 = MAX - 1, diff = MAX - 1 + MAX - MAX + 1 = MAX
t2 = MAX,     diff = 0
t2 = 0,       diff = 0 + MAX - MAX + 1 = 1


t1 = MAX - 1

t2 = MAX - 2, diff = MAX - 2 + MAX - MAX + 1 + 1 = MAX
t2 = MAX - 1, diff = 0
t2 = MAX,     diff = MAX - MAX + 1 = 1
t2 = 0,       diff = 0 + MAX - MAX + 1 + 1 = 2
t2 = 1,       diff = 1 + MAX - MAX + 1 + 1 = 3


It's very likely that when t1 == t2, the correct result is 0. Let's catch that first. Wrap around case can have then the extra +1. 


-Petri

On 16.09.15 14:39, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>
>> -----Original Message-----
>> From: EXT Ivan Khoronzhuk [mailto:ivan.khoronzhuk@linaro.org]
>> Sent: Wednesday, September 16, 2015 1:47 PM
>> To: lng-odp@lists.linaro.org; Savolainen, Petri (Nokia - FI/Espoo)
>> Subject: Re: [lng-odp] [PATCH v2 3/5] linux-generic: odp_time: reutrn 0
>> if t2 = t1 instead of MAX for diff
>>
>> On 16.09.15 13:23, Ivan Khoronzhuk wrote:
>>> Petri,
>>>
>>> What about this fix? It's similar to to CPU API.
>>>
>>> On 11.09.15 13:04, Ivan Khoronzhuk wrote:
>>>> It's better to describe by example:
>>>>
>>>> cur = 15;
>>>> start = 15;
>>>> diff = 2;
>>>> while (odp_time_cycles_diff(start, cur) < diff) {
>>>>      cur = odp_time_cycles();
>>>> }
>>>>
>>>> This example has to work. It's possible only when t2 - t1 = 0 if t2
>> = t1.
>>>> The validation test on it will be added later.
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>>> ---
>>>>    platform/linux-generic/odp_time.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/platform/linux-generic/odp_time.c b/platform/linux-
>> generic/odp_time.c
>>>> index a08833d..a007d69 100644
>>>> --- a/platform/linux-generic/odp_time.c
>>>> +++ b/platform/linux-generic/odp_time.c
>>>> @@ -14,7 +14,7 @@
>>>>
>>>>    uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2)
>>>>    {
>>>> -    if (odp_likely(t2 > t1))
>>>> +    if (odp_likely(t2 >= t1))
>>>>            return t2 - t1;
>>>>
>>>>        return t2 + (UINT64_MAX - t1);
>>>>
>>>
>>> But I have additional proposition. Maybe I'm wrong, but
>>> one cycle can be lost here (equal as in CPU api, I'm ready to fix it
>> also)
>>>
>>> For instance:
>>>
>>> start = MAX - 2;
>>> cur = 1
>>>
>>> res = MAX - MAX + 2 + 1 = 3;
>>> It's correct. But in real it will be:
>>>
>>> (MAX - 2)
>>>             -> 1 cycle
>>> (MAX - 1)
>>>             -> 2 cycle
>>> MAX
>>>             -> 3 cycle
>>> 0
>>>             -> 4 cycle
>>> 1
>>>
>>> The function returns 3 cycles difference,
>>> but due to 0, physically, timer counts 4 cycles.
>>>
>>> Not sure, but I should send +1 patch that corrects it to:
>>>
>>> return t2 + (UINT64_MAX - t1) + 1;
>>>
>>> due to counter in continuous mode is reset to 0, then continues
>> counting.
>>>
>>> Can we apply this on cycle counter? (then I need correct CPU API
>> implementation also)
>>> Is it reseted to zero or wraps to 1 for all arches?
>>>
>>>
>>
>> For instance, from here
>> http://download.intel.com/design/intelxscale/27347302.pdf,
>> (Intel XScale(r) Core) CCNT behaves like:
>>
>> "When CCNT reaches its maximum value 0xFFFF,FFFF, the next clock
>> cycle will cause it to roll over to zero"
>>
>> But I'm not sure about other arches hidden under linux-generic.
>> I tend to believe that it's applicable for all cases.
>>
>> --
>> Regards,
>> Ivan Khoronzhuk
>
>
> uint64_t odp_time_diff_cycles(uint64_t t1, uint64_t t2)
> {
> 	if (odp_unlikely(t2 == t1))
> 		return 0;
>
> 	if (odp_likely(t2 > t1))
> 		return t2 - t1;

Sorry, but why not
if (odp_likely(t2 >= t1))
	return t2 - t1;

as in patch.

>
>         return t2 + (UINT64_MAX - t1) + 1;
> }
>
>
> t1 = MAX
>
> t2 = MAX - 1, diff = MAX - 1 + MAX - MAX + 1 = MAX
True, as we count +1 cycle in real.
Ok.

> t2 = MAX,     diff = 0
True.

> t2 = 0,       diff = 0 + MAX - MAX + 1 = 1
Also true. As for count from MAX to 0 we need 1 cycle.
Ok.

>
>
> t1 = MAX - 1
> t2 = MAX - 2, diff = MAX - 2 + MAX - MAX + 1 + 1 = MAX
True and OK.

> t2 = MAX - 1, diff = 0
True and OK.

> t2 = MAX,     diff = MAX - MAX + 1 = 1
True and OK.

> t2 = 0,       diff = 0 + MAX - MAX + 1 + 1 = 2
True, as we need 2 cycles to reach 0.
Ok.

> t2 = 1,       diff = 1 + MAX - MAX + 1 + 1 = 3
True, as we need 3 cycles to reach 1.
Ok.

>
>
> It's very likely that when t1 == t2, the correct result is 0.
Yep, I'm not proposing to change this patch. I propose to send additional patch later.
It will be rather series, as I want to correct cpu API also.

  Let's catch that first. Wrap around case can have then the extra +1.
Right, that is I worry about. But seems on current arches it's applicable.
Also we can limit it to arches we are sure, as -1 cycle it's obvious error now and
it adds additional error in accuracy +-2 cycles instead of 1. In case of bad cycle counter
resolution, say 64, it can have valuable impact, especially if it's multiplied on some value.

>
>
> -Petri
>
>
>
>