diff mbox

[v3] linux-generic: odp_timer: warn if tick is lost

Message ID 1447153298-16211-1-git-send-email-ivan.khoronzhuk@linaro.org
State Superseded
Headers show

Commit Message

Ivan Khoronzhuk Nov. 10, 2015, 11:01 a.m. UTC
If tick is lost then application should be warned about this.
It means that actual timer resolution is worse than expected.
The default timer resolution is set 10ms, that is equal to jiffy
on most systems. The default resolution is set bearing in mind
that on a CPU runs maximum two threads that ideally fits in 10ms.
But user can change it to be smaller, in case if CPU0 is isolated
and it handles only the timer ticks. This patch helps user to set
correct timer resolution.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---

Since v1:
- changed ODP_ABORT on ODP_ERR

v1:
https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html

 platform/linux-generic/odp_timer.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Maxim Uvarov Nov. 10, 2015, 11:11 a.m. UTC | #1
On 11/10/2015 14:01, Ivan Khoronzhuk wrote:
> If tick is lost then application should be warned about this.
> It means that actual timer resolution is worse than expected.
> The default timer resolution is set 10ms, that is equal to jiffy
> on most systems. The default resolution is set bearing in mind
> that on a CPU runs maximum two threads that ideally fits in 10ms.
> But user can change it to be smaller, in case if CPU0 is isolated
> and it handles only the timer ticks. This patch helps user to set
> correct timer resolution.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
>
> Since v1:
> - changed ODP_ABORT on ODP_ERR
>
> v1:
> https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html
>
>   platform/linux-generic/odp_timer.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
> index e8f0267..aa80256 100644
> --- a/platform/linux-generic/odp_timer.c
> +++ b/platform/linux-generic/odp_timer.c
> @@ -632,7 +632,14 @@ static unsigned odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)
>   
>   static void timer_notify(sigval_t sigval)
>   {
> +	uint64_t overrun;
>   	odp_timer_pool *tp = (odp_timer_pool *)sigval.sival_ptr;
> +
> +	overrun = timer_getoverrun(tp->timerid);
   int timer_getoverrun(timer_t timerid);

Maxim.
> +	if (overrun)
> +		ODP_ERR("Error: need to increase resolution, lost tickets: %"
> +			PRIu64"\n", overrun);
> +
>   #ifdef __ARM_ARCH
>   	odp_timer *array = &tp->timers[0];
>   	uint32_t i;
Ivan Khoronzhuk Nov. 10, 2015, 11:37 a.m. UTC | #2
On 10.11.15 13:11, Maxim Uvarov wrote:
> On 11/10/2015 14:01, Ivan Khoronzhuk wrote:
>> If tick is lost then application should be warned about this.
>> It means that actual timer resolution is worse than expected.
>> The default timer resolution is set 10ms, that is equal to jiffy
>> on most systems. The default resolution is set bearing in mind
>> that on a CPU runs maximum two threads that ideally fits in 10ms.
>> But user can change it to be smaller, in case if CPU0 is isolated
>> and it handles only the timer ticks. This patch helps user to set
>> correct timer resolution.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> ---
>>
>> Since v1:
>> - changed ODP_ABORT on ODP_ERR
>>
>> v1:
>> https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html
>>
>>   platform/linux-generic/odp_timer.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
>> index e8f0267..aa80256 100644
>> --- a/platform/linux-generic/odp_timer.c
>> +++ b/platform/linux-generic/odp_timer.c
>> @@ -632,7 +632,14 @@ static unsigned odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)
>>   static void timer_notify(sigval_t sigval)
>>   {
>> +    uint64_t overrun;
>>       odp_timer_pool *tp = (odp_timer_pool *)sigval.sival_ptr;
>> +
>> +    overrun = timer_getoverrun(tp->timerid);
>    int timer_getoverrun(timer_t timerid);
>
> Maxim.

Yep. correct in v4

>> +    if (overrun)
>> +        ODP_ERR("Error: need to increase resolution, lost tickets: %"
>> +            PRIu64"\n", overrun);
>> +
>>   #ifdef __ARM_ARCH
>>       odp_timer *array = &tp->timers[0];
>>       uint32_t i;
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
Ivan Khoronzhuk Nov. 10, 2015, 12:24 p.m. UTC | #3
On 10.11.15 13:37, Ivan Khoronzhuk wrote:
>
>
> On 10.11.15 13:11, Maxim Uvarov wrote:
>> On 11/10/2015 14:01, Ivan Khoronzhuk wrote:
>>> If tick is lost then application should be warned about this.
>>> It means that actual timer resolution is worse than expected.
>>> The default timer resolution is set 10ms, that is equal to jiffy
>>> on most systems. The default resolution is set bearing in mind
>>> that on a CPU runs maximum two threads that ideally fits in 10ms.
>>> But user can change it to be smaller, in case if CPU0 is isolated
>>> and it handles only the timer ticks. This patch helps user to set
>>> correct timer resolution.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>>> ---
>>>
>>> Since v1:
>>> - changed ODP_ABORT on ODP_ERR
>>>
>>> v1:
>>> https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html
>>>
>>>   platform/linux-generic/odp_timer.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
>>> index e8f0267..aa80256 100644
>>> --- a/platform/linux-generic/odp_timer.c
>>> +++ b/platform/linux-generic/odp_timer.c
>>> @@ -632,7 +632,14 @@ static unsigned odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)
>>>   static void timer_notify(sigval_t sigval)
>>>   {
>>> +    uint64_t overrun;
>>>       odp_timer_pool *tp = (odp_timer_pool *)sigval.sival_ptr;
>>> +
>>> +    overrun = timer_getoverrun(tp->timerid);
>>    int timer_getoverrun(timer_t timerid);
>>
>> Maxim.
>
> Yep. correct in v4

I will include this patch in series with:
"[lng-odp] [PATCH] example: timer: don't set timeout less than resolution"
as both of them allow to avoid issue described at:
https://bugs.linaro.org/show_bug.cgi?id=1449

>
>>> +    if (overrun)
>>> +        ODP_ERR("Error: need to increase resolution, lost tickets: %"
>>> +            PRIu64"\n", overrun);
>>> +
>>>   #ifdef __ARM_ARCH
>>>       odp_timer *array = &tp->timers[0];
>>>       uint32_t i;
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> https://lists.linaro.org/mailman/listinfo/lng-odp
>
Ola Liljedahl Nov. 10, 2015, 2:43 p.m. UTC | #4
On 10 November 2015 at 13:24, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
wrote:

>

>

> On 10.11.15 13:37, Ivan Khoronzhuk wrote:

>

>>

>>

>> On 10.11.15 13:11, Maxim Uvarov wrote:

>>

>>> On 11/10/2015 14:01, Ivan Khoronzhuk wrote:

>>>

>>>> If tick is lost then application should be warned about this.

>>>> It means that actual timer resolution is worse than expected.

>>>> The default timer resolution is set 10ms, that is equal to jiffy

>>>> on most systems. The default resolution is set bearing in mind

>>>> that on a CPU runs maximum two threads that ideally fits in 10ms.

>>>> But user can change it to be smaller, in case if CPU0 is isolated

>>>> and it handles only the timer ticks. This patch helps user to set

>>>> correct timer resolution.

>>>>

>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>

>>>> ---

>>>>

>>>> Since v1:

>>>> - changed ODP_ABORT on ODP_ERR

>>>>

>>>> v1:

>>>> https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html

>>>>

>>>>   platform/linux-generic/odp_timer.c | 7 +++++++

>>>>   1 file changed, 7 insertions(+)

>>>>

>>>> diff --git a/platform/linux-generic/odp_timer.c

>>>> b/platform/linux-generic/odp_timer.c

>>>> index e8f0267..aa80256 100644

>>>> --- a/platform/linux-generic/odp_timer.c

>>>> +++ b/platform/linux-generic/odp_timer.c

>>>> @@ -632,7 +632,14 @@ static unsigned

>>>> odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)

>>>>   static void timer_notify(sigval_t sigval)

>>>>   {

>>>> +    uint64_t overrun;

>>>>       odp_timer_pool *tp = (odp_timer_pool *)sigval.sival_ptr;

>>>> +

>>>> +    overrun = timer_getoverrun(tp->timerid);

>>>>

>>>    int timer_getoverrun(timer_t timerid);

>>>

>>> Maxim.

>>>

>>

>> Yep. correct in v4

>>

>

> I will include this patch in series with:

> "[lng-odp] [PATCH] example: timer: don't set timeout less than resolution"

> as both of them allow to avoid issue described at:

> https://bugs.linaro.org/show_bug.cgi?id=1449

>

>

>

>> +    if (overrun)

>>>> +        ODP_ERR("Error: need to increase resolution, lost tickets: %"

>>>>

>>> tickets -> ticks

I would write: "Error: timer ticks lost, need to increase timer resolution".

What's the overrun parameter for?

Better to print the name of the timer.



> +            PRIu64"\n", overrun);

>>>> +

>>>>   #ifdef __ARM_ARCH

>>>>       odp_timer *array = &tp->timers[0];

>>>>       uint32_t i;

>>>>

>>>

>>> _______________________________________________

>>> lng-odp mailing list

>>> lng-odp@lists.linaro.org

>>> https://lists.linaro.org/mailman/listinfo/lng-odp

>>>

>>

>>

> --

> Regards,

> Ivan Khoronzhuk

> _______________________________________________

> lng-odp mailing list

> lng-odp@lists.linaro.org

> https://lists.linaro.org/mailman/listinfo/lng-odp

>
Ivan Khoronzhuk Nov. 10, 2015, 2:50 p.m. UTC | #5
On 10.11.15 16:43, Ola Liljedahl wrote:
>
>
> On 10 November 2015 at 13:24, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org <mailto:ivan.khoronzhuk@linaro.org>> wrote:
>
>
>
>     On 10.11.15 13:37, Ivan Khoronzhuk wrote:
>
>
>
>         On 10.11.15 13:11, Maxim Uvarov wrote:
>
>             On 11/10/2015 14:01, Ivan Khoronzhuk wrote:
>
>                 If tick is lost then application should be warned about this.
>                 It means that actual timer resolution is worse than expected.
>                 The default timer resolution is set 10ms, that is equal to jiffy
>                 on most systems. The default resolution is set bearing in mind
>                 that on a CPU runs maximum two threads that ideally fits in 10ms.
>                 But user can change it to be smaller, in case if CPU0 is isolated
>                 and it handles only the timer ticks. This patch helps user to set
>                 correct timer resolution.
>
>                 Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org <mailto:ivan.khoronzhuk@linaro.org>>
>                 ---
>
>                 Since v1:
>                 - changed ODP_ABORT on ODP_ERR
>
>                 v1:
>                 https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html
>
>                    platform/linux-generic/odp_timer.c | 7 +++++++
>                    1 file changed, 7 insertions(+)
>
>                 diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
>                 index e8f0267..aa80256 100644
>                 --- a/platform/linux-generic/odp_timer.c
>                 +++ b/platform/linux-generic/odp_timer.c
>                 @@ -632,7 +632,14 @@ static unsigned odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)
>                    static void timer_notify(sigval_t sigval)
>                    {
>                 +    uint64_t overrun;
>                        odp_timer_pool *tp = (odp_timer_pool *)sigval.sival_ptr;
>                 +
>                 +    overrun = timer_getoverrun(tp->timerid);
>
>                 int timer_getoverrun(timer_t timerid);
>
>             Maxim.
>
>
>         Yep. correct in v4
>
>
>     I will include this patch in series with:
>     "[lng-odp] [PATCH] example: timer: don't set timeout less than resolution"
>     as both of them allow to avoid issue described at:
>     https://bugs.linaro.org/show_bug.cgi?id=1449
>
>
>
>                 +    if (overrun)
>                 +        ODP_ERR("Error: need to increase resolution, lost tickets: %"
>
> tickets -> ticks
> I would write: "Error: timer ticks lost, need to increase timer resolution".
>
> What's the overrun parameter for?
I've sent version with corrected warn. Please look see new patch in series:
"[lng-odp] [PATCH 0/3] add warnings to improve timer usage"

overrun it's not lost, it's like late ticks....

>
> Better to print the name of the timer.

Maybe timer pool name better. The ticks are overrun for all timers in timer pool.

>
>                 +            PRIu64"\n", overrun);
>                 +
>                    #ifdef __ARM_ARCH
>                        odp_timer *array = &tp->timers[0];
>                        uint32_t i;
>
>
>             _______________________________________________
>             lng-odp mailing list
>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>             https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>     --
>     Regards,
>     Ivan Khoronzhuk
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
Ola Liljedahl Nov. 10, 2015, 2:58 p.m. UTC | #6
On 10 November 2015 at 15:50, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
wrote:

>

>

> On 10.11.15 16:43, Ola Liljedahl wrote:

>

>>

>>

>> On 10 November 2015 at 13:24, Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org

>> <mailto:ivan.khoronzhuk@linaro.org>> wrote:

>>

>>

>>

>>     On 10.11.15 13:37, Ivan Khoronzhuk wrote:

>>

>>

>>

>>         On 10.11.15 13:11, Maxim Uvarov wrote:

>>

>>             On 11/10/2015 14:01, Ivan Khoronzhuk wrote:

>>

>>                 If tick is lost then application should be warned about

>> this.

>>                 It means that actual timer resolution is worse than

>> expected.

>>                 The default timer resolution is set 10ms, that is equal

>> to jiffy

>>                 on most systems. The default resolution is set bearing in

>> mind

>>                 that on a CPU runs maximum two threads that ideally fits

>> in 10ms.

>>                 But user can change it to be smaller, in case if CPU0 is

>> isolated

>>                 and it handles only the timer ticks. This patch helps

>> user to set

>>                 correct timer resolution.

>>

>>                 Signed-off-by: Ivan Khoronzhuk <

>> ivan.khoronzhuk@linaro.org <mailto:ivan.khoronzhuk@linaro.org>>

>>

>>                 ---

>>

>>                 Since v1:

>>                 - changed ODP_ABORT on ODP_ERR

>>

>>                 v1:

>>

>> https://lists.linaro.org/pipermail/lng-odp/2015-November/017026.html

>>

>>                    platform/linux-generic/odp_timer.c | 7 +++++++

>>                    1 file changed, 7 insertions(+)

>>

>>                 diff --git a/platform/linux-generic/odp_timer.c

>> b/platform/linux-generic/odp_timer.c

>>                 index e8f0267..aa80256 100644

>>                 --- a/platform/linux-generic/odp_timer.c

>>                 +++ b/platform/linux-generic/odp_timer.c

>>                 @@ -632,7 +632,14 @@ static unsigned

>> odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)

>>                    static void timer_notify(sigval_t sigval)

>>                    {

>>                 +    uint64_t overrun;

>>                        odp_timer_pool *tp = (odp_timer_pool

>> *)sigval.sival_ptr;

>>                 +

>>                 +    overrun = timer_getoverrun(tp->timerid);

>>

>>                 int timer_getoverrun(timer_t timerid);

>>

>>             Maxim.

>>

>>

>>         Yep. correct in v4

>>

>>

>>     I will include this patch in series with:

>>     "[lng-odp] [PATCH] example: timer: don't set timeout less than

>> resolution"

>>     as both of them allow to avoid issue described at:

>>     https://bugs.linaro.org/show_bug.cgi?id=1449

>>

>>

>>

>>                 +    if (overrun)

>>                 +        ODP_ERR("Error: need to increase resolution,

>> lost tickets: %"

>>

>> tickets -> ticks

>> I would write: "Error: timer ticks lost, need to increase timer

>> resolution".

>>

>> What's the overrun parameter for?

>>

> I've sent version with corrected warn. Please look see new patch in series:

> "[lng-odp] [PATCH 0/3] add warnings to improve timer usage"

>

> overrun it's not lost, it's like late ticks....


OK I wasn't sure if timer ticks were lost or just late.
If they are just late, I think a warning should suffice.


>

>

>

>> Better to print the name of the timer.

>>

>

> Maybe timer pool name better. The ticks are overrun for all timers in

> timer pool.

>

That's what I meant. Individual timers do not have names.


>

>

>>                 +            PRIu64"\n", overrun);

>>                 +

>>                    #ifdef __ARM_ARCH

>>                        odp_timer *array = &tp->timers[0];

>>                        uint32_t i;

>>

>>

>>             _______________________________________________

>>             lng-odp mailing list

>>             lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>

>>             https://lists.linaro.org/mailman/listinfo/lng-odp

>>

>>

>>

>>     --

>>     Regards,

>>     Ivan Khoronzhuk

>>     _______________________________________________

>>     lng-odp mailing list

>>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>

>>     https://lists.linaro.org/mailman/listinfo/lng-odp

>>

>>

>>

> --

> Regards,

> Ivan Khoronzhuk

>
diff mbox

Patch

diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
index e8f0267..aa80256 100644
--- a/platform/linux-generic/odp_timer.c
+++ b/platform/linux-generic/odp_timer.c
@@ -632,7 +632,14 @@  static unsigned odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick)
 
 static void timer_notify(sigval_t sigval)
 {
+	uint64_t overrun;
 	odp_timer_pool *tp = (odp_timer_pool *)sigval.sival_ptr;
+
+	overrun = timer_getoverrun(tp->timerid);
+	if (overrun)
+		ODP_ERR("Error: need to increase resolution, lost tickets: %"
+			PRIu64"\n", overrun);
+
 #ifdef __ARM_ARCH
 	odp_timer *array = &tp->timers[0];
 	uint32_t i;