shorten overrun loops of periodic timers

RFC PATCH In review Jan Kiszka 3 participants 11 messages Aug 3, 2006 – Aug 18, 2006

lore

Share & tools

lore

Jan Kiszka

author

2006-08-03 07:01 UTC #0 lore raw

RFC PATCH 4/4 shorten overrun loops of periodic timers

A simple patch, just like suggested by Gilles, to avoid looping over periodic xntimer handlers in case of overruns.

It saves the current TSC on loop entry and uses this value later when forwarding the timer. Is is the overhead of re-reading the TSC on all archs negligible and should we rather go that way?

Jan

---
 ksrc/nucleus/timer.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

ksrc/nucleus/timer.c +5 -4

Index: xenomai/ksrc/nucleus/timer.c
===================================================================
@@ -200,12 +200,13 @@ static void xntimer_do_tick_aperiodic(vo
 	xntimerq_t *timerq = &sched->timerqueue;
 	xntimerh_t *holder;
 	xntimer_t *timer;
+	xnticks_t now;
 
 	while ((holder = xntimerq_head(timerq)) != NULL) {
 		timer = aplink2timer(holder);
 
-		if (xntimerh_date(&timer->aplink) - nkschedlat >
-		    xnarch_get_cpu_tsc())
+		now = xnarch_get_cpu_tsc();
+		if (xntimerh_date(&timer->aplink) - nkschedlat > now)
 			/* No need to continue in aperiodic mode since timeout
 			   dates are ordered by increasing values. */
 			break;

@@ -236,8 +237,8 @@ static void xntimer_do_tick_aperiodic(vo
 				}
 #endif /* CONFIG_XENO_OPT_DEBUG || __XENO_SIM__ */
 			} else if (timer->interval == XN_INFINITE) {
-				xntimerh_date(&timer->aplink) +=
-				    nkpod->htimer.interval;
+				while ((xntimerh_date(&timer->aplink) +=
+				        nkpod->htimer.interval) < now);
 				continue;
 			}
 		} else

Philippe Gerum

2006-08-03 17:01 UTC #1 lore raw

Quoted 7 lines · Jan Kiszka

A simple patch, just like suggested by Gilles, to avoid looping over periodic xntimer handlers in case of overruns.
It saves the current TSC on loop entry and uses this value later when forwarding the timer. Is is the overhead of re-reading the TSC on all archs negligible and should we rather go that way?

The configuration for which reading the TSC is fairly expensive is x86 +TSC emulation, since this incurs reading and writing a couple of 8254-related i/o ports. This is not an issue for other archs/x86-configs.

Quoted 5 lines

Jan
_______________________________________________
Xenomai-core mailing list
[email protected]
https://mail.gna.org/listinfo/xenomai-core

Signature · 1 line

Philippe.

Gilles Chanteperdrix

2006-08-03 17:26 UTC #2 lore raw

In reply to Jan Kiszka

ksrc/nucleus/timer.c static void xntimer_do_tick_aperiodic(vo · line 241

Quoted 12 lines · Jan Kiszka

A simple patch, just like suggested by Gilles, to avoid looping over periodic xntimer handlers in case of overruns.
It saves the current TSC on loop entry and uses this value later when forwarding the timer. Is is the overhead of re-reading the TSC on all archs negligible and should we rather go that way?
(...)
ksrc/nucleus/timer.c +2 -2
-				xntimerh_date(&timer->aplink) +=
-				    nkpod->htimer.interval;
+				while ((xntimerh_date(&timer->aplink) +=
+				        nkpod->htimer.interval) < now);

I think you are patching the wrong addition, the one you are interested in is most probably the one at the bottom of xntimer_do_tick_aperiodic.

Signature · 1 line

					    Gilles Chanteperdrix.

Jan Kiszka

author

2006-08-03 18:37 UTC #3 lore raw

Quoted 8 lines · Gilles Chanteperdrix

Quoted 6 lines · Jan Kiszka
A simple patch, just like suggested by Gilles, to avoid looping over periodic xntimer handlers in case of overruns.
It saves the current TSC on loop entry and uses this value later when forwarding the timer. Is is the overhead of re-reading the TSC on all archs negligible and should we rather go that way?

Quoted 9 lines

Review
Quoted 5 lines
(...)
(quoted diff) +2 -2
-				xntimerh_date(&timer->aplink) +=
-				    nkpod->htimer.interval;
+				while ((xntimerh_date(&timer->aplink) +=
+				        nkpod->htimer.interval) < now);
I think you are patching the wrong addition, the one you are interested in is most probably the one at the bottom of xntimer_do_tick_aperiodic.

Ouch, indeed. Guess I should start reading what I patch. Here comes a second try.

I'm still in favour of saving the TSC instead of re-reading it. Otherwise we would have to pave the code with #ifdefs for the case xnarch_get_cpu_tsc() is slow for a specific setup. Not that nice, is it?

Jan

---
 ksrc/nucleus/timer.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

ksrc/nucleus/timer.c +4 -3

Index: xenomai/ksrc/nucleus/timer.c
===================================================================
@@ -200,12 +200,13 @@ static void xntimer_do_tick_aperiodic(vo
 	xntimerq_t *timerq = &sched->timerqueue;
 	xntimerh_t *holder;
 	xntimer_t *timer;
+	xnticks_t now;
 
 	while ((holder = xntimerq_head(timerq)) != NULL) {
 		timer = aplink2timer(holder);
 
-		if (xntimerh_date(&timer->aplink) - nkschedlat >
-		    xnarch_get_cpu_tsc())
+		now = xnarch_get_cpu_tsc();
+		if (xntimerh_date(&timer->aplink) - nkschedlat > now)
 			/* No need to continue in aperiodic mode since timeout
 			   dates are ordered by increasing values. */
 			break;

@@ -247,7 +248,7 @@ static void xntimer_do_tick_aperiodic(vo
 			   translates into precious microsecs on low-end hw. */
 			__setbits(sched->status, XNHTICK);
 
-		xntimerh_date(&timer->aplink) += timer->interval;
+		while ((xntimerh_date(&timer->aplink) += timer->interval) < now);
 		xntimer_enqueue_aperiodic(timer);
 	}

Gilles Chanteperdrix

2006-08-16 15:43 UTC #4 lore raw

Quoted 19 lines · Jan Kiszka

Quoted 8 lines · Gilles Chanteperdrix
Quoted 6 lines · Jan Kiszka
A simple patch, just like suggested by Gilles, to avoid looping over periodic xntimer handlers in case of overruns.
It saves the current TSC on loop entry and uses this value later when forwarding the timer. Is is the overhead of re-reading the TSC on all archs negligible and should we rather go that way?
Quoted 9 lines
Review
Quoted 5 lines
(...)
(quoted diff) +2 -2
-				xntimerh_date(&timer->aplink) +=
-				    nkpod->htimer.interval;
+				while ((xntimerh_date(&timer->aplink) +=
+				        nkpod->htimer.interval) < now);
I think you are patching the wrong addition, the one you are interested in is most probably the one at the bottom of xntimer_do_tick_aperiodic.

Quoted 6 lines

Ouch, indeed. Guess I should start reading what I patch. Here comes a second try.
I'm still in favour of saving the TSC instead of re-reading it. Otherwise we would have to pave the code with #ifdefs for the case xnarch_get_cpu_tsc() is slow for a specific setup. Not that nice, is it?

I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?

Signature · 1 line

					    Gilles Chanteperdrix.

Jan Kiszka

author

2006-08-16 16:20 UTC #5 lore raw

Quoted 27 lines · Gilles Chanteperdrix

Quoted 19 lines · Jan Kiszka
Quoted 8 lines · Gilles Chanteperdrix
Quoted 6 lines · Jan Kiszka
A simple patch, just like suggested by Gilles, to avoid looping over periodic xntimer handlers in case of overruns.
It saves the current TSC on loop entry and uses this value later when forwarding the timer. Is is the overhead of re-reading the TSC on all archs negligible and should we rather go that way?
Quoted 9 lines
Review
Quoted 5 lines
(...)
(quoted diff) +2 -2
-				xntimerh_date(&timer->aplink) +=
-				    nkpod->htimer.interval;
+				while ((xntimerh_date(&timer->aplink) +=
+				        nkpod->htimer.interval) < now);
I think you are patching the wrong addition, the one you are interested in is most probably the one at the bottom of xntimer_do_tick_aperiodic.
Quoted 6 lines
Ouch, indeed. Guess I should start reading what I patch. Here comes a second try.
I'm still in favour of saving the TSC instead of re-reading it. Otherwise we would have to pave the code with #ifdefs for the case xnarch_get_cpu_tsc() is slow for a specific setup. Not that nice, is it?

Quoted 7 lines

I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?

Mmh, that gets close to hrtimer_forward now: push an overdue timer to an expiry date that is in the future and return the number of overruns. But do we still want this optimisation of the broken path then? It starts getting complex, probably adding more code than it is worth. I'm starting to vote against my own patch...

Jan

Gilles Chanteperdrix

2006-08-16 17:22 UTC #6 lore raw

Quoted 8 lines · Jan Kiszka

Quoted 7 lines
I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?

Quoted 5 lines

Mmh, that gets close to hrtimer_forward now: push an overdue timer to an expiry date that is in the future and return the number of overruns. But do we still want this optimisation of the broken path then? It starts getting complex, probably adding more code than it is worth. I'm starting to vote against my own patch...

The code to compute the number of overruns from the tsc exists in xnpod_wait_thread_period, is not this just a matter of refactoring this code in an xntimer_getoverrun ?

Signature · 1 line

					    Gilles Chanteperdrix.

Jan Kiszka

author

2006-08-17 07:55 UTC #7 lore raw

Quoted 15 lines · Gilles Chanteperdrix

Quoted 8 lines · Jan Kiszka
Quoted 7 lines
I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?
Quoted 5 lines
Mmh, that gets close to hrtimer_forward now: push an overdue timer to an expiry date that is in the future and return the number of overruns. But do we still want this optimisation of the broken path then? It starts getting complex, probably adding more code than it is worth. I'm starting to vote against my own patch...

Quoted 4 lines

The code to compute the number of overruns from the tsc exists in xnpod_wait_thread_period, is not this just a matter of refactoring this code in an xntimer_getoverrun ?

That overrun calculation takes place in thread context, we were discussing this for the timer handler. That means, if we generalise, we would have to pass the overrun counter somehow from the handler to the awakened thread. Would take another struct field, probably in xnthread. On the other hand, if that could save code in the posix skin, it may make sense.

Jan

Gilles Chanteperdrix

2006-08-17 13:03 UTC #8 lore raw

Quoted 21 lines · Jan Kiszka

Quoted 15 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
Quoted 7 lines
I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?
Quoted 5 lines
Mmh, that gets close to hrtimer_forward now: push an overdue timer to an expiry date that is in the future and return the number of overruns. But do we still want this optimisation of the broken path then? It starts getting complex, probably adding more code than it is worth. I'm starting to vote against my own patch...
Quoted 4 lines
The code to compute the number of overruns from the tsc exists in xnpod_wait_thread_period, is not this just a matter of refactoring this code in an xntimer_getoverrun ?

Quoted 6 lines

That overrun calculation takes place in thread context, we were discussing this for the timer handler. That means, if we generalise, we would have to pass the overrun counter somehow from the handler to the awakened thread. Would take another struct field, probably in xnthread. On the other hand, if that could save code in the posix skin, it may make sense.

What I had in mind was that timer handlers or threads that are interested in the overruns count could call xntimer_getoverruns, which computes the overruns count "on-the-fly" using the tsc, i.e. without requiring any additional field.

Signature · 1 line

					    Gilles Chanteperdrix.

Jan Kiszka

author

2006-08-17 13:35 UTC #9 lore raw

Quoted 29 lines · Gilles Chanteperdrix

Quoted 21 lines · Jan Kiszka
Quoted 15 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
Quoted 7 lines
I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?
Quoted 5 lines
Mmh, that gets close to hrtimer_forward now: push an overdue timer to an expiry date that is in the future and return the number of overruns. But do we still want this optimisation of the broken path then? It starts getting complex, probably adding more code than it is worth. I'm starting to vote against my own patch...
Quoted 4 lines
The code to compute the number of overruns from the tsc exists in xnpod_wait_thread_period, is not this just a matter of refactoring this code in an xntimer_getoverrun ?
Quoted 6 lines
That overrun calculation takes place in thread context, we were discussing this for the timer handler. That means, if we generalise, we would have to pass the overrun counter somehow from the handler to the awakened thread. Would take another struct field, probably in xnthread. On the other hand, if that could save code in the posix skin, it may make sense.

Quoted 5 lines

What I had in mind was that timer handlers or threads that are interested in the overruns count could call xntimer_getoverruns, which computes the overruns count "on-the-fly" using the tsc, i.e. without requiring any additional field.

Mmh, may work. I guess we need some code now. I already rebased my series over trunk, I could give this approach a try later.

Jan

Jan Kiszka

author

2006-08-18 06:09 UTC #10 lore raw

Quoted 30 lines · Jan Kiszka

Quoted 29 lines · Gilles Chanteperdrix
Quoted 27 lines · Jan Kiszka
Quoted 15 lines · Gilles Chanteperdrix
Quoted 8 lines · Jan Kiszka
Quoted 7 lines
I am thinking again about this patch: some handlers need to be rewritten, for example the posix timers handler, because the handler relies on the fact that it is called for every timer expiry to compute the overruns count. So maybe this patch should come with the addition of an xntimer_getoverrun service that computes the overrun count using the tsc ?
Quoted 5 lines
Mmh, that gets close to hrtimer_forward now: push an overdue timer to an expiry date that is in the future and return the number of overruns. But do we still want this optimisation of the broken path then? It starts getting complex, probably adding more code than it is worth. I'm starting to vote against my own patch...
Quoted 4 lines
The code to compute the number of overruns from the tsc exists in xnpod_wait_thread_period, is not this just a matter of refactoring this code in an xntimer_getoverrun ?
That overrun calculation takes place in thread context, we were discussing this for the timer handler. That means, if we generalise, we would have to pass the overrun counter somehow from the handler to the awakened thread. Would take another struct field, probably in xnthread. On the other hand, if that could save code in the posix skin, it may make sense.

Quoted 9 lines

Quoted 5 lines
What I had in mind was that timer handlers or threads that are interested in the overruns count could call xntimer_getoverruns, which computes the overruns count "on-the-fly" using the tsc, i.e. without requiring any additional field.
Mmh, may work. I guess we need some code now. I already rebased my series over trunk, I could give this approach a try later.

No, does not work smoothly: While pse51_base_timer_handler would be fine with xntimer_get_overruns, xnpod_wait_thread_period does its calculation based on the expiry saved in thread->pexpect. But xntimer_get_overruns would do this based on the timer object (as its name suggests), and that expiry will have moved on between the timer handler invocation and the thread activation. That means xntimer_get_overruns would only be useful in timer handler context.

Back to square #1 one: What can we gain here? My patch complicates at least one user, the posix skin, and that only for the sake of slightly improving a still broken system schedule...

Jan