Topics

[PATCH 1/2] arc_timer: fix tickless idle


Desfarges, Simon <simon.desfarges@...>
 

From: Simon Desfarges <simon.desfarges(a)intel.com>

When exiting from tickless idle uppon an external IRQ, the TICK timer is set
to fire at next TICK boundary. The current algorithm can lead to a point that
timer0_count register is higher than the timer0_limit register.

In this situation the next TICK will fire after a full wrapp of the counter
(~133 seconds).

This condition appears when the counter reaches the limit after the Interrupt
Pending flag is checked. At this point the counter is automatically wrapped to
0, but is set just next the limit to fire at next TICK boundary by SW.
At exit of the _timer_idle_exit function, the timer handler is called, and sets
the limit to 1 TICK. At this point the situation is:
- limit register == 1 TICK
- count register is just below the old limit register and higher than 1 TICK

To fix this issue, at _timer_idle_exit, the limit register is always set to 1 TICK and
the count register set such as the next TICK fires on time.

Change-Id: Ifa002809d426aa04109592e53d2b02a224f51101
Tracked-On: https://jira.ndg.intel.com/browse/FIRE-4213
Signed-off-by: Simon Desfarges <simon.desfarges(a)intel.com>
---
drivers/timer/arcv2_timer0.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/timer/arcv2_timer0.c b/drivers/timer/arcv2_timer0.c
index 932ceac..5fec988 100644
--- a/drivers/timer/arcv2_timer0.c
+++ b/drivers/timer/arcv2_timer0.c
@@ -220,7 +220,7 @@ void _timer_idle_enter(int32_t ticks)
}

programmed_ticks = ticks;
- programmed_limit = (programmed_ticks * cycles_per_tick);
+ programmed_limit = (programmed_ticks * cycles_per_tick) - 1;

timer0_limit_register_set(programmed_limit);

@@ -276,9 +276,6 @@ void _timer_idle_exit(void)
* A non-timer interrupt occurred. Announce any
* ticks that have elapsed during the tickless idle.
*/
-
- uint32_t remaining_cycles = programmed_limit - current_count;
-
_sys_idle_elapsed_ticks = current_count / cycles_per_tick;
if (_sys_idle_elapsed_ticks > 0) {
update_accumulated_count();
@@ -289,11 +286,8 @@ void _timer_idle_exit(void)
* Ensure the timer will expire at the end of the next tick in case
* the ISR makes any tasks and/or fibers ready to run.
*/
-
- if (remaining_cycles >= cycles_per_tick) {
- timer0_count_register_set(programmed_limit -
- ((remaining_cycles - 1) % cycles_per_tick) - 1);
- }
+ timer0_limit_register_set(cycles_per_tick - 1);
+ timer0_count_register_set(timer0_count_register_get() % cycles_per_tick);
}
#else
static void tickless_idle_init(void) {}
--
1.9.1


Mitsis, Peter <Peter.Mitsis@...>
 

See [Peter]

-----Original Message-----
From: simon.desfarges(a)intel.com [mailto:simon.desfarges(a)intel.com]
Sent: March-17-16 2:42 PM
To: devel(a)lists.zephyrproject.org
Cc: DESFARGES, SIMON
Subject: [devel] [PATCH 1/2] arc_timer: fix tickless idle

From: Simon Desfarges <simon.desfarges(a)intel.com>

When exiting from tickless idle uppon an external IRQ, the TICK timer is
set to fire at next TICK boundary. The current algorithm can lead to a
point that timer0_count register is higher than the timer0_limit register.

In this situation the next TICK will fire after a full wrapp of the
counter
(~133 seconds).

This condition appears when the counter reaches the limit after the
Interrupt Pending flag is checked. At this point the counter is
automatically wrapped to 0, but is set just next the limit to fire at next
TICK boundary by SW.
At exit of the _timer_idle_exit function, the timer handler is called, and
sets the limit to 1 TICK. At this point the situation is:
- limit register == 1 TICK
- count register is just below the old limit register and higher than 1
TICK

To fix this issue, at _timer_idle_exit, the limit register is always set
to 1 TICK and the count register set such as the next TICK fires on time.

Change-Id: Ifa002809d426aa04109592e53d2b02a224f51101
Tracked-On: https://jira.ndg.intel.com/browse/FIRE-4213
Signed-off-by: Simon Desfarges <simon.desfarges(a)intel.com>
---
drivers/timer/arcv2_timer0.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/timer/arcv2_timer0.c b/drivers/timer/arcv2_timer0.c
index 932ceac..5fec988 100644
--- a/drivers/timer/arcv2_timer0.c
+++ b/drivers/timer/arcv2_timer0.c
@@ -220,7 +220,7 @@ void _timer_idle_enter(int32_t ticks)
}

programmed_ticks = ticks;
- programmed_limit = (programmed_ticks * cycles_per_tick);
+ programmed_limit = (programmed_ticks * cycles_per_tick) - 1;
[Peter]: Good catch on this off by one error.

timer0_limit_register_set(programmed_limit);

@@ -276,9 +276,6 @@ void _timer_idle_exit(void)
* A non-timer interrupt occurred. Announce any
* ticks that have elapsed during the tickless idle.
*/
-
- uint32_t remaining_cycles = programmed_limit - current_count;
-
_sys_idle_elapsed_ticks = current_count / cycles_per_tick;
if (_sys_idle_elapsed_ticks > 0) {
update_accumulated_count();
@@ -289,11 +286,8 @@ void _timer_idle_exit(void)
* Ensure the timer will expire at the end of the next tick in case
* the ISR makes any tasks and/or fibers ready to run.
*/
-
- if (remaining_cycles >= cycles_per_tick) {
- timer0_count_register_set(programmed_limit -
- ((remaining_cycles - 1) % cycles_per_tick) - 1);
- }
+ timer0_limit_register_set(cycles_per_tick - 1);
+ timer0_count_register_set(timer0_count_register_get() %
+cycles_per_tick);
[Peter]:

First an historical note. The reason the original code only touched the count register was that it was not known if lowering the limit register to a value below that of the count register would trigger an interrupt. (I did not have access to a board with the ARCv2 timer and so was unable to confirm or deny that behavior.) That being said, if changing the limit value from a value higher than the count to a value lower than the count does not generate an interrupt, then I have no objections on that front.

Second, Using timer0_count_register_get() at this point is a no-no as it introduces a race condition that can cause us to lose track of a whole tick. Remember, the timer is always counting. If the elapsed ticks were announced just prior to a "tick boundary", the call to timer0_count_register_set() could then occur just after the "tick boundary". The result of this scenario is that nearly one whole tick may be lost when the modulus operation is performed. This is why we use the <current_count> retrieved from the start of _timer_idle_exit(). Yes, it does introduce some gradual drift, but this can be ameliorated somewhat by only modifying the count if there is more than one tick left in the tickles idle.

Hope this helps.

}
#else
static void tickless_idle_init(void) {}
--
1.9.1


Desfarges, Simon <simon.desfarges@...>
 

Hello,
Thanks for the answer, I will review my copy.

BTW, the same test bench shows no jitter in the Quark TICK. So I would like to have the same behavior for ARC.
Do you think it is achievable ? I have doubts because of the fact that loapic decrements its counter (logic is reversed)
and looks to be safer.

See my [Simon] below.

-----Original Message-----
From: Mitsis, Peter [mailto:Peter.Mitsis(a)windriver.com]
Sent: Thursday, March 17, 2016 9:19 PM
To: Desfarges, Simon <simon.desfarges(a)intel.com>;
devel(a)lists.zephyrproject.org
Cc: Desfarges, Simon <simon.desfarges(a)intel.com>
Subject: RE: [devel] [PATCH 1/2] arc_timer: fix tickless idle

See [Peter]

-----Original Message-----
From: simon.desfarges(a)intel.com [mailto:simon.desfarges(a)intel.com]
Sent: March-17-16 2:42 PM
To: devel(a)lists.zephyrproject.org
Cc: DESFARGES, SIMON
Subject: [devel] [PATCH 1/2] arc_timer: fix tickless idle

From: Simon Desfarges <simon.desfarges(a)intel.com>

When exiting from tickless idle uppon an external IRQ, the TICK timer is
set to fire at next TICK boundary. The current algorithm can lead to a
point that timer0_count register is higher than the timer0_limit register.

In this situation the next TICK will fire after a full wrapp of the
counter
(~133 seconds).

This condition appears when the counter reaches the limit after the
Interrupt Pending flag is checked. At this point the counter is
automatically wrapped to 0, but is set just next the limit to fire at next
TICK boundary by SW.
At exit of the _timer_idle_exit function, the timer handler is called, and
sets the limit to 1 TICK. At this point the situation is:
- limit register == 1 TICK
- count register is just below the old limit register and higher than 1
TICK

To fix this issue, at _timer_idle_exit, the limit register is always set
to 1 TICK and the count register set such as the next TICK fires on time.

Change-Id: Ifa002809d426aa04109592e53d2b02a224f51101
Tracked-On: https://jira.ndg.intel.com/browse/FIRE-4213
Signed-off-by: Simon Desfarges <simon.desfarges(a)intel.com>
---
drivers/timer/arcv2_timer0.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/timer/arcv2_timer0.c b/drivers/timer/arcv2_timer0.c
index 932ceac..5fec988 100644
--- a/drivers/timer/arcv2_timer0.c
+++ b/drivers/timer/arcv2_timer0.c
@@ -220,7 +220,7 @@ void _timer_idle_enter(int32_t ticks)
}

programmed_ticks = ticks;
- programmed_limit = (programmed_ticks * cycles_per_tick);
+ programmed_limit = (programmed_ticks * cycles_per_tick) - 1;
[Peter]: Good catch on this off by one error.
[Simon] I will separate in another patch and try to make it merged asap so we can forget this point.

timer0_limit_register_set(programmed_limit);

@@ -276,9 +276,6 @@ void _timer_idle_exit(void)
* A non-timer interrupt occurred. Announce any
* ticks that have elapsed during the tickless idle.
*/
-
- uint32_t remaining_cycles = programmed_limit - current_count;
-
_sys_idle_elapsed_ticks = current_count / cycles_per_tick;
if (_sys_idle_elapsed_ticks > 0) {
update_accumulated_count();
@@ -289,11 +286,8 @@ void _timer_idle_exit(void)
* Ensure the timer will expire at the end of the next tick in case
* the ISR makes any tasks and/or fibers ready to run.
*/
-
- if (remaining_cycles >= cycles_per_tick) {
- timer0_count_register_set(programmed_limit -
- ((remaining_cycles - 1) % cycles_per_tick) - 1);
- }
+ timer0_limit_register_set(cycles_per_tick - 1);
+ timer0_count_register_set(timer0_count_register_get() %
+cycles_per_tick);
[Peter]:

First an historical note. The reason the original code only touched the count
register was that it was not known if lowering the limit register to a value
below that of the count register would trigger an interrupt. (I did not have
access to a board with the ARCv2 timer and so was unable to confirm or deny
that behavior.) That being said, if changing the limit value from a value higher
than the count to a value lower than the count does not generate an
interrupt, then I have no objections on that front.
[Simon] This is the case I experimented. The ISR is generated only if count == limit.


Second, Using timer0_count_register_get() at this point is a no-no as it
introduces a race condition that can cause us to lose track of a whole tick.
Remember, the timer is always counting. If the elapsed ticks were
announced just prior to a "tick boundary", the call to
timer0_count_register_set() could then occur just after the "tick boundary".
The result of this scenario is that nearly one whole tick may be lost when the
modulus operation is performed. This is why we use the <current_count>
retrieved from the start of _timer_idle_exit(). Yes, it does introduce some
gradual drift, but this can be ameliorated somewhat by only modifying the
count if there is more than one tick left in the tickles idle.
[Simon]
I especially call this function to limit this gradual drift. As far as I understand, in every point of this function,
it is possible to have a race condition or an overlap of the counter. The case where the counter overlaps
after checking the 'IP' bit and before setting the count register is handled, because the ISR will fire anyway
and 1 tick will be announced.

I see in the loapic timer there is a 'timer_known_to_have_expired' variable, handling this exact case,
I will take a look at it (I guess you already did it...).


Hope this helps.

}
#else
static void tickless_idle_init(void) {}
--
1.9.1