[Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 321 bytes --]

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: xntimer-handle-periodic-overrun.patch --]
[-- Type: text/x-patch; name="xntimer-handle-periodic-overrun.patch", Size: 1209 bytes --]

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

Index: xenomai/ksrc/nucleus/timer.c
===================================================================
--- xenomai.orig/ksrc/nucleus/timer.c
+++ 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Philippe Gerum]

On Thu, 2006-08-03 at 09:01 +0200, Jan Kiszka wrote:
> 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.

> Jan
> _______________________________________________
> Xenomai-core mailing list
> Xenomai-core@domain.hid
> https://mail.gna.org/listinfo/xenomai-core
-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > 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?

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

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 1055 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > 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?
> 
>  >(...)
>  > -				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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: xntimer-handle-periodic-overrun-v2.patch --]
[-- Type: text/x-patch; name="xntimer-handle-periodic-overrun-v2.patch", Size: 1170 bytes --]

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

Index: xenomai/ksrc/nucleus/timer.c
===================================================================
--- xenomai.orig/ksrc/nucleus/timer.c
+++ 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);
 	}
 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > Gilles Chanteperdrix wrote:
 > > Jan Kiszka wrote:
 > >  > 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?
 > > 
 > >  >(...)
 > >  > -				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?

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 ?

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > Gilles Chanteperdrix wrote:
>  > > Jan Kiszka wrote:
>  > >  > 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?
>  > > 
>  > >  >(...)
>  > >  > -				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?
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > > 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...

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 ?

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1384 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > > 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...
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Gilles Chanteperdrix]

Jan Kiszka wrote:
 > Gilles Chanteperdrix wrote:
 > > Jan Kiszka wrote:
 > >  > > 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...
 > > 
 > > 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.

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.

-- 


					    Gilles Chanteperdrix.

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]

Gilles Chanteperdrix wrote:
> Jan Kiszka wrote:
>  > Gilles Chanteperdrix wrote:
>  > > Jan Kiszka wrote:
>  > >  > > 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...
>  > > 
>  > > 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.
> 
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH 4/4] shorten overrun loops of periodic timers

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

Jan Kiszka wrote:
> Gilles Chanteperdrix wrote:
>> Jan Kiszka wrote:
>>  > Gilles Chanteperdrix wrote:
>>  > > Jan Kiszka wrote:
>>  > >  > > 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...
>>  > > 
>>  > > 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.
>>
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]