[Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Dmitry Adamushko]

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

[ resent. subject was broken ]

------------------------------------


Hello,

[ pipe.c.patch ]  update the "timeout" variable so that the remaining
timeout is used in case of consequent xnsynch_sleep_on() calls.

As I understand that may indeed happen in case when another thread "steals"
data while this one waits to be scheduled in.

[ timer.c.patch] xnticks_t is unsigned while (as I understand)
"xntlholder_date(&timer->plink) - nkpod->jiffies" can be negative.
In this case, some positive big number is returned and any code that relies
on xnthread_timeout() or xntimer_get_timeout() and runs over periodic mode
won't work properly (e.g. xnsynch_sleep_on() and xnpipe_recv() now). Not
sure 1 should be returned in this case (so far I just did it the same way as
xntimer_get_timeout_aperiodic()), I guess 0 would be better in both cases.

At least in theory, 1 may cause an (even) infinite loop in xnpipe_recv() as
I don't like a check for "timeout > 1" to be placed there. It's something
that should be decided at the timer layer - I mean, whether it's too late to
sleep or not.

hope, I haven't overlooked anything this time :o)


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #1.2: Type: text/html, Size: 1407 bytes --]

[-- Attachment #2: pipe.c.patch --]
[-- Type: text/x-patch, Size: 310 bytes --]

--- pipe-SVN.c	2006-08-20 15:04:25.000000000 +0200
+++ pipe.c	2006-08-20 21:29:55.000000000 +0200
@@ -450,6 +450,9 @@ ssize_t xnpipe_recv(int minor, struct xn
 			ret = -EIDRM;
 			goto unlock_and_exit;
 		}
+
+		/* remaining timeout */		
+		timeout = xnthread_timeout(thread);
 	}
 
 	*pmh = link2mh(holder);

[-- Attachment #3: timer.c.patch --]
[-- Type: text/x-patch, Size: 534 bytes --]

--- timer-SVN.c	2006-08-20 15:50:18.000000000 +0200
+++ timer.c	2006-08-20 15:54:30.000000000 +0200
@@ -313,7 +313,12 @@ static xnticks_t xntimer_get_date_period
 
 static xnticks_t xntimer_get_timeout_periodic(xntimer_t *timer)
 {
-	return xntlholder_date(&timer->plink) - nkpod->jiffies;
+	xnticks_t now = nkpod->jiffies;
+	
+	if (xntlholder_date(&timer->plink) < now)
+		return 1;	/* Will elapse shortly. */
+	
+	return xntlholder_date(&timer->plink) - now;
 }
 
 static xnticks_t xntimer_get_raw_expiry_periodic(xntimer_t *timer)

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Philippe Gerum]

On Mon, 2006-08-21 at 10:36 +0200, Dmitry Adamushko wrote:

>  Not sure 1 should be returned in this case (so far I just did it the
> same way as xntimer_get_timeout_aperiodic()), I guess 0 would be
> better in both cases.

Zero (XN_INFINITE) is reserved to indicate a non-running timer.

-- 
Philippe.

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Philippe Gerum]

On Mon, 2006-08-21 at 10:36 +0200, Dmitry Adamushko wrote:

> [ timer.c.patch] xnticks_t is unsigned while (as I understand)
> "xntlholder_date(&timer->plink) - nkpod->jiffies" can be negative.

Well, normally, it should not. In periodic mode, each and every tick is
processed by the timer handler which also maintains the time base
(nkpod->jiffies), and as such, no timer should remain outstanding after
its wakeup time has elapsed, otherwise, something is broken in the timer
wheel management in the first place.

IOW, we should never miss a tick, and consequently, negative timeouts
cannot exist, since a timer is either running (= 0), or scheduled to
elapse in the future (> 0). On the contrary, aperiodic mode is based on
some free running TSC value not controlled by the nucleus, and as such,
we might well ask for the timeout value slightly before the related
timer elapses, and end up reading the TSC a bit later than its timeout
date.

-- 
Philippe.

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Dmitry Adamushko]

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

On 21/08/06, Philippe Gerum <rpm@xenomai.org> wrote:
>
> On Mon, 2006-08-21 at 10:36 +0200, Dmitry Adamushko wrote:
>
> > [ timer.c.patch] xnticks_t is unsigned while (as I understand)
> > "xntlholder_date(&timer->plink) - nkpod->jiffies" can be negative.


ok, I somehow missed a check for xntimer_running_p() in
xntimer_get_timeout(). I bet it was not there when I was looking at this
part of code yesterday or maybe I just had too much ummmm wonderful belgium
beer :o)

Actually xntimer_get_timeout() should be called with nklock held + irq off
as it must be atomic. But anyway, this is how it's used in all places and
it's probably not supposed to be used "as is" from any upper layer. So it's
ok.


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1167 bytes --]

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Philippe Gerum]

On Mon, 2006-08-21 at 19:45 +0200, Dmitry Adamushko wrote:
> 
> 
> On 21/08/06, Philippe Gerum <rpm@xenomai.org> wrote:
>         On Mon, 2006-08-21 at 10:36 +0200, Dmitry Adamushko wrote:
>         
>         > [ timer.c.patch] xnticks_t is unsigned while (as I
>         understand)
>         > "xntlholder_date(&timer->plink) - nkpod->jiffies" can be
>         negative.
> 
> ok, I somehow missed a check for xntimer_running_p() in
> xntimer_get_timeout(). I bet it was not there when I was looking at
> this part of code yesterday or maybe I just had too much ummmm
> wonderful belgium beer :o)

It's been there since the fusion times! Ok, it must be the beer... :o>

> 
> Actually xntimer_get_timeout() should be called with nklock held + irq
> off as it must be atomic. But anyway, this is how it's used in all
> places and it's probably not supposed to be used "as is" from any
> upper layer. So it's ok.
> 

Indeed.

> 
> 
> -- 
> Best regards,
> Dmitry Adamushko
-- 
Philippe.

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Philippe Gerum]

On Mon, 2006-08-21 at 10:36 +0200, Dmitry Adamushko wrote:

> 
> [ pipe.c.patch ]  update the "timeout" variable so that the remaining
> timeout is used in case of consequent xnsynch_sleep_on() calls.
> 
> As I understand that may indeed happen in case when another thread
> "steals" data while this one waits to be scheduled in.

Indeed, but this case is already handled in xnsynch_sleep_on()
(see the test for the XNROBBED bit).

-- 
Philippe.

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Dmitry Adamushko]

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

what about pipe-related change (I mean timeslice updating in xnpipe_recv())
?


-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 147 bytes --]

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Philippe Gerum]

On Mon, 2006-08-21 at 12:47 +0200, Dmitry Adamushko wrote:
> 
> what about pipe-related change (I mean timeslice updating in
> xnpipe_recv()) ?
> 

It's basically useless, since xnsynch_sleep_on() handles the resource
stealing case internally, and the loop in xnpipe_recv is fake actually.
Think of it as a goto statement in disguise.

> 
> -- 
> Best regards,
> Dmitry Adamushko
-- 
Philippe.

[Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Dmitry Adamushko]

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

On Mon, 2006-08-21 at 12:47 +0200, Dmitry Adamushko wrote:
> >
> > what about pipe-related change (I mean timeslice updating in
> > xnpipe_recv()) ?
> >
>
> It's basically useless, since xnsynch_sleep_on() handles the resource
> stealing case internally, and the loop in xnpipe_recv is fake actually.
> Think of it as a goto statement in disguise.


It has nothing to do with "resource stealing" (in terms of synch.c). The
synch object is not PIP at all.

task1 : blocked in xnpipe_recv()

task2 : xnpipe_send() ---> wakes up task1 ---> task1 is waiting to be
scheduled in

task3 [prio > task2.prio] : gets CPU and calls xnpipe_recv()

   task3 gets a message so state->inq is empty now.

...

task2 : calls getq(&state->inq) which is NULL now (if there were no more
messages)

calls xnsynch_sleep_on() again with the initial "timeout".

No?



>
> > --


> Best regards,
> Dmitry Adamushko
--
Philippe.





-- 
Best regards,
Dmitry Adamushko

[-- Attachment #2: Type: text/html, Size: 1654 bytes --]

Re: [Xenomai-core] [patch, minor] xnpipe_recv and xntimer_get_timeout_periodic()

[Philippe Gerum]

On Mon, 2006-08-21 at 13:47 +0200, Dmitry Adamushko wrote:
>                              From: 
> Dmitry Adamushko
> <dmitry.adamushko@domain.hid>
>                                To: 
> xenomai@xenomai.org
>                           Subject: 
> [Xenomai-core] [patch, minor]
> xnpipe_recv and
> xntimer_get_timeout_periodic()
>                              Date: 
> Mon, 21 Aug 2006 13:47:00 +0200
> 
> 
> 
> 
> 
>         On Mon, 2006-08-21 at 12:47 +0200, Dmitry Adamushko wrote: 
>         >
>         > what about pipe-related change (I mean timeslice updating
>         in 
>         > xnpipe_recv()) ?
>         >
>         
>         It's basically useless, since xnsynch_sleep_on() handles the
>         resource
>         stealing case internally, and the loop in xnpipe_recv is fake
>         actually.
>         Think of it as a goto statement in disguise.
> 
> It has nothing to do with "resource stealing" (in terms of synch.c).
> The synch object is not PIP at all.
> 

Ok, I thought we were discussing a more general issue about a new
potential side-effect of the resource stealing feature.

> task1 : blocked in xnpipe_recv()
> 
> task2 : xnpipe_send() ---> wakes up task1 ---> task1 is waiting to be
> scheduled in
> 
> task3 [prio > task2.prio] : gets CPU and calls xnpipe_recv()
> 
>    task3 gets a message so state->inq is empty now.
> 
> ...
> 
> task2 : calls getq(&state->inq) which is NULL now (if there were no
> more messages)
> 
> calls xnsynch_sleep_on() again with the initial "timeout".
>   

Definitely, yes. Please ignore my previous comment. I'll apply this one,
thanks.

-- 
Philippe.