All of lore.kernel.org
 help / color / mirror / Atom feed
* [Xenomai-help] -110 error on rt_task_send... bug?
@ 2006-08-08  1:50 Vincent Levesque
  2006-08-08 22:12 ` Jan Kiszka
  0 siblings, 1 reply; 16+ messages in thread
From: Vincent Levesque @ 2006-08-08  1:50 UTC (permalink / raw)
  To: xenomai; +Cc: andrewg

Hello,

I'm having some problems with rt_task_send/receive/reply in the native 
skin. The code attached at the end of this email works fine on Xenomai 
2.1 but rt_task_send() fails with error message -110 on Xenomai 2.2.0 
and today's subversion trunk. I checked the error values of all other 
calls (removed for clarity) and everything seems to be ok. The -110 
error code is returned either when rt_task_send() times out or when 
rt_task_reply() is called, whichever comes first. If I set it to 
TM_INFINITE, rt_task_send() never returns and the system hangs when I 
press control-c. I've reproduced this behavior on two computers.

Am I doing something wrong that was tolerated by 2.1? Is this a bug in 
Xenomai 2.2?

Thank you,

Vincent Levesque
vleves@domain.hid



gcc -o test test.c `/usr/xenomai/bin/xeno-config --xeno-cflags` -O2 
`/usr/xenomai/bin/xeno-config --xeno-ldflags` -lnative

--------------------------------------------------------
test.c
--------------------------------------------------------

#include <stdio.h>
#include <sys/mman.h>
#include <native/task.h>

void recv_task(void *arg)
{
    RT_TASK_MCB mcb_receive;
    int flowid;
    int data;

    mcb_receive.data = (caddr_t)&data;
    mcb_receive.size = sizeof(int);
    flowid = rt_task_receive(&mcb_receive,TM_INFINITE);
    if (mcb_receive.opcode == 1)
    {
        printf("receiving %d\n", data);
        rt_task_reply(flowid, NULL);
    }
}

int main (int argc, char *argv[])
{
    int data, rv;
    RT_TASK_MCB mcb_send;
    RT_TASK peer;
    RT_TASK task;

    mlockall(MCL_CURRENT | MCL_FUTURE);

    rt_task_spawn(&peer, "recv_task", 0, 99, T_FPU, &recv_task, NULL);
    rt_task_shadow(&task, "send_task", 50, 0);

    data = 13;
    mcb_send.opcode = 1;
    mcb_send.data = (caddr_t)&data;
    mcb_send.size = sizeof(int);

    printf("sending %d\n", data);
    rv = rt_task_send(&peer,&mcb_send,NULL,1E9);
    if (rv < 0)
    {
        switch(rv)
        {
            case -ENOBUFS:        printf("ENOBUFS\n");
            case -EWOULDBLOCK:    printf("EWOULDBLOCK\n");
            case -EIDRM:        printf("EIDRM\n");
            case -EINTR:        printf("EINTR\n");
            case -EPERM:        printf("EPERM\n");
            default:            printf("unknown error: %d\n", rv);
        }
    }

    rt_task_delete(&peer);
    return 0;
}



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-08  1:50 [Xenomai-help] -110 error on rt_task_send... bug? Vincent Levesque
@ 2006-08-08 22:12 ` Jan Kiszka
  2006-08-09  9:35   ` Dmitry Adamushko
  0 siblings, 1 reply; 16+ messages in thread
From: Jan Kiszka @ 2006-08-08 22:12 UTC (permalink / raw)
  To: Vincent Levesque; +Cc: xenomai, andrewg

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

Vincent Levesque wrote:
> Hello,
> 
> I'm having some problems with rt_task_send/receive/reply in the native
> skin. The code attached at the end of this email works fine on Xenomai
> 2.1 but rt_task_send() fails with error message -110 on Xenomai 2.2.0
> and today's subversion trunk. I checked the error values of all other
> calls (removed for clarity) and everything seems to be ok. The -110
> error code is returned either when rt_task_send() times out or when
> rt_task_reply() is called, whichever comes first. If I set it to
> TM_INFINITE, rt_task_send() never returns and the system hangs when I
> press control-c. I've reproduced this behavior on two computers.
> 
> Am I doing something wrong that was tolerated by 2.1? Is this a bug in
> Xenomai 2.2?

Nothing. This looks like a good catch of a regression in 2.2.

I'm not that deep into rt_task_send/receive design, but this problem
seems to be related to some heavy disagree between xnsynch_sleep_on and
rt_task_send about the correct ownership of the receiver's send queue.

Reading this comment [1] and then looking at that piece of code [2],
something must go utterly wrong wrt/ message passing in Xenomai 2.2. [1]
says the owner of task->msendq will always be "task" and never "sender"
(the current thread), so the test in [2] will always fail. The
lock-steeling code was introduced with 2.2, so this effect did not occur
with 2.1.

I have no idea yet how to solve it cleanly. Maybe Philippe finds some
time to comment on this before he'll be away for the next week. In the
meantime message passing users of the native skin may better stay on 2.1.x.

Jan


[1]http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/skins/native/task.c#L1750
[2]http://www.rts.uni-hannover.de/xenomai/lxr/source/ksrc/nucleus/synch.c#L212


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-08 22:12 ` Jan Kiszka
@ 2006-08-09  9:35   ` Dmitry Adamushko
  2006-08-09 11:42     ` [Xenomai-core] " Dmitry Adamushko
  2006-08-10 22:13     ` Philippe Gerum
  0 siblings, 2 replies; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-09  9:35 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: xenomai, andrewg

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

Hello,


I'm not that deep into rt_task_send/receive design, but this problem
> seems to be related to some heavy disagree between xnsynch_sleep_on and
> rt_task_send about the correct ownership of the receiver's send queue.


Not transfering the ownership causes this problem, yes. But looking briefly
at the code... there is even yet another problem (if I haven't overlooked
something) that also exists in 2.1.

rt_task_reply() wakes up a sender with :

[code]

if (receiver->wait_args.mps.mcb_s.flowid == flowid) {
                        /* Note that the following will cause the receiver
to be
                           unblocked without transferring the ownership of
the
                           msendq object, since we want the sender to keep
it. */
                        xnpod_resume_thread(&receiver->thread_base, XNPEND);
                        break;

[/code]

Now let's suppose : sender.prio > recv.prio

so recv.prio is boosted when sender executes xnsynch_sleep_on().

The "recv" task (the one that calls rt_task_receive() and rt_task_reply())
doesn't use xnsynch_* functions (like xnsynch_wakeup_this_sleeper() ) to
actually wake up the sender. Therefore xnsynch_clear_boost() is never called
and "recv" keeps temporarily inhereted priority as a permanent one from now
on.

No?

I have a few ideas on how to fix it intelegently so if Philippe will not
come up with a quick solution, I may take a look?


-- 
Best regards,
Dmitry Adamushko

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-09  9:35   ` Dmitry Adamushko
@ 2006-08-09 11:42     ` Dmitry Adamushko
  2006-08-09 15:09       ` Ulrich Schwab
  2006-08-10 22:13     ` Philippe Gerum
  1 sibling, 1 reply; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-09 11:42 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: xenomai, andrewg, xenomai


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

So could anyone test this patch and let me know if it works?

Note : I haven't compiled it even as I don't have a proper environment. But
the changes are pretty simple so it should be ok.

There was actually yet another problem mmm... who cares to delete a sender
from the msendq?

Now should be ok or maybe I'm completely wrong and see the things which do
not exist at all :)

p.s. I cc'ed "xenomai-core" as it may involve further discussions and it's
the right place indeed.

-- 
Best regards,
Dmitry Adamushko

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

[-- Attachment #2: synch-fowner.patch --]
[-- Type: text/x-patch, Size: 4022 bytes --]

diff -urp xenomai-a/include/nucleus/synch.h xenomai-b/include/nucleus/synch.h
--- xenomai-a/include/nucleus/synch.h	2006-07-20 11:09:01.000000000 +0200
+++ xenomai-b/include/nucleus/synch.h	2006-08-09 12:53:37.044217000 +0200
@@ -28,6 +28,7 @@
 #define XNSYNCH_NOPIP   0x0
 #define XNSYNCH_PIP     0x2
 #define XNSYNCH_DREORD  0x4
+#define XNSYNCH_FOWNER	0x20	/* Fixed owner */
 
 #if defined(__KERNEL__) || defined(__XENO_SIM__)
 
diff -urp xenomai-a/ksrc/nucleus/synch.c xenomai-b/ksrc/nucleus/synch.c
--- xenomai-a/ksrc/nucleus/synch.c	2006-06-15 14:15:25.000000000 +0200
+++ xenomai-b/ksrc/nucleus/synch.c	2006-08-09 13:28:55.199081000 +0200
@@ -37,6 +37,14 @@
 #include <nucleus/module.h>
 #include <nucleus/ltt.h>
 
+/* temporarily here */
+static inline void xnsynch_set_owner_internal(xnsynch_t *synch, xnthread_t *thread)
+{
+	if (!testbits(synch->status, XNSYNCH_FOWNER))
+		synch->owner = thread;
+}
+
+
 /*! 
  * \fn void xnsynch_init(xnsynch_t *synch, xnflags_t flags);
  * \brief Initialize a synchronization object.
@@ -181,7 +189,7 @@ void xnsynch_sleep_on(xnsynch_t *synch, 
 
 				if (testbits(synch->status, XNSYNCH_PENDING)) {
 					/* Ownership is still pending, steal the resource. */
-					synch->owner = thread;
+					xnsynch_set_owner_internal(synch, thread);
 					__clrbits(thread->status,
 						  XNRMID | XNTIMEO | XNBREAK);
 					goto grab_ownership;
@@ -209,7 +217,7 @@ void xnsynch_sleep_on(xnsynch_t *synch, 
 
 			xnpod_suspend_thread(thread, XNPEND, timeout, synch);
 
-			if (unlikely(synch->owner != thread)) {
+			if (!testbits(synch->status, XNSYNCH_OWNER) && unlikely(synch->owner != thread)) {
 				/* Somebody stole us the ownership while we were ready to
 				   run, waiting for the CPU: we need to wait again for the
 				   resource. */
@@ -362,12 +370,12 @@ xnthread_t *xnsynch_wakeup_one_sleeper(x
 	if (holder) {
 		thread = link2thread(holder, plink);
 		thread->wchan = NULL;
-		synch->owner = thread;
+		xnsynch_set_owner_internal(synch, thread);
 		__setbits(synch->status, XNSYNCH_PENDING);
 		xnltt_log_event(xeno_ev_wakeup1, thread->name, synch);
 		xnpod_resume_thread(thread, XNPEND);
 	} else {
-		synch->owner = NULL;
+		xnsynch_set_owner_internal(synch, thread);
 		__clrbits(synch->status, XNSYNCH_PENDING);
 	}
 
@@ -435,7 +443,7 @@ xnpholder_t *xnsynch_wakeup_this_sleeper
 	nholder = poppq(&synch->pendq, holder);
 	thread = link2thread(holder, plink);
 	thread->wchan = NULL;
-	synch->owner = thread;
+	xnsynch_set_owner_internal(synch, thread);
 	__setbits(synch->status, XNSYNCH_PENDING);
 	xnltt_log_event(xeno_ev_wakeupx, thread->name, synch);
 	xnpod_resume_thread(thread, XNPEND);
@@ -523,7 +531,7 @@ int xnsynch_flush(xnsynch_t *synch, xnfl
 		status = XNSYNCH_RESCHED;
 	}
 
-	synch->owner = NULL;
+	xnsynch_set_owner_internal(synch, NULL);
 	__clrbits(synch->status, XNSYNCH_PENDING);
 
 	xnlock_put_irqrestore(&nklock, s);
diff -urp xenomai-a/ksrc/skins/native/task.c xenomai-b/ksrc/skins/native/task.c
--- xenomai-a/ksrc/skins/native/task.c	2006-07-30 10:50:49.000000000 +0200
+++ xenomai-b/ksrc/skins/native/task.c	2006-08-09 13:32:21.917770000 +0200
@@ -262,7 +262,7 @@ int rt_task_create(RT_TASK *task,
 
 #ifdef CONFIG_XENO_OPT_NATIVE_MPS
 	xnsynch_init(&task->mrecv, XNSYNCH_FIFO);
-	xnsynch_init(&task->msendq, XNSYNCH_PRIO | XNSYNCH_PIP);
+	xnsynch_init(&task->msendq, XNSYNCH_PRIO | XNSYNCH_PIP | XNSYNCH_FOWNER);
 	xnsynch_set_owner(&task->msendq, &task->thread_base);
 	task->flowgen = 0;
 #endif /* CONFIG_XENO_OPT_NATIVE_MPS */
@@ -2057,10 +2057,7 @@ int rt_task_reply(int flowid, RT_TASK_MC
 		   identifier from other senders wrt to a given receiver. */
 
 		if (receiver->wait_args.mps.mcb_s.flowid == flowid) {
-			/* Note that the following will cause the receiver to be
-			   unblocked without transferring the ownership of the
-			   msendq object, since we want the sender to keep it. */
-			xnpod_resume_thread(&receiver->thread_base, XNPEND);
+			xnsynch_wakeup_this_sleeper(&sender->msendq, holder);
 			break;
 		}
 	}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-09 11:42     ` [Xenomai-core] " Dmitry Adamushko
@ 2006-08-09 15:09       ` Ulrich Schwab
  2006-08-09 15:42         ` Dmitry Adamushko
  2006-08-10  8:25         ` Jan Kiszka
  0 siblings, 2 replies; 16+ messages in thread
From: Ulrich Schwab @ 2006-08-09 15:09 UTC (permalink / raw)
  To: xenomai; +Cc: andrewg, Jan Kiszka

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

On Wednesday 09 August 2006 13:42, Dmitry Adamushko wrote:
> So could anyone test this patch and let me know if it works?
>
> Note : I haven't compiled it even as I don't have a proper environment. But
> the changes are pretty simple so it should be ok.
I just did run the test program supplied by Vincent after applying Your patch
(against 2.2.0)
It did work !
Only one small typo was in the patch, it is fixed in the attached patch 
version.

>
> There was actually yet another problem mmm... who cares to delete a sender
> from the msendq?
>
> Now should be ok or maybe I'm completely wrong and see the things which do
> not exist at all :)
>
> p.s. I cc'ed "xenomai-core" as it may involve further discussions and it's
> the right place indeed.

-- 
====================================================
  inmess GmbH
  Frankfurter Str. 74
  D - 64521 Gross-Gerau
  Phone: +49 6152 97790
  Fax  : +49 6152 977920
  mail : info@domain.hid
  web:   www.inmess.de
====================================================

[-- Attachment #2: synch-fowner.patch --]
[-- Type: text/x-diff, Size: 4023 bytes --]

diff -urp xenomai-a/include/nucleus/synch.h xenomai-b/include/nucleus/synch.h
--- xenomai-a/include/nucleus/synch.h	2006-07-20 11:09:01.000000000 +0200
+++ xenomai-b/include/nucleus/synch.h	2006-08-09 12:53:37.044217000 +0200
@@ -28,6 +28,7 @@
 #define XNSYNCH_NOPIP   0x0
 #define XNSYNCH_PIP     0x2
 #define XNSYNCH_DREORD  0x4
+#define XNSYNCH_FOWNER	0x20	/* Fixed owner */
 
 #if defined(__KERNEL__) || defined(__XENO_SIM__)
 
diff -urp xenomai-a/ksrc/nucleus/synch.c xenomai-b/ksrc/nucleus/synch.c
--- xenomai-a/ksrc/nucleus/synch.c	2006-06-15 14:15:25.000000000 +0200
+++ xenomai-b/ksrc/nucleus/synch.c	2006-08-09 13:28:55.199081000 +0200
@@ -37,6 +37,14 @@
 #include <nucleus/module.h>
 #include <nucleus/ltt.h>
 
+/* temporarily here */
+static inline void xnsynch_set_owner_internal(xnsynch_t *synch, xnthread_t *thread)
+{
+	if (!testbits(synch->status, XNSYNCH_FOWNER))
+		synch->owner = thread;
+}
+
+
 /*! 
  * \fn void xnsynch_init(xnsynch_t *synch, xnflags_t flags);
  * \brief Initialize a synchronization object.
@@ -181,7 +189,7 @@ void xnsynch_sleep_on(xnsynch_t *synch, 
 
 				if (testbits(synch->status, XNSYNCH_PENDING)) {
 					/* Ownership is still pending, steal the resource. */
-					synch->owner = thread;
+					xnsynch_set_owner_internal(synch, thread);
 					__clrbits(thread->status,
 						  XNRMID | XNTIMEO | XNBREAK);
 					goto grab_ownership;
@@ -209,7 +217,7 @@ void xnsynch_sleep_on(xnsynch_t *synch, 
 
 			xnpod_suspend_thread(thread, XNPEND, timeout, synch);
 
-			if (unlikely(synch->owner != thread)) {
+			if (!testbits(synch->status, XNSYNCH_FOWNER) && unlikely(synch->owner != thread)) {
 				/* Somebody stole us the ownership while we were ready to
 				   run, waiting for the CPU: we need to wait again for the
 				   resource. */
@@ -362,12 +370,12 @@ xnthread_t *xnsynch_wakeup_one_sleeper(x
 	if (holder) {
 		thread = link2thread(holder, plink);
 		thread->wchan = NULL;
-		synch->owner = thread;
+		xnsynch_set_owner_internal(synch, thread);
 		__setbits(synch->status, XNSYNCH_PENDING);
 		xnltt_log_event(xeno_ev_wakeup1, thread->name, synch);
 		xnpod_resume_thread(thread, XNPEND);
 	} else {
-		synch->owner = NULL;
+		xnsynch_set_owner_internal(synch, thread);
 		__clrbits(synch->status, XNSYNCH_PENDING);
 	}
 
@@ -435,7 +443,7 @@ xnpholder_t *xnsynch_wakeup_this_sleeper
 	nholder = poppq(&synch->pendq, holder);
 	thread = link2thread(holder, plink);
 	thread->wchan = NULL;
-	synch->owner = thread;
+	xnsynch_set_owner_internal(synch, thread);
 	__setbits(synch->status, XNSYNCH_PENDING);
 	xnltt_log_event(xeno_ev_wakeupx, thread->name, synch);
 	xnpod_resume_thread(thread, XNPEND);
@@ -523,7 +531,7 @@ int xnsynch_flush(xnsynch_t *synch, xnfl
 		status = XNSYNCH_RESCHED;
 	}
 
-	synch->owner = NULL;
+	xnsynch_set_owner_internal(synch, NULL);
 	__clrbits(synch->status, XNSYNCH_PENDING);
 
 	xnlock_put_irqrestore(&nklock, s);
diff -urp xenomai-a/ksrc/skins/native/task.c xenomai-b/ksrc/skins/native/task.c
--- xenomai-a/ksrc/skins/native/task.c	2006-07-30 10:50:49.000000000 +0200
+++ xenomai-b/ksrc/skins/native/task.c	2006-08-09 13:32:21.917770000 +0200
@@ -262,7 +262,7 @@ int rt_task_create(RT_TASK *task,
 
 #ifdef CONFIG_XENO_OPT_NATIVE_MPS
 	xnsynch_init(&task->mrecv, XNSYNCH_FIFO);
-	xnsynch_init(&task->msendq, XNSYNCH_PRIO | XNSYNCH_PIP);
+	xnsynch_init(&task->msendq, XNSYNCH_PRIO | XNSYNCH_PIP | XNSYNCH_FOWNER);
 	xnsynch_set_owner(&task->msendq, &task->thread_base);
 	task->flowgen = 0;
 #endif /* CONFIG_XENO_OPT_NATIVE_MPS */
@@ -2057,10 +2057,7 @@ int rt_task_reply(int flowid, RT_TASK_MC
 		   identifier from other senders wrt to a given receiver. */
 
 		if (receiver->wait_args.mps.mcb_s.flowid == flowid) {
-			/* Note that the following will cause the receiver to be
-			   unblocked without transferring the ownership of the
-			   msendq object, since we want the sender to keep it. */
-			xnpod_resume_thread(&receiver->thread_base, XNPEND);
+			xnsynch_wakeup_this_sleeper(&sender->msendq, holder);
 			break;
 		}
 	}

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-09 15:09       ` Ulrich Schwab
@ 2006-08-09 15:42         ` Dmitry Adamushko
  2006-08-10 18:14           ` Vincent Levesque
  2006-08-10  8:25         ` Jan Kiszka
  1 sibling, 1 reply; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-09 15:42 UTC (permalink / raw)
  To: Ulrich Schwab; +Cc: xenomai, andrewg, Jan Kiszka, xenomai

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

> > So could anyone test this patch and let me know if it works?
> >
> > Note : I haven't compiled it even as I don't have a proper environment.
> But
> > the changes are pretty simple so it should be ok.
> I just did run the test program supplied by Vincent after applying Your
> patch
> (against 2.2.0)
> It did work !
>


That's good. Thanks for your assistance.

So synch. message passing interface (IOW, rt_task_send/receive/reply) is
mgmglmgmm... slightly broken at the moment for all versions.

The ones who need it may use this patch so far. I'll think a bit more on it
in the mean time but I tend to think that "Fixed Ownership" (expressed by
XNSYNCH_FOWNER) should really be a property of the synch. object indeed (as
in the current patch).



> Only one small typo was in the patch, it is fixed in the attached patch
> version.

Ok, now I need to run diff -u old-patch new-patch to find out this typo :)


-- 
Best regards,
Dmitry Adamushko

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-09 15:09       ` Ulrich Schwab
  2006-08-09 15:42         ` Dmitry Adamushko
@ 2006-08-10  8:25         ` Jan Kiszka
  2006-08-10 10:11           ` Dmitry Adamushko
  1 sibling, 1 reply; 16+ messages in thread
From: Jan Kiszka @ 2006-08-10  8:25 UTC (permalink / raw)
  To: Dmitry Adamushko; +Cc: andrewg, xenomai

Hi Dmitry,

some comments on the patch, but I still haven't run it. The basic idea
is sane, but we have to wait for Philippe's comments of course.

Ulrich Schwab wrote:
> On Wednesday 09 August 2006 13:42, Dmitry Adamushko wrote:
>> So could anyone test this patch and let me know if it works?
>>
>> Note : I haven't compiled it even as I don't have a proper environment. But
>> the changes are pretty simple so it should be ok.
> I just did run the test program supplied by Vincent after applying Your patch
> (against 2.2.0)
> It did work !
> Only one small typo was in the patch, it is fixed in the attached patch 
> version.
> 
>> There was actually yet another problem mmm... who cares to delete a sender
>> from the msendq?
>>
>> Now should be ok or maybe I'm completely wrong and see the things which do
>> not exist at all :)
>>
>> p.s. I cc'ed "xenomai-core" as it may involve further discussions and it's
>> the right place indeed.
> 
> 
> ------------------------------------------------------------------------
> 
> diff -urp xenomai-a/include/nucleus/synch.h xenomai-b/include/nucleus/synch.h
> --- xenomai-a/include/nucleus/synch.h	2006-07-20 11:09:01.000000000 +0200
> +++ xenomai-b/include/nucleus/synch.h	2006-08-09 12:53:37.044217000 +0200
> @@ -28,6 +28,7 @@
>  #define XNSYNCH_NOPIP   0x0
>  #define XNSYNCH_PIP     0x2
>  #define XNSYNCH_DREORD  0x4
> +#define XNSYNCH_FOWNER	0x20	/* Fixed owner */
>  
>  #if defined(__KERNEL__) || defined(__XENO_SIM__)
>  
> diff -urp xenomai-a/ksrc/nucleus/synch.c xenomai-b/ksrc/nucleus/synch.c
> --- xenomai-a/ksrc/nucleus/synch.c	2006-06-15 14:15:25.000000000 +0200
> +++ xenomai-b/ksrc/nucleus/synch.c	2006-08-09 13:28:55.199081000 +0200
> @@ -37,6 +37,14 @@
>  #include <nucleus/module.h>
>  #include <nucleus/ltt.h>
>  
> +/* temporarily here */
> +static inline void xnsynch_set_owner_internal(xnsynch_t *synch, xnthread_t *thread)
> +{
> +	if (!testbits(synch->status, XNSYNCH_FOWNER))
> +		synch->owner = thread;
> +}
> +
> +

I don't think we should use this inline function after the changes I'm
suggesting below. There will be too many exceptions from this pattern,
and the code is more readable with this unrolled.

>  /*! 
>   * \fn void xnsynch_init(xnsynch_t *synch, xnflags_t flags);
>   * \brief Initialize a synchronization object.
> @@ -181,7 +189,7 @@ void xnsynch_sleep_on(xnsynch_t *synch, 
>  
>  				if (testbits(synch->status, XNSYNCH_PENDING)) {
>  					/* Ownership is still pending, steal the resource. */
> -					synch->owner = thread;
> +					xnsynch_set_owner_internal(synch, thread);
>  					__clrbits(thread->status,
>  						  XNRMID | XNTIMEO | XNBREAK);
>  					goto grab_ownership;

Mmm, given the fixed ownership, should we ever enter this code path
under XNSYNCH_FOWNER? Is there a scenario with XNSYNCH_FOWNER |
XNSYNCH_PENDING? If yes, I would say: fix that one. Then the change
above is no longer required.

> @@ -209,7 +217,7 @@ void xnsynch_sleep_on(xnsynch_t *synch, 
>  
>  			xnpod_suspend_thread(thread, XNPEND, timeout, synch);
>  
> -			if (unlikely(synch->owner != thread)) {
> +			if (!testbits(synch->status, XNSYNCH_FOWNER) && unlikely(synch->owner != thread)) {
>  				/* Somebody stole us the ownership while we were ready to
>  				   run, waiting for the CPU: we need to wait again for the
>  				   resource. */

(synch->owner != thread) is more likely to be unlikely ;). I would
reorder it, i.e. put the owner check to the front again.

> @@ -362,12 +370,12 @@ xnthread_t *xnsynch_wakeup_one_sleeper(x
>  	if (holder) {
>  		thread = link2thread(holder, plink);
>  		thread->wchan = NULL;
> -		synch->owner = thread;
> +		xnsynch_set_owner_internal(synch, thread);
>  		__setbits(synch->status, XNSYNCH_PENDING);

Here we are: I think we shouldn't set XNSYNCH_PENDING if XNSYNCH_FOWNER
is set.

>  		xnltt_log_event(xeno_ev_wakeup1, thread->name, synch);
>  		xnpod_resume_thread(thread, XNPEND);
>  	} else {
> -		synch->owner = NULL;
> +		xnsynch_set_owner_internal(synch, thread);
>  		__clrbits(synch->status, XNSYNCH_PENDING);
>  	}
>  
> @@ -435,7 +443,7 @@ xnpholder_t *xnsynch_wakeup_this_sleeper
>  	nholder = poppq(&synch->pendq, holder);
>  	thread = link2thread(holder, plink);
>  	thread->wchan = NULL;
> -	synch->owner = thread;
> +	xnsynch_set_owner_internal(synch, thread);
>  	__setbits(synch->status, XNSYNCH_PENDING);

...and here again.

>  	xnltt_log_event(xeno_ev_wakeupx, thread->name, synch);
>  	xnpod_resume_thread(thread, XNPEND);
> @@ -523,7 +531,7 @@ int xnsynch_flush(xnsynch_t *synch, xnfl
>  		status = XNSYNCH_RESCHED;
>  	}
>  
> -	synch->owner = NULL;
> +	xnsynch_set_owner_internal(synch, NULL);
>  	__clrbits(synch->status, XNSYNCH_PENDING);
>  
>  	xnlock_put_irqrestore(&nklock, s);
> diff -urp xenomai-a/ksrc/skins/native/task.c xenomai-b/ksrc/skins/native/task.c
> --- xenomai-a/ksrc/skins/native/task.c	2006-07-30 10:50:49.000000000 +0200
> +++ xenomai-b/ksrc/skins/native/task.c	2006-08-09 13:32:21.917770000 +0200
> @@ -262,7 +262,7 @@ int rt_task_create(RT_TASK *task,
>  
>  #ifdef CONFIG_XENO_OPT_NATIVE_MPS
>  	xnsynch_init(&task->mrecv, XNSYNCH_FIFO);
> -	xnsynch_init(&task->msendq, XNSYNCH_PRIO | XNSYNCH_PIP);
> +	xnsynch_init(&task->msendq, XNSYNCH_PRIO | XNSYNCH_PIP | XNSYNCH_FOWNER);
>  	xnsynch_set_owner(&task->msendq, &task->thread_base);
>  	task->flowgen = 0;
>  #endif /* CONFIG_XENO_OPT_NATIVE_MPS */
> @@ -2057,10 +2057,7 @@ int rt_task_reply(int flowid, RT_TASK_MC
>  		   identifier from other senders wrt to a given receiver. */
>  
>  		if (receiver->wait_args.mps.mcb_s.flowid == flowid) {
> -			/* Note that the following will cause the receiver to be
> -			   unblocked without transferring the ownership of the
> -			   msendq object, since we want the sender to keep it. */
> -			xnpod_resume_thread(&receiver->thread_base, XNPEND);
> +			xnsynch_wakeup_this_sleeper(&sender->msendq, holder);

Yeah, this somehow looks cleaner to me.

>  			break;
>  		}
>  	}
> 

Jan


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-10  8:25         ` Jan Kiszka
@ 2006-08-10 10:11           ` Dmitry Adamushko
  0 siblings, 0 replies; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-10 10:11 UTC (permalink / raw)
  To: Jan Kiszka; +Cc: andrewg, xenomai

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

Hello,

if you take a look e.g. at how synch->owner is used in the current SVN
codebase then you will notice that this field is actually usefull only for
PIP objects but xnsynch_wakeup_* and friends update it for all cases. No, I
don't want to say that's why let's do the same with PENDING.

This is a preliminary patch and, that said, I'm going to take a closer look
at the synch.c.

Regarding PENDING : not sure at the moment if it would be helpful (have to
think about possible use cases), but with the help of PENDING it's possible
to support "ownership"-stealing for FOWNER objects. Sounds funny? :)

if (testbits(synch->status, XNSYNCH_FOWNER | XNSYNCH_PENDING) ==
XNSYNCH_FOWNER) {

// the object has been stolen while a thread was waiting to be scheduled on

}


-- 
Best regards,
Dmitry Adamushko

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-09 15:42         ` Dmitry Adamushko
@ 2006-08-10 18:14           ` Vincent Levesque
  2006-08-10 18:18             ` Dmitry Adamushko
  0 siblings, 1 reply; 16+ messages in thread
From: Vincent Levesque @ 2006-08-10 18:14 UTC (permalink / raw)
  To: xenomai; +Cc: andrewg

The patch works here too. Thanks for looking into this so quickly. I'll 
switch back to 2.1 until this is resolved.

Vincent Levesque
vleves@domain.hid

Dmitry Adamushko wrote:
>
>     > So could anyone test this patch and let me know if it works?
>     >
>     > Note : I haven't compiled it even as I don't have a proper
>     environment. But
>     > the changes are pretty simple so it should be ok.
>     I just did run the test program supplied by Vincent after applying
>     Your patch
>     (against 2.2.0)
>     It did work !
>
>
>
> That's good. Thanks for your assistance.
>
> So synch. message passing interface (IOW, rt_task_send/receive/reply) 
> is mgmglmgmm... slightly broken at the moment for all versions.
>
> The ones who need it may use this patch so far. I'll think a bit more 
> on it in the mean time but I tend to think that "Fixed Ownership" 
> (expressed by XNSYNCH_FOWNER) should really be a property of the 
> synch. object indeed (as in the current patch).
>
>
>
> > Only one small typo was in the patch, it is fixed in the attached patch
> > version.
>
> Ok, now I need to run diff -u old-patch new-patch to find out this typo :)
>
>  
> -- 
> Best regards,
> Dmitry Adamushko
>




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-core] Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-10 18:14           ` Vincent Levesque
@ 2006-08-10 18:18             ` Dmitry Adamushko
  0 siblings, 0 replies; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-10 18:18 UTC (permalink / raw)
  To: Vincent Levesque; +Cc: xenomai, andrewg, xenomai

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

The patch works here too. Thanks for looking into this so quickly. I'll
> switch back to 2.1 until this is resolved.
>

It's also broken in 2.1. There are 2 problems there not releted to recently
introduced ownership-stealing part. That said, you may safely (well, you
remember this "WITHOUT ANY WARRANTY..." but indeed :) apply this patch so
far if you really need synch. message passing mechanism.


-- 
Best regards,
Dmitry Adamushko

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-09  9:35   ` Dmitry Adamushko
  2006-08-09 11:42     ` [Xenomai-core] " Dmitry Adamushko
@ 2006-08-10 22:13     ` Philippe Gerum
  2006-08-11  7:50       ` Dmitry Adamushko
  2006-08-12 10:33       ` Dmitry Adamushko
  1 sibling, 2 replies; 16+ messages in thread
From: Philippe Gerum @ 2006-08-10 22:13 UTC (permalink / raw)
  To: Dmitry Adamushko; +Cc: xenomai, andrewg, Jan Kiszka

On Wed, 2006-08-09 at 11:35 +0200, Dmitry Adamushko wrote: 
> 
> Hello,
>  
> 
>         I'm not that deep into rt_task_send/receive design, but this
>         problem
>         seems to be related to some heavy disagree between
>         xnsynch_sleep_on and 
>         rt_task_send about the correct ownership of the receiver's
>         send queue. 
> 
> 
> Not transfering the ownership causes this problem, yes. But looking
> briefly at the code... there is even yet another problem (if I haven't
> overlooked something) that also exists in 2.1.
> 
> rt_task_reply() wakes up a sender with : 
> 
> [code]
> 
> if (receiver->wait_args.mps.mcb_s.flowid == flowid) {
>                         /* Note that the following will cause the
> receiver to be
>                            unblocked without transferring the
> ownership of the
>                            msendq object, since we want the sender to
> keep it. */
>                         xnpod_resume_thread(&receiver->thread_base,
> XNPEND);
>                         break;
> 
> [/code]
> 
> 
> Now let's suppose : sender.prio > recv.prio
> 
> so recv.prio is boosted when sender executes xnsynch_sleep_on().
> 
> The "recv" task (the one that calls rt_task_receive() and
> rt_task_reply()) doesn't use xnsynch_* functions (like
> xnsynch_wakeup_this_sleeper() ) to actually wake up the sender.
> Therefore xnsynch_clear_boost() is never called and "recv" keeps
> temporarily inhereted priority as a permanent one from now on.
> No? 
> 

No. xnpod_resume_thread() detects that the sender is pending on the
msendq, and eventually calls xnsynch_forget_sleeper(), which removes the
sender from the pend queue, then clears the boost.

The problem is triggered by synch->owner being cleared in
xnsynch_flush(), as a result of the receiver task to exit (called from
the native task deletion hook, i.e. xnsynch_destroy(&task->msendq)).
The problem to fix is indeed in xnsynch_sleep_on(), but it's rather a
matter of making sure that the awakened thread is not going to check the
owner field of a synch object that has just been destroyed.

> I have a few ideas on how to fix it intelegently so if Philippe will
> not come up with a quick solution, I may take a look?
> 
> 
> -- 
> Best regards,
> Dmitry Adamushko
> 
> _______________________________________________
> Xenomai-help mailing list
> Xenomai-help@domain.hid
> https://mail.gna.org/listinfo/xenomai-help
-- 
Philippe.




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-10 22:13     ` Philippe Gerum
@ 2006-08-11  7:50       ` Dmitry Adamushko
  2006-08-12 10:33       ` Dmitry Adamushko
  1 sibling, 0 replies; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-11  7:50 UTC (permalink / raw)
  To: rpm; +Cc: xenomai, andrewg, Jan Kiszka

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

No. xnpod_resume_thread() detects that the sender is pending on the
> msendq, and eventually calls xnsynch_forget_sleeper(), which removes the
> sender from the pend queue, then clears the boost.


I overlooked it indeed.


The problem is triggered by synch->owner being cleared in
> xnsynch_flush(), as a result of the receiver task to exit (called from
> the native task deletion hook, i.e. xnsynch_destroy(&task->msendq)).
> The problem to fix is indeed in xnsynch_sleep_on(), but it's rather a
> matter of making sure that the awakened thread is not going to check the
> owner field of a synch object that has just been destroyed.
>
>
and moreover I was completely wrong. So the reputation of 2.1 is restored
and that's more important indeed than err... mine own :o)


-- 
Best regards,
Dmitry Adamushko

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-10 22:13     ` Philippe Gerum
  2006-08-11  7:50       ` Dmitry Adamushko
@ 2006-08-12 10:33       ` Dmitry Adamushko
  2006-08-12 21:14         ` Philippe Gerum
  1 sibling, 1 reply; 16+ messages in thread
From: Dmitry Adamushko @ 2006-08-12 10:33 UTC (permalink / raw)
  To: rpm; +Cc: xenomai, Jan Kiszka

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

>
>
> The problem is triggered by synch->owner being cleared in
> xnsynch_flush(), as a result of the receiver task to exit (called from
> the native task deletion hook, i.e. xnsynch_destroy(&task->msendq)).



As I understood it didn't work in all cases and not only when a receiver
exits. What's wrong with the case Jan revealed?

I mean rt_task_reply() makes use of a direct call to xnpod_resume_thread()
to avoid a change of the owner while a sender finds itself being woken up in
xnsynch_sleep_on() - the PRIO + PIP branch - and the first thing it does is
a check for (synch->owner != thread) and it's obviously that the sender is
not the owner (but the receiver).

No?

> --
>
> Philippe.

-- 
Best regards,
Dmitry Adamushko

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-12 10:33       ` Dmitry Adamushko
@ 2006-08-12 21:14         ` Philippe Gerum
  2006-08-13 14:47           ` Philippe Gerum
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Gerum @ 2006-08-12 21:14 UTC (permalink / raw)
  To: Dmitry Adamushko; +Cc: xenomai, Jan Kiszka

On Sat, 2006-08-12 at 12:33 +0200, Dmitry Adamushko wrote:
>         
>         The problem is triggered by synch->owner being cleared in
>         xnsynch_flush(), as a result of the receiver task to exit
>         (called from 
>         the native task deletion hook, i.e.
>         xnsynch_destroy(&task->msendq)). 
>  
>  
> As I understood it didn't work in all cases and not only when a
> receiver exits. What's wrong with the case Jan revealed?
>  
> I mean rt_task_reply() makes use of a direct call to
> xnpod_resume_thread() to avoid a change of the owner while a sender
> finds itself being woken up in xnsynch_sleep_on() - the PRIO + PIP
> branch - and the first thing it does is a check for (synch->owner !=
> thread) and it's obviously that the sender is not the owner (but the
> receiver).
> 
> No?

Yes, but there is even more than this, which will require some fixing.

There is a design issue regarding how we deal with the lifetime of the
RT_TASK::msendq member: if the embodying task (i.e. the server task)
exits after the client task has been replied to, but before such task
has had a chance to return from xnsynch_sleep_on(), then the latter
routine would use a _destroyed_ synch. object inside the return path.

Due to the object stealing feature, xnsynch_sleep_on() now relies on the
assumption that the pended synch object still exists upon return from
xnpod_suspend_thread(), and this is really, really bad. The only thing
that may be accessed safely on this return path is the resuming thread's
control block itself (obviously), and _not_ what it was pending on. It's
the caller's business to find out whether the awaited resource is still
usable, but the nucleus should not rely on this assumption, given that a
rescheduling is likely to have taken place in xnsynch_sleep_on(), and
lots of things might have happended since then which are way outside of
its knowledge.

IOW, a construct like "if (synch-> ...." when returning from suspension
is the root of all evil. We must not depend on anything requiring such
dereference when resuming the thread. A way to fix this would be to
introduce a new thread status flag which gets raised whenever some
object ownership is stolen, so that we would not have to inspect the
synch object anymore to know about such situation, and rely on
potentially wrong information from the ->owner field. We would only have
to inspect the status flags of the thread that got "robbed" (i.e. the
one that resumes) and act accordingly. I will post a patch implementing
this, after I have solved all the related issues.

PS: note that in the particular case of the message passing services,
object stealing cannot happen, since we never call synch wakeup services
that transfer ownership but rather resume the thread bluntly, so the
PENDING bit never gets raised, thus preventing any stealing.

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




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-12 21:14         ` Philippe Gerum
@ 2006-08-13 14:47           ` Philippe Gerum
  2006-08-13 20:24             ` Philippe Gerum
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Gerum @ 2006-08-13 14:47 UTC (permalink / raw)
  To: Dmitry Adamushko; +Cc: xenomai, Jan Kiszka

On Sat, 2006-08-12 at 23:14 +0200, Philippe Gerum wrote:

> IOW, a construct like "if (synch-> ...." when returning from suspension
> is the root of all evil. We must not depend on anything requiring such
> dereference when resuming the thread. A way to fix this would be to
> introduce a new thread status flag which gets raised whenever some
> object ownership is stolen, so that we would not have to inspect the
> synch object anymore to know about such situation, and rely on
> potentially wrong information from the ->owner field. We would only have
> to inspect the status flags of the thread that got "robbed" (i.e. the
> one that resumes) and act accordingly. I will post a patch implementing
> this, after I have solved all the related issues.

The patch below provides a more general fix to the issues introduced by
the resource stealing feature. There is still one case where this code
could attempt to reuse a stolen resource unsafely, but the calling code
would have to be quite fragile in the first place. Normal use cases
should be ok.

Already checked on the simulator, but please give this a try.

Index: ksrc/nucleus/pod.c
===================================================================
--- ksrc/nucleus/pod.c	(revision 1420)
+++ ksrc/nucleus/pod.c	(working copy)
@@ -1419,7 +1419,7 @@
 			__clrbits(thread->status, XNREADY);
 		}
 
-		__clrbits(thread->status, XNRMID | XNTIMEO | XNBREAK);
+		__clrbits(thread->status, XNRMID | XNTIMEO | XNBREAK | XNWAKEN | XNROBBED);
 	}
 
 	__setbits(thread->status, mask);
Index: ksrc/nucleus/synch.c
===================================================================
--- ksrc/nucleus/synch.c	(revision 1420)
+++ ksrc/nucleus/synch.c	(working copy)
@@ -92,7 +92,7 @@
 	if (flags & XNSYNCH_PIP)
 		flags |= XNSYNCH_PRIO;	/* Obviously... */
 
-	synch->status = flags & ~(XNSYNCH_CLAIMED | XNSYNCH_PENDING);
+	synch->status = flags & ~XNSYNCH_CLAIMED;
 	synch->owner = NULL;
 	synch->cleanup = NULL;	/* Only works for PIP-enabled objects. */
 	initpq(&synch->pendq, xnpod_get_qdir(nkpod),
@@ -169,79 +169,74 @@
 
 	xnltt_log_event(xeno_ev_sleepon, thread->name, synch);
 
-	if (testbits(synch->status, XNSYNCH_PRIO)) {
+	if (!testbits(synch->status, XNSYNCH_PRIO)) { /* i.e. FIFO */
+		appendpq(&synch->pendq, &thread->plink);
+		xnpod_suspend_thread(thread, XNPEND, timeout, synch);
+		goto unlock_and_exit;
+	}
 
-		if (testbits(synch->status, XNSYNCH_PIP)) {
-		      redo:
-			owner = synch->owner;
+	if (!testbits(synch->status, XNSYNCH_PIP)) { /* i.e. no ownership */
+		insertpqf(&synch->pendq, &thread->plink, thread->cprio);
+		xnpod_suspend_thread(thread, XNPEND, timeout, synch);
+		goto unlock_and_exit;
+	}
 
-			if (owner
-			    && xnpod_compare_prio(thread->cprio,
-						  owner->cprio) > 0) {
+redo:
+	owner = synch->owner;
 
-				if (testbits(synch->status, XNSYNCH_PENDING)) {
-					/* Ownership is still pending, steal the resource. */
-					synch->owner = thread;
-					__clrbits(thread->status,
-						  XNRMID | XNTIMEO | XNBREAK);
-					goto grab_ownership;
-				}
+	if (!owner) {
+		synch->owner = thread;
+		__clrbits(thread->status, XNRMID | XNTIMEO | XNBREAK);
+		goto unlock_and_exit;
+	}
 
-				if (!testbits(owner->status, XNBOOST)) {
-					owner->bprio = owner->cprio;
-					__setbits(owner->status, XNBOOST);
-				}
+	if (xnpod_compare_prio(thread->cprio, owner->cprio) > 0) {
 
-				if (testbits(synch->status, XNSYNCH_CLAIMED))
-					removepq(&owner->claimq, &synch->link);
-				else
-					__setbits(synch->status,
-						  XNSYNCH_CLAIMED);
+		if (testbits(owner->status, XNWAKEN)) {
+			/* Ownership is still pending, steal the resource. */
+			synch->owner = thread;
+			__clrbits(thread->status, XNRMID | XNTIMEO | XNBREAK);
+			__setbits(owner->status, XNROBBED);
+			goto unlock_and_exit;
+		}
 
-				insertpqf(&synch->pendq, &thread->plink,
-					  thread->cprio);
-				insertpqf(&owner->claimq, &synch->link,
-					  thread->cprio);
-				xnsynch_renice_thread(owner, thread->cprio);
-			} else
-				insertpqf(&synch->pendq, &thread->plink,
-					  thread->cprio);
+		if (!testbits(owner->status, XNBOOST)) {
+			owner->bprio = owner->cprio;
+			__setbits(owner->status, XNBOOST);
+		}
 
-			xnpod_suspend_thread(thread, XNPEND, timeout, synch);
+		if (testbits(synch->status, XNSYNCH_CLAIMED))
+			removepq(&owner->claimq, &synch->link);
+		else
+			__setbits(synch->status, XNSYNCH_CLAIMED);
 
-			if (unlikely(testbits(thread->status, XNRMID | XNTIMEO | XNBREAK)))
-				goto unlock_and_exit;
+		insertpqf(&synch->pendq, &thread->plink, thread->cprio);
+		insertpqf(&owner->claimq, &synch->link, thread->cprio);
+		xnsynch_renice_thread(owner, thread->cprio);
+	} else
+		insertpqf(&synch->pendq, &thread->plink, thread->cprio);
 
-			if (unlikely(synch->owner != thread)) {
-				/* Somebody stole us the ownership while we were ready to
-				   run, waiting for the CPU: we need to wait again for the
-				   resource. */
-				if (timeout == XN_INFINITE)
-					goto redo;
-				timeout = xnthread_timeout(thread);
-				if (timeout > 1)	/* Otherwise, it's too late, time elapsed. */
-					goto redo;
-				__setbits(thread->status, XNTIMEO);
-				goto unlock_and_exit;
-			}
-		} else {
-			insertpqf(&synch->pendq, &thread->plink, thread->cprio);
-			xnpod_suspend_thread(thread, XNPEND, timeout, synch);
-		}
-	} else {		/* otherwise FIFO */
-		appendpq(&synch->pendq, &thread->plink);
-		xnpod_suspend_thread(thread, XNPEND, timeout, synch);
-		if (unlikely(testbits(thread->status, XNRMID | XNTIMEO | XNBREAK)))
-			goto unlock_and_exit;
-	}
+	xnpod_suspend_thread(thread, XNPEND, timeout, synch);
 
-      grab_ownership:
+	if (testbits(thread->status, XNRMID | XNTIMEO | XNBREAK))
+		goto unlock_and_exit;
 
-	/* Now the resource is truely owned by the caller. */
-	__clrbits(synch->status, XNSYNCH_PENDING);
+	if (testbits(thread->status, XNROBBED)) {
+		/* Somebody stole us the ownership while we were ready
+		   to run, waiting for the CPU: we need to wait again
+		   for the resource. */
+		if (timeout == XN_INFINITE)
+			goto redo;
+		timeout = xnthread_timeout(thread);
+		if (timeout > 1) /* Otherwise, it's too late. */
+			goto redo;
+		__setbits(thread->status, XNTIMEO);
+	}
 
       unlock_and_exit:
 
+	__clrbits(thread->status, XNWAKEN);
+
 	xnlock_put_irqrestore(&nklock, s);
 }
 
@@ -368,13 +363,11 @@
 		thread = link2thread(holder, plink);
 		thread->wchan = NULL;
 		synch->owner = thread;
-		__setbits(synch->status, XNSYNCH_PENDING);
+		__setbits(thread->status, XNWAKEN);
 		xnltt_log_event(xeno_ev_wakeup1, thread->name, synch);
 		xnpod_resume_thread(thread, XNPEND);
-	} else {
+	} else
 		synch->owner = NULL;
-		__clrbits(synch->status, XNSYNCH_PENDING);
-	}
 
 	if (testbits(synch->status, XNSYNCH_CLAIMED))
 		xnsynch_clear_boost(synch, lastowner);
@@ -441,7 +434,7 @@
 	thread = link2thread(holder, plink);
 	thread->wchan = NULL;
 	synch->owner = thread;
-	__setbits(synch->status, XNSYNCH_PENDING);
+	__setbits(thread->status, XNWAKEN);
 	xnltt_log_event(xeno_ev_wakeupx, thread->name, synch);
 	xnpod_resume_thread(thread, XNPEND);
 
@@ -467,7 +460,7 @@
  * is cleared, such as before the resource is deleted.
  *
  * @param synch The descriptor address of the synchronization object
- * whose ownership is changed.
+ * to be flushed.
  *
  * @param reason Some flags to set in the status mask of every
  * unblocked thread. Zero is an acceptable value. The following bits
@@ -528,9 +521,6 @@
 		status = XNSYNCH_RESCHED;
 	}
 
-	synch->owner = NULL;
-	__clrbits(synch->status, XNSYNCH_PENDING);
-
 	xnlock_put_irqrestore(&nklock, s);
 
 	xnarch_post_graph_if(synch, 0, emptypq_p(&synch->pendq));
Index: include/nucleus/synch.h
===================================================================
--- include/nucleus/synch.h	(revision 1420)
+++ include/nucleus/synch.h	(working copy)
@@ -32,7 +32,6 @@
 #if defined(__KERNEL__) || defined(__XENO_SIM__)
 
 #define XNSYNCH_CLAIMED 0x8	/* Claimed by other thread(s) w/ PIP */
-#define XNSYNCH_PENDING 0x10	/* Pending ownership -- may be stolen  */
 
 /* Spare flags usable by upper interfaces */
 #define XNSYNCH_SPARE0  0x01000000
@@ -86,13 +85,11 @@
 void xnsynch_init(xnsynch_t *synch,
 		  xnflags_t flags);
 
-#define xnsynch_destroy(synch) \
-xnsynch_flush(synch,XNRMID)
+#define xnsynch_destroy(synch) xnsynch_flush(synch,XNRMID)
 
 static inline void xnsynch_set_owner (xnsynch_t *synch, struct xnthread *thread)
 {
     synch->owner = thread;
-    __clrbits(synch->status, XNSYNCH_PENDING);
 }
 
 static inline void xnsynch_register_cleanup (xnsynch_t *synch, void (*handler)(xnsynch_t *))
Index: include/nucleus/thread.h
===================================================================
--- include/nucleus/thread.h	(revision 1420)
+++ include/nucleus/thread.h	(working copy)
@@ -53,6 +53,8 @@
 #define XNSHADOW  0x00800000	/* Shadow thread */
 #define XNROOT    0x01000000	/* Root thread (i.e. Linux/IDLE) */
 #define XNINVPS   0x02000000	/* Using inverted priority scale */
+#define XNWAKEN   0x04000000	/* Thread waken up upon resource availability */
+#define XNROBBED  0x08000000	/* Robbed from resource ownership */
 
 /*
   Must follow the declaration order of the above bits. Status symbols
@@ -73,12 +75,12 @@
   'o' -> Priority coupling off.
   'f' -> FPU enabled (for kernel threads).
 */
-#define XNTHREAD_SLABEL_INIT { \
-  'S', 'W', 'D', 'R', 'U', \
-  '.', '.', '.', 'X', 'H', \
-  '.', '.', '.', '.', 'b', 'T', \
-  'l', 'r', '.', 's', 't', 'o', \
-  'f', '.', '.', '.'		\
+#define XNTHREAD_SLABEL_INIT  {		\
+	'S', 'W', 'D', 'R', 'U',	\
+	'.', '.', '.', 'X', 'H',	\
+	'.', '.', '.', '.', 'b', 'T',	\
+	'l', 'r', '.', 's', 't', 'o',	\
+	'f', '.', '.', '.', '.', '.'	\
 }
 
 #define XNTHREAD_BLOCK_BITS   (XNSUSP|XNPEND|XNDELAY|XNDORMANT|XNRELAX|XNHELD)
-- 
Philippe.




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Xenomai-help] -110 error on rt_task_send... bug?
  2006-08-13 14:47           ` Philippe Gerum
@ 2006-08-13 20:24             ` Philippe Gerum
  0 siblings, 0 replies; 16+ messages in thread
From: Philippe Gerum @ 2006-08-13 20:24 UTC (permalink / raw)
  To: Dmitry Adamushko; +Cc: xenomai, Jan Kiszka

On Sun, 2006-08-13 at 16:47 +0200, Philippe Gerum wrote:
> On Sat, 2006-08-12 at 23:14 +0200, Philippe Gerum wrote:
> 
> > IOW, a construct like "if (synch-> ...." when returning from suspension
> > is the root of all evil. We must not depend on anything requiring such
> > dereference when resuming the thread. A way to fix this would be to
> > introduce a new thread status flag which gets raised whenever some
> > object ownership is stolen, so that we would not have to inspect the
> > synch object anymore to know about such situation, and rely on
> > potentially wrong information from the ->owner field. We would only have
> > to inspect the status flags of the thread that got "robbed" (i.e. the
> > one that resumes) and act accordingly. I will post a patch implementing
> > this, after I have solved all the related issues.
> 
> The patch below provides a more general fix to the issues introduced by
> the resource stealing feature. There is still one case where this code
> could attempt to reuse a stolen resource unsafely,

More precisely, a stolen then deleted resource, and those events must
happen while the caller of xnsynch_sleep_on has been readied from
suspension, but not yet switched in. Deletion is the issue there,
meaning that the resource might be destroyed immediately after it has
been released, without any attempt to synchronize with the next consumer
of such resource.

-- 
Philippe.




^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2006-08-13 20:24 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-08-08  1:50 [Xenomai-help] -110 error on rt_task_send... bug? Vincent Levesque
2006-08-08 22:12 ` Jan Kiszka
2006-08-09  9:35   ` Dmitry Adamushko
2006-08-09 11:42     ` [Xenomai-core] " Dmitry Adamushko
2006-08-09 15:09       ` Ulrich Schwab
2006-08-09 15:42         ` Dmitry Adamushko
2006-08-10 18:14           ` Vincent Levesque
2006-08-10 18:18             ` Dmitry Adamushko
2006-08-10  8:25         ` Jan Kiszka
2006-08-10 10:11           ` Dmitry Adamushko
2006-08-10 22:13     ` Philippe Gerum
2006-08-11  7:50       ` Dmitry Adamushko
2006-08-12 10:33       ` Dmitry Adamushko
2006-08-12 21:14         ` Philippe Gerum
2006-08-13 14:47           ` Philippe Gerum
2006-08-13 20:24             ` Philippe Gerum

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.