Re: [Xenomai-core] [Xenomai-help] How to fix t_delete call in psos skin?

[Alexandre Coffignal <alexandre.coffignal@domain.hid>]

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

Philippe Gerum a écrit :
> On Wed, 2009-11-04 at 10:20 +0100, Alexandre Coffignal wrote:
>   
>> Hello,
>>
>> I'm trying to use t_delete routine in psos skin (xenomai-2.4.7) from a 
>> user space application .
>> I'm encountring two different behaviors:
>> -1- Calling t_delete(0) from a psos task seems to work perfectly (e.g. 
>> task self deletes).
>> -2- Calling t_delete(task_id) with another task id (e.g. current task 
>> tries to delete another task) doesn't result in task deletion but 
>> returns 0 (success).
>>
>> I've been through both psos skin (ksrc/psos+/task.c) and nucleus 
>> (ksrc/nucleus/pod.c) source code.
>>
>> xnpod_delete_thread() behaves differently depending on previously 
>> described t_delete calls:
>>
>> -1- First case (t_delete(0)):
>>
>> Execution skip condition in ksrc/nucleus/pod.c at line 1173:
>>
>>   if (xnthread_user_task(thread) != NULL &&
>>       !xnthread_test_state(thread, XNDORMANT) &&
>>       !xnpod_current_p(thread)) {
>>       if (!xnpod_userspace_p())
>>           xnshadow_send_sig(thread, SIGKILL, 1);
>>       /*
>>        * Otherwise, assume the interface library has issued
>>        * pthread_cancel on the target thread, which should
>>        * cause the current service to be called for
>>        * self-deletion of that thread.
>>        */
>>       goto unlock_and_exit;
>>   }
>> #endif /* CONFIG_XENO_OPT_PERVASIVE */
>>
>> and continues to run after it. This results in a successful deletion.
>>
>>
>> -2- Second case:
>>
>> Execution enters one of the first conditions (if 
>> (xnthread_user_task(thread))...)  but skip the second one : 
>> xnshadow_send_sig(thread, SIGKILL, 1) is not executed;
>> goto unlock_and_exit; instruction is then executed.
>> ->Our thread is never deleted.
>>
>> -3-
>> Modifing actual source code by commenting goto intruction or moving it 
>> into second condition (if (!xnpod_userspace_p()) mentioned above) 
>> results in a working t_delete (task_id).
>>
>> - I'm not sure of the possible side effects of such a modification?
>> - Is it correct to do so?
>>
>>     
>
> Nope, this would defeat the purpose of testing those conditions, and end
> up breaking the nucleus. The implicit logic behind such block is that a
> real-time task should always exit on behalf of its own context, so that
> its Linux side never survives the removal of the mated Xenomai shadow
> TCB. Otherwise, you would get a real-time Xenomai task running without
> any Xenomai TCB.
>
> The fix has to happen in userland, and only there, by implementing what
> the comment suggests, which is still missing from the psos skin. i.e.
> have the t_delete() wrapper issue pthread_cancel() under the hood, in
> src/skins/psos/task.c. The native and POSIX APIs illustrate what is
> needed in this respect, albeit the way they implement this is not 100%
> portable to the psos skin context.
>
> PS: this discussion should rather happen on xenomai-core. Redirecting
> now.
>
>   

Hello,

Thank you for your response.
I tried to adopt the same philosophy as in the native API.
Please find attached a patch that modifies psos skin this way:

in /src/skins/psos+/task.c
A reference on pthread is kept at task creation time (in 
psos_task_trampoline and t_shadow routines) to be retrieved at deletion 
time (in t_delete routine, when tid isn't 0).
When t_delete is called with tid != 0, pthread_cancel() is issued with 
this reference.

include/psos+/task.h
Pthread reference has been added in psostask struct (kernel side) and is 
passed to it by using SKINCALL at creation time.

ksrc/skins/psos+/syscall.c & /include/psos+/syscall.h
Pthread reference is passed to it by using SKINCALL at creation time (in 
__t_create).
An additional skin call (__t_get_pthread) allow to retreive pthread 
reference from user space

Does this fix seem more suitable to you?
Do you see someting else missing?

Please advice.

Fabrice Gasnier


>> As mentioned in the source code comment, it is assumed "the interface 
>> library has issued pthread_cancel" ...
>> I assume it might be missing somewhere?
>>
>> Please can you help on this topic?
>>
>> Thanks in advance.
>>
>> Please find a patch regarding this modification.
>>
>> diff --git a/ksrc/nucleus/pod.c b/ksrc/nucleus/pod.c
>> index 9348ce1..ee362df 100644
>> --- a/ksrc/nucleus/pod.c
>> +++ b/ksrc/nucleus/pod.c
>> @@ -1177,14 +1177,16 @@ void xnpod_delete_thread(xnthread_t *thread)
>>             !xnthread_test_state(thread, XNDORMANT) &&
>>             !xnpod_current_p(thread)) {
>>                 if (!xnpod_userspace_p())
>> +               {
>>                         xnshadow_send_sig(thread, SIGKILL, 1);
>> +                       goto unlock_and_exit;
>> +               }
>>                 /*
>>                  * Otherwise, assume the interface library has issued
>>                  * pthread_cancel on the target thread, which should
>>                  * cause the current service to be called for
>>                  * self-deletion of that thread.
>>                  */
>> -               goto unlock_and_exit;
>>         }
>>  #endif /* CONFIG_XENO_OPT_PERVASIVE */
>>  
>>     
>
>   
>> Fabrice.
>>
>>
>> --------------------------------
>> CénoSYS <http://www.cenosys.com>
>> 10, Rue Xavier Bichat
>> F-72000 Le MANS
>> --------------------------------
>>
>>
>> _______________________________________________
>> Xenomai-help mailing list
>> Xenomai-help@domain.hid
>> https://mail.gna.org/listinfo/xenomai-help
>>     
>
>
>   



[-- Attachment #2: patch_psos_t_delete --]
[-- Type: text/plain, Size: 6826 bytes --]

Index: xenomai-2.4.7_bugless/include/psos+/syscall.h
===================================================================
--- xenomai-2.4.7_bugless.orig/include/psos+/syscall.h	2009-11-05 09:12:29.000000000 +0100
+++ xenomai-2.4.7_bugless/include/psos+/syscall.h	2009-11-05 09:13:14.000000000 +0100
@@ -73,6 +73,7 @@
 #define __psos_as_send      45
 /* Xenomai extension: get raw count of jiffies */
 #define __psos_tm_getc      46
+#define __psos_t_get_pthread		47	/* get hidden pthread_t identifier. */
 
 #ifdef __KERNEL__
 
Index: xenomai-2.4.7_bugless/ksrc/skins/psos+/syscall.c
===================================================================
--- xenomai-2.4.7_bugless.orig/ksrc/skins/psos+/syscall.c	2009-11-05 09:16:48.000000000 +0100
+++ xenomai-2.4.7_bugless/ksrc/skins/psos+/syscall.c	2009-11-05 09:16:57.000000000 +0100
@@ -67,29 +67,37 @@
 	xncompletion_t __user *u_completion;
 	u_long prio, flags, tid, err;
 	char name[XNOBJECT_NAME_LEN];
+	struct arg_bulk5 bulk;
 	psostask_t *task;
 
-	if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg1(regs), 4))
+	if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg1(regs), sizeof(bulk)))
+		return -EFAULT;
+
+	__xn_copy_from_user(curr, &bulk, (void __user *)__xn_reg_arg1(regs),
+			    sizeof(bulk));
+
+	if (!__xn_access_ok(curr, VERIFY_READ, bulk.a1, sizeof(name))) {
 		return -EFAULT;
+	}
 
 	/* Get task name. */
-	__xn_strncpy_from_user(curr, name, (const char __user *)__xn_reg_arg1(regs),
+	__xn_strncpy_from_user(curr, name, (const char __user *)bulk.a1,
 			       sizeof(name) - 1);
 	name[sizeof(name) - 1] = '\0';
 	strncpy(curr->comm, name, sizeof(curr->comm));
 	curr->comm[sizeof(curr->comm) - 1] = '\0';
 
 	if (!__xn_access_ok
-	    (curr, VERIFY_WRITE, __xn_reg_arg4(regs), sizeof(tid)))
+		(curr, VERIFY_WRITE, bulk.a4, sizeof(tid)))
 		return -EFAULT;
 
 	/* Task priority. */
-	prio = __xn_reg_arg2(regs);
+	prio = bulk.a2;
 	/* Task flags. Force FPU support in user-space. This will lead
 	   to a no-op if the platform does not support it. */
-	flags = __xn_reg_arg3(regs) | T_SHADOW | T_FPU;
+	flags = bulk.a3 | T_SHADOW | T_FPU;
 	/* Completion descriptor our parent thread is pending on. */
-	u_completion = (xncompletion_t __user *)__xn_reg_arg5(regs);
+	u_completion = (xncompletion_t __user *)__xn_reg_arg2(regs);
 
 	err = t_create(name, prio, 0, 0, flags, &tid);
 
@@ -99,7 +107,8 @@
 		 * about the new thread id, so we can manipulate its
 		 * TCB pointer freely. */
 		tid = xnthread_handle(&task->threadbase);
-		__xn_copy_to_user(curr, (void __user *)__xn_reg_arg4(regs), &tid,
+		task->opaque2 = bulk.a5; /* hidden pthread_t identifier. */
+		__xn_copy_to_user(curr, (void __user *) bulk.a4, &tid,
 				  sizeof(tid));
 		err = xnshadow_map(&task->threadbase, u_completion); /* May be NULL */
 	} else {
@@ -1443,6 +1452,36 @@
 	return as_send((u_long)task, signals);
 }
 
+
+/*
+ * int __t_get_pthread(u_long tid, u_long *pthread)
+ */
+static int __t_get_pthread(struct task_struct *curr, struct pt_regs *regs)
+{
+	xnhandle_t handle;
+	psostask_t *task;
+	u_long pthread;
+
+	handle = __xn_reg_arg1(regs);
+
+	if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg2(regs), sizeof(u_long)))
+		return -EFAULT;
+
+	if (handle)
+		task = (psostask_t *)xnregistry_fetch(handle);
+	else
+		task = __psos_task_current(curr);
+
+	if (!task)
+		return ERR_OBJID;
+
+	pthread = task->opaque2; /* hidden pthread_t identifier. */
+
+	__xn_copy_to_user(curr, (void __user *) __xn_reg_arg2(regs), &pthread, sizeof(u_long));
+
+	return SUCCESS;
+}
+
 static void *psos_shadow_eventcb(int event, void *data)
 {
 	struct psos_resource_holder *rh;
@@ -1524,6 +1563,7 @@
 	[__psos_tm_signal] = {&__tm_signal, __xn_exec_primary},
 	[__psos_as_send] = {&__as_send, __xn_exec_conforming},
 	[__psos_tm_getc] = {&__tm_getc, __xn_exec_any},
+	[__psos_t_get_pthread] = {&__t_get_pthread, __xn_exec_any},
 };
 
 extern xntbase_t *psos_tbase;
Index: xenomai-2.4.7_bugless/src/skins/psos+/task.c
===================================================================
--- xenomai-2.4.7_bugless.orig/src/skins/psos+/task.c	2009-11-05 09:17:31.000000000 +0100
+++ xenomai-2.4.7_bugless/src/skins/psos+/task.c	2009-11-05 09:17:56.000000000 +0100
@@ -26,6 +26,15 @@
 #include <memory.h>
 #include <psos+/psos.h>
 
+struct arg_bulk5{
+
+    u_long a1;
+    u_long a2;
+    u_long a3;
+    u_long a4;
+    u_long a5;
+};
+
 extern int __psos_muxid;
 
 struct psos_task_iargs {
@@ -73,6 +82,7 @@
 	struct sched_param param;
 	int policy;
 	long err;
+	struct arg_bulk5 bulk;
 
 	policy = psos_task_set_posix_priority(iargs->prio, &param);
 	pthread_setschedparam(pthread_self(), policy, &param);
@@ -81,10 +91,14 @@
 
 	old_sigharden_handler = signal(SIGHARDEN, &psos_task_sigharden);
 
-	err = XENOMAI_SKINCALL5(__psos_muxid,
-				__psos_t_create,
-				iargs->name, iargs->prio, iargs->flags,
-				iargs->tid_r, iargs->completionp);
+	bulk.a1 = (u_long)iargs->name;
+	bulk.a2 = (u_long)iargs->prio;
+	bulk.a3 = (u_long)iargs->flags;
+	bulk.a4 = (u_long)iargs->tid_r;
+	bulk.a5 = (u_long)pthread_self();
+
+	err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_create, &bulk, iargs->completionp);
+
 	if (err)
 		goto fail;
 
@@ -172,14 +186,19 @@
 		u_long flags,
 		u_long *tid_r)
 {
+	struct arg_bulk5 bulk;
+
 	pthread_setcanceltype(PTHREAD_CANCEL_ASYNCHRONOUS, NULL);
 
 	old_sigharden_handler = signal(SIGHARDEN, &psos_task_sigharden);
 
-	return XENOMAI_SKINCALL5(__psos_muxid,
-				 __psos_t_create,
-				 name, prio, flags,
-				 tid_r, NULL);
+	bulk.a1 = (u_long)name;
+	bulk.a2 = (u_long)prio;
+	bulk.a3 = (u_long)flags;
+	bulk.a4 = (u_long)tid_r;
+	bulk.a5 = (u_long)pthread_self();
+
+	return XENOMAI_SKINCALL2(__psos_muxid, __psos_t_create, &bulk, NULL);
 }
 
 u_long t_start(u_long tid,
@@ -196,6 +215,18 @@
 
 u_long t_delete(u_long tid)
 {
+	long err;
+	u_long pthread;
+
+	if(tid)
+	{
+		err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_get_pthread, tid, &pthread);
+		if (err)
+			return err;
+		err = pthread_cancel((pthread_t)pthread);
+		if (err)
+			return err;
+	}
 	return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
 }
 
Index: xenomai-2.4.7_bugless/include/psos+/task.h
===================================================================
--- xenomai-2.4.7_bugless.orig/include/psos+/task.h	2009-11-05 09:19:49.000000000 +0100
+++ xenomai-2.4.7_bugless/include/psos+/task.h	2009-11-05 09:19:58.000000000 +0100
@@ -26,6 +26,16 @@
 
 #define PSOS_TASK_MAGIC 0x81810101
 
+struct arg_bulk5{
+
+    u_long a1;
+    u_long a2;
+    u_long a3;
+    u_long a4;
+    u_long a5;
+};
+
+
 typedef struct psostask {
 
     unsigned magic;   /* Magic code - must be first */
@@ -64,6 +74,8 @@
 
     } waitargs;
 
+    u_long opaque2; /* hidden pthread_t identifier. */
+
 } psostask_t;
 
 static inline psostask_t *thread2psostask (xnthread_t *t)