All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [Xenomai-core] [Fwd: Re: [Xenomai-help] How to fix t_delete call in psos skin?]
@ 2009-12-07  9:42 Fabrice Gasnier
  2009-12-15 16:54 ` Philippe Gerum
  0 siblings, 1 reply; 6+ messages in thread
From: Fabrice Gasnier @ 2009-12-07  9:42 UTC (permalink / raw)
  To: Philippe Gerum; +Cc: xenomai

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

Hello,

I come back to you to submit a slightly different patch.
I observed that __psos_t_delete sometimes returns ERR_OBJID after 
pthread_cancel has been issued, due to thread isn't found on kernel side.
The attached patch uses the same mechanism as in native API 
(rt_task_delete) where equivalent error is being filtered.

Please find attached this patch (formated with git format-patch origin).

If it's looks like ok on your side, do you think this patch can be 
integrated in xenomai project?

Best regards,

Fabrice

Philippe Gerum a écrit :
> On Fri, 2009-11-13 at 15:11 +0100, Alexandre Coffignal wrote:
>  
>> Hello,
>>
>> Please find attached a new patch that should take into account your 
>> latest remarks.
>> This patch has been created using 'git diff -p' command and applies 
>> on 2.4.10 revision.
>> I'm not sure I fully understand your remark on missing critical 
>> section. Indeed, this code is similar to __t_delete routine that 
>> doesn't seem to have critical section enforcement?
>> Should a critical section be added to it as well ?
>>     
>
> No, __t_delete() is safe because it does not directly dereference the
> task pointer fetched from the registry, but simply passes it to the
> actual t_delete() syscall, which will revalidate that pointer before
> doing anything silly with it.
>
> __t_get_pthread() on the other hand, does dereference the task pointer
> to fetch the opaque handle, which is unsafe unless this is done within a
> critical section.
>
>  
>> Anyway, I tried to replicate __t_ident mechanism as you mentioned.
>>
>> Please let me know your feeling about this.
>>
>> Fabrice
>>    
>>>
>>>
>>> On Thu, 2009-11-05 at 11:54 +0100, Alexandre Coffignal wrote:
>>>
>>>        
>>>> Does this fix seem more suitable to you?
>>>> Do you see someting else missing?
>>>>
>>>>             
>>> The logic is fine, but the implementation should get closer to what is
>>> being done in the -head branch.
>>>
>>> Preliminary remarks:
>>>
>>> - Please follow the kernel CodingStyle guidelines for anything that is
>>> Xenomai-related, including userland code. Keeping a single code style
>>> throughout our code makes things easier for reviewing, and keeps visual
>>> pollution to the lowest possible level.
>>>
>>> - Please rebase your work on 2.4.10 for pushing those changes to 
>>> Xenomai
>>> mainline. There is no point for us to consider changes to legacy
>>> releases which are not being actively maintained anymore; you can 
>>> always
>>> backport those changes to 2.4.7 locally if you really fancy dealing 
>>> with
>>> bugs solved between 2.4.7 and 2.4.10...
>>>
>>> - Please tell your CVS/SVN diff tool to use -p, when formatting 
>>> patches.
>>>  
>>>        
>>>> Please advice.
>>>>             
>>>        
>>>> plain text document attachment (patch_psos_t_delete)
>>>> 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. */
>>>>             
>>> psos_t_getpth is enough. To be really, really pSOS-conformant when
>>> introducing a routine, we should either use:
>>>
>>> 1) a preposterously long argument list
>>>
>>> or, at the very least:
>>>  
>>> 2) an absolutely unpronounceable C identifier
>>>
>>> __psos_t_getpth happily qualifies for #2.
>>>
>>>        
>>>>  
>>>>  #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;
>>>>             
>>> Please have a look at what is being done in the -head branch, for the
>>> very same routine (in the absence of -p passed to diff, seems to be
>>> __t_create). An arg bulk has been introduced there, so you just need to
>>> backport this.
>>>
>>>        
>>>>      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;
>>>>             
>>> Critical section enforcement is missing. What if the caller gets
>>> preempted after the handle is fetched, and the non-current tid gets
>>> killed by a concurrent thread right after? See t_ident for an
>>> illustration.
>>>
>>> If the deletion happens after the critical section, well, we would just
>>> have to hope that pthread_cancel() does a good job at validating its
>>> handle, but at least we would not have referred to stale memory on our
>>> end.
>>>
>>>        
>>>> +
>>>> +    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;
>>>> +};
>>>>             
>>> Rather use psos_arg_bulk like in -head tree.
>>>
>>>        
>>>> +
>>>>  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;
>>>> +    }
>>>>             
>>> We should handle the self-deletion case specifically:
>>>
>>> +    else {
>>> +        /* Silently migrate to avoid raising SIGXCPU. */
>>> +        XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
>>> +        pthread_exit(NULL);
>>> +    }
>>>
>>> Actually, we should also test for the case when a non-zero tid ends up
>>> pointing at the current thread as well, e.g.
>>>
>>>         if (tid == 0)
>>>             goto self_delete;
>>>
>>>         err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, 
>>> &ptid);
>>>         if (err)
>>>             return err;
>>>
>>>         if ((pthread_t)ptid == pthread_self())
>>>             goto self_delete;
>>>
>>>         err = pthread_cancel((pthread_t)ptid);
>>>         if (err)
>>>             return -err; /* differentiate from pSOS codes */
>>>
>>>         return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
>>> self_delete:
>>>          /* Silently migrate to avoid raising SIGXCPU. */         
>>> XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
>>>         pthread_exit(NULL);
>>>
>>>         return SUCCESS; /* not reached */
>>>
>>>        
>>>>      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. */
>>>>             
>>> No need to make this more opaque than required to the reviewer; 
>>> besides,
>>> we only need this field when user-space support is compiled in.
>>>
>>> -    u_long opaque2; /* hidden pthread_t identifier. */
>>>
>>> +    #ifdef CONFIG_XENO_OPT_PERVASIVE
>>> +        u_long pthread;
>>> +    #endif
>>>
>>>        
>>>> +
>>>>  } psostask_t;
>>>>  
>>>>  static inline psostask_t *thread2psostask (xnthread_t *t)
>>>>             
>>>         
>> plain text document attachment (patch_psos_t_delete)
>> diff --git a/include/psos+/syscall.h b/include/psos+/syscall.h
>> index 55ee8a0..25c0a2d 100644
>> --- a/include/psos+/syscall.h
>> +++ b/include/psos+/syscall.h
>> @@ -73,6 +73,16 @@
>>  #define __psos_as_send      45
>>  /* Xenomai extension: get raw count of jiffies */
>>  #define __psos_tm_getc      46
>> +#define __psos_t_getpth        47    /* get hidden pthread_t 
>> identifier. */
>> +
>> +struct psos_arg_bulk{
>> +
>> +    u_long a1;
>> +    u_long a2;
>> +    u_long a3;
>> +    u_long a4;
>> +    u_long a5;
>> +};
>>  
>>  #ifdef __KERNEL__
>>  
>> diff --git a/include/psos+/task.h b/include/psos+/task.h
>> index ca01573..7294f0a 100644
>> --- a/include/psos+/task.h
>> +++ b/include/psos+/task.h
>> @@ -64,6 +64,10 @@ typedef struct psostask {
>>  
>>      } waitargs;
>>  
>> +    #ifdef CONFIG_XENO_OPT_PERVASIVE
>> +    u_long pthread; /* hidden pthread_t identifier. */
>> +    #endif
>> +
>>  } psostask_t;
>>  
>>  static inline psostask_t *thread2psostask (xnthread_t *t)
>> diff --git a/ksrc/skins/psos+/syscall.c b/ksrc/skins/psos+/syscall.c
>> index 1753901..a16cec5 100644
>> --- a/ksrc/skins/psos+/syscall.c
>> +++ b/ksrc/skins/psos+/syscall.c
>> @@ -67,29 +67,37 @@ static int __t_create(struct task_struct *curr, 
>> struct pt_regs *regs)
>>      xncompletion_t __user *u_completion;
>>      u_long prio, flags, tid, err;
>>      char name[XNOBJECT_NAME_LEN];
>> +    struct psos_arg_bulk 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,10 @@ static int __t_create(struct task_struct *curr, 
>> struct pt_regs *regs)
>>           * 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,
>> +        #ifdef CONFIG_XENO_OPT_PERVASIVE
>> +        task->pthread = bulk.a5; /* hidden pthread_t identifier. */
>> +        #endif
>> +        __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 +1454,47 @@ static int __as_send(struct task_struct *curr, 
>> struct pt_regs *regs)
>>      return as_send((u_long)task, signals);
>>  }
>>  
>> +
>> +/*
>> + * int __t_getpth(u_long tid, u_long *pthread)
>> + */
>> +static int __t_getpth(struct task_struct *curr, struct pt_regs *regs)
>> +{
>> +    xnhandle_t handle;
>> +    psostask_t *task;
>> +    spl_t s;
>> +    u_long err = SUCCESS;
>> +    u_long pthread;
>> +
>> +    handle = __xn_reg_arg1(regs);
>> +
>> +    if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg2(regs), 
>> sizeof(u_long)))
>> +        return -EFAULT;
>> +
>> +    xnlock_get_irqsave(&nklock, s);
>> +
>> +    if (handle)
>> +        task = (psostask_t *)xnregistry_fetch(handle);
>> +    else
>> +        task = __psos_task_current(curr);
>> +
>> +    if (!task)
>> +        err = ERR_OBJID;
>> +    else
>> +    {
>> +        #ifdef CONFIG_XENO_OPT_PERVASIVE
>> +        pthread = task->pthread; /* hidden pthread_t identifier. */
>> +        #endif
>> +    }
>> +
>> +    xnlock_put_irqrestore(&nklock, s);
>> +
>> +    if(err == SUCCESS)
>> +        __xn_copy_to_user(curr, (void __user *) __xn_reg_arg2(regs), 
>> &pthread, sizeof(u_long));
>> +
>> +    return err;
>> +}
>> +
>>  static void *psos_shadow_eventcb(int event, void *data)
>>  {
>>      struct psos_resource_holder *rh;
>> @@ -1524,6 +1576,7 @@ static xnsysent_t __systab[] = {
>>      [__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_getpth] = {&__t_getpth, __xn_exec_any},
>>  };
>>  
>>  extern xntbase_t *psos_tbase;
>> diff --git a/src/skins/psos+/task.c b/src/skins/psos+/task.c
>> index 9358ea8..62e2419 100644
>> --- a/src/skins/psos+/task.c
>> +++ b/src/skins/psos+/task.c
>> @@ -73,6 +73,7 @@ static void *psos_task_trampoline(void *cookie)
>>      struct sched_param param;
>>      int policy;
>>      long err;
>> +    struct psos_arg_bulk bulk;
>>  
>>      policy = psos_task_set_posix_priority(iargs->prio, &param);
>>      pthread_setschedparam(pthread_self(), policy, &param);
>> @@ -81,10 +82,14 @@ static void *psos_task_trampoline(void *cookie)
>>  
>>      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 +177,19 @@ u_long t_shadow(const char *name, /* Xenomai 
>> extension. */
>>          u_long flags,
>>          u_long *tid_r)
>>  {
>> +    struct psos_arg_bulk 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,7 +206,32 @@ u_long t_start(u_long tid,
>>  
>>  u_long t_delete(u_long tid)
>>  {
>> +    long err;
>> +    u_long ptid;
>> +
>> +    if (tid == 0)
>> +        goto self_delete;
>> +
>> +    err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, &ptid);
>> +    if (err)
>> +        return err;
>> +
>> +    if ((pthread_t)ptid == pthread_self())
>> +        goto self_delete;
>> +
>> +    err = pthread_cancel((pthread_t)ptid);
>> +    if (err)
>> +        return -err; /* differentiate from pSOS codes */
>> +
>>      return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
>> +
>> +self_delete:
>> +
>> +     /* Silently migrate to avoid raising SIGXCPU. */
>> +    XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
>> +    pthread_exit(NULL);
>> +
>> +    return SUCCESS; /* not reached */
>>  }
>>  
>>  u_long t_suspend(u_long tid)
>> _______________________________________________
>> Xenomai-core mailing list
>> Xenomai-core@domain.hid
>> https://mail.gna.org/listinfo/xenomai-core
>>     
>
>
>   


[-- Attachment #2: 0001-Fix-t_delete-call-in-psos-skin-from-user-land.patch --]
[-- Type: text/x-patch, Size: 7567 bytes --]

>From 9235829ceaf327c53929c9138eb1b783d1b83b1c Mon Sep 17 00:00:00 2001
From: Fabrice Gasnier <fabrice.gasnier@domain.hid>
Date: Mon, 7 Dec 2009 09:42:19 +0100
Subject: [PATCH] Fix t_delete call in psos skin from user land

---
 include/psos+/syscall.h    |   10 ++++++
 include/psos+/task.h       |    4 ++
 ksrc/skins/psos+/syscall.c |   67 +++++++++++++++++++++++++++++++++++++++----
 src/skins/psos+/task.c     |   57 +++++++++++++++++++++++++++++++------
 4 files changed, 122 insertions(+), 16 deletions(-)

diff --git a/include/psos+/syscall.h b/include/psos+/syscall.h
index 55ee8a0..25c0a2d 100644
--- a/include/psos+/syscall.h
+++ b/include/psos+/syscall.h
@@ -73,6 +73,16 @@
 #define __psos_as_send      45
 /* Xenomai extension: get raw count of jiffies */
 #define __psos_tm_getc      46
+#define __psos_t_getpth		47	/* get hidden pthread_t identifier. */
+
+struct psos_arg_bulk{
+
+    u_long a1;
+    u_long a2;
+    u_long a3;
+    u_long a4;
+    u_long a5;
+};
 
 #ifdef __KERNEL__
 
diff --git a/include/psos+/task.h b/include/psos+/task.h
index ca01573..7294f0a 100644
--- a/include/psos+/task.h
+++ b/include/psos+/task.h
@@ -64,6 +64,10 @@ typedef struct psostask {
 
     } waitargs;
 
+	#ifdef CONFIG_XENO_OPT_PERVASIVE
+    u_long pthread; /* hidden pthread_t identifier. */
+	#endif
+
 } psostask_t;
 
 static inline psostask_t *thread2psostask (xnthread_t *t)
diff --git a/ksrc/skins/psos+/syscall.c b/ksrc/skins/psos+/syscall.c
index 1753901..a16cec5 100644
--- a/ksrc/skins/psos+/syscall.c
+++ b/ksrc/skins/psos+/syscall.c
@@ -67,29 +67,37 @@ static int __t_create(struct task_struct *curr, struct pt_regs *regs)
 	xncompletion_t __user *u_completion;
 	u_long prio, flags, tid, err;
 	char name[XNOBJECT_NAME_LEN];
+	struct psos_arg_bulk 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,10 @@ static int __t_create(struct task_struct *curr, struct pt_regs *regs)
 		 * 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,
+		#ifdef CONFIG_XENO_OPT_PERVASIVE
+		task->pthread = bulk.a5; /* hidden pthread_t identifier. */
+		#endif
+		__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 +1454,47 @@ static int __as_send(struct task_struct *curr, struct pt_regs *regs)
 	return as_send((u_long)task, signals);
 }
 
+
+/*
+ * int __t_getpth(u_long tid, u_long *pthread)
+ */
+static int __t_getpth(struct task_struct *curr, struct pt_regs *regs)
+{
+	xnhandle_t handle;
+	psostask_t *task;
+	spl_t s;
+	u_long err = SUCCESS;
+	u_long pthread;
+
+	handle = __xn_reg_arg1(regs);
+
+	if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg2(regs), sizeof(u_long)))
+		return -EFAULT;
+
+	xnlock_get_irqsave(&nklock, s);
+
+	if (handle)
+		task = (psostask_t *)xnregistry_fetch(handle);
+	else
+		task = __psos_task_current(curr);
+
+	if (!task)
+		err = ERR_OBJID;
+	else
+	{
+		#ifdef CONFIG_XENO_OPT_PERVASIVE
+		pthread = task->pthread; /* hidden pthread_t identifier. */
+		#endif
+	}
+
+	xnlock_put_irqrestore(&nklock, s);
+
+	if(err == SUCCESS)
+		__xn_copy_to_user(curr, (void __user *) __xn_reg_arg2(regs), &pthread, sizeof(u_long));
+
+	return err;
+}
+
 static void *psos_shadow_eventcb(int event, void *data)
 {
 	struct psos_resource_holder *rh;
@@ -1524,6 +1576,7 @@ static xnsysent_t __systab[] = {
 	[__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_getpth] = {&__t_getpth, __xn_exec_any},
 };
 
 extern xntbase_t *psos_tbase;
diff --git a/src/skins/psos+/task.c b/src/skins/psos+/task.c
index 9358ea8..7121a52 100644
--- a/src/skins/psos+/task.c
+++ b/src/skins/psos+/task.c
@@ -73,6 +73,7 @@ static void *psos_task_trampoline(void *cookie)
 	struct sched_param param;
 	int policy;
 	long err;
+	struct psos_arg_bulk bulk;
 
 	policy = psos_task_set_posix_priority(iargs->prio, &param);
 	pthread_setschedparam(pthread_self(), policy, &param);
@@ -81,10 +82,14 @@ static void *psos_task_trampoline(void *cookie)
 
 	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 +177,19 @@ u_long t_shadow(const char *name, /* Xenomai extension. */
 		u_long flags,
 		u_long *tid_r)
 {
+	struct psos_arg_bulk 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,7 +206,36 @@ u_long t_start(u_long tid,
 
 u_long t_delete(u_long tid)
 {
-	return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
+	long err;
+	u_long ptid;
+
+	if (tid == 0)
+		goto self_delete;
+
+	err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, &ptid);
+	if (err)
+		return err;
+
+	if ((pthread_t)ptid == pthread_self())
+		goto self_delete;
+
+	err = pthread_cancel((pthread_t)ptid);
+	if (err)
+		return -err; /* differentiate from pSOS codes */
+
+	err = XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
+	if (err == ERR_OBJID)
+		return SUCCESS;
+
+	return err;
+
+self_delete:
+
+	 /* Silently migrate to avoid raising SIGXCPU. */
+	XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
+	pthread_exit(NULL);
+
+	return SUCCESS; /* not reached */
 }
 
 u_long t_suspend(u_long tid)
-- 
1.6.0.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [Xenomai-core] [Fwd: Re: [Xenomai-help] How to fix t_delete call in psos skin?]
@ 2009-11-13 14:11 Alexandre Coffignal
  2009-11-13 14:32 ` Philippe Gerum
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandre Coffignal @ 2009-11-13 14:11 UTC (permalink / raw)
  To: xenomai; +Cc: 'Fabrice Gasnier'

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

Hello,

Please find attached a new patch that should take into account your 
latest remarks.
This patch has been created using 'git diff -p' command and applies on 
2.4.10 revision.
I'm not sure I fully understand your remark on missing critical section. 
Indeed, this code is similar to __t_delete routine that doesn't seem to 
have critical section enforcement?
Should a critical section be added to it as well ?
Anyway, I tried to replicate __t_ident mechanism as you mentioned.

Please let me know your feeling about this.

Fabrice

> -------- Message original --------
> Sujet: 	Re: [Xenomai-help] How to fix t_delete call in psos skin?
> Date: 	Fri, 06 Nov 2009 12:28:33 +0100
> De: 	Philippe Gerum <rpm@xenomai.org>
> Organisation: 	Xenomai
> Pour :: 	Alexandre Coffignal <alexandre.coffignal@domain.hid>
> Copie à :: 	xenomai@xenomai.org
> Références: 	<4AF1474C.7000701@domain.hid>
> <1257331856.2210.85.camel@domain.hid>
> <4AF2AEF9.2050207@domain.hid>
>
>
>
> On Thu, 2009-11-05 at 11:54 +0100, Alexandre Coffignal wrote:
>
>   
>> Does this fix seem more suitable to you?
>> Do you see someting else missing?
>>
>>     
>
> The logic is fine, but the implementation should get closer to what is
> being done in the -head branch.
>
> Preliminary remarks:
>
> - Please follow the kernel CodingStyle guidelines for anything that is
> Xenomai-related, including userland code. Keeping a single code style
> throughout our code makes things easier for reviewing, and keeps visual
> pollution to the lowest possible level.
>
> - Please rebase your work on 2.4.10 for pushing those changes to Xenomai
> mainline. There is no point for us to consider changes to legacy
> releases which are not being actively maintained anymore; you can always
> backport those changes to 2.4.7 locally if you really fancy dealing with
> bugs solved between 2.4.7 and 2.4.10...
>
> - Please tell your CVS/SVN diff tool to use -p, when formatting patches.
>  
>   
>> Please advice.
>>     
>
>   
>> plain text document attachment (patch_psos_t_delete)
>> 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. */
>>     
>
> psos_t_getpth is enough. To be really, really pSOS-conformant when
> introducing a routine, we should either use:
>
> 1) a preposterously long argument list
>
> or, at the very least:
>  
> 2) an absolutely unpronounceable C identifier
>
> __psos_t_getpth happily qualifies for #2.
>
>   
>>  
>>  #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;
>>     
>
> Please have a look at what is being done in the -head branch, for the
> very same routine (in the absence of -p passed to diff, seems to be
> __t_create). An arg bulk has been introduced there, so you just need to
> backport this.
>
>   
>>  	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;
>>     
>
> Critical section enforcement is missing. What if the caller gets
> preempted after the handle is fetched, and the non-current tid gets
> killed by a concurrent thread right after? See t_ident for an
> illustration.
>
> If the deletion happens after the critical section, well, we would just
> have to hope that pthread_cancel() does a good job at validating its
> handle, but at least we would not have referred to stale memory on our
> end.
>
>   
>> +
>> +	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;
>> +};
>>     
>
> Rather use psos_arg_bulk like in -head tree.
>
>   
>> +
>>  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;
>> +	}
>>     
>
> We should handle the self-deletion case specifically:
>
> +	else {
> +		/* Silently migrate to avoid raising SIGXCPU. */
> +		XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
> +		pthread_exit(NULL);
> +	}
>
> Actually, we should also test for the case when a non-zero tid ends up
> pointing at the current thread as well, e.g.
>
> 		if (tid == 0)
> 			goto self_delete;
>
> 		err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, &ptid);
> 		if (err)
> 			return err;
>
> 		if ((pthread_t)ptid == pthread_self())
> 			goto self_delete;
>
> 		err = pthread_cancel((pthread_t)ptid);
> 		if (err)
> 			return -err; /* differentiate from pSOS codes */
>
> 		return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
> self_delete:
> 		 /* Silently migrate to avoid raising SIGXCPU. */ 
> 		XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
> 		pthread_exit(NULL);
>
> 		return SUCCESS; /* not reached */
>
>   
>>  	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. */
>>     
>
> No need to make this more opaque than required to the reviewer; besides,
> we only need this field when user-space support is compiled in.
>
> -	u_long opaque2; /* hidden pthread_t identifier. */
>
> +	#ifdef CONFIG_XENO_OPT_PERVASIVE
> +		u_long pthread;
> +	#endif
>
>   
>> +
>>  } psostask_t;
>>  
>>  static inline psostask_t *thread2psostask (xnthread_t *t)
>>     
>
>
>   



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

diff --git a/include/psos+/syscall.h b/include/psos+/syscall.h
index 55ee8a0..25c0a2d 100644
--- a/include/psos+/syscall.h
+++ b/include/psos+/syscall.h
@@ -73,6 +73,16 @@
 #define __psos_as_send      45
 /* Xenomai extension: get raw count of jiffies */
 #define __psos_tm_getc      46
+#define __psos_t_getpth		47	/* get hidden pthread_t identifier. */
+
+struct psos_arg_bulk{
+
+    u_long a1;
+    u_long a2;
+    u_long a3;
+    u_long a4;
+    u_long a5;
+};
 
 #ifdef __KERNEL__
 
diff --git a/include/psos+/task.h b/include/psos+/task.h
index ca01573..7294f0a 100644
--- a/include/psos+/task.h
+++ b/include/psos+/task.h
@@ -64,6 +64,10 @@ typedef struct psostask {
 
     } waitargs;
 
+	#ifdef CONFIG_XENO_OPT_PERVASIVE
+    u_long pthread; /* hidden pthread_t identifier. */
+	#endif
+
 } psostask_t;
 
 static inline psostask_t *thread2psostask (xnthread_t *t)
diff --git a/ksrc/skins/psos+/syscall.c b/ksrc/skins/psos+/syscall.c
index 1753901..a16cec5 100644
--- a/ksrc/skins/psos+/syscall.c
+++ b/ksrc/skins/psos+/syscall.c
@@ -67,29 +67,37 @@ static int __t_create(struct task_struct *curr, struct pt_regs *regs)
 	xncompletion_t __user *u_completion;
 	u_long prio, flags, tid, err;
 	char name[XNOBJECT_NAME_LEN];
+	struct psos_arg_bulk 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,10 @@ static int __t_create(struct task_struct *curr, struct pt_regs *regs)
 		 * 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,
+		#ifdef CONFIG_XENO_OPT_PERVASIVE
+		task->pthread = bulk.a5; /* hidden pthread_t identifier. */
+		#endif
+		__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 +1454,47 @@ static int __as_send(struct task_struct *curr, struct pt_regs *regs)
 	return as_send((u_long)task, signals);
 }
 
+
+/*
+ * int __t_getpth(u_long tid, u_long *pthread)
+ */
+static int __t_getpth(struct task_struct *curr, struct pt_regs *regs)
+{
+	xnhandle_t handle;
+	psostask_t *task;
+	spl_t s;
+	u_long err = SUCCESS;
+	u_long pthread;
+
+	handle = __xn_reg_arg1(regs);
+
+	if (!__xn_access_ok(curr, VERIFY_READ, __xn_reg_arg2(regs), sizeof(u_long)))
+		return -EFAULT;
+
+	xnlock_get_irqsave(&nklock, s);
+
+	if (handle)
+		task = (psostask_t *)xnregistry_fetch(handle);
+	else
+		task = __psos_task_current(curr);
+
+	if (!task)
+		err = ERR_OBJID;
+	else
+	{
+		#ifdef CONFIG_XENO_OPT_PERVASIVE
+		pthread = task->pthread; /* hidden pthread_t identifier. */
+		#endif
+	}
+
+	xnlock_put_irqrestore(&nklock, s);
+
+	if(err == SUCCESS)
+		__xn_copy_to_user(curr, (void __user *) __xn_reg_arg2(regs), &pthread, sizeof(u_long));
+
+	return err;
+}
+
 static void *psos_shadow_eventcb(int event, void *data)
 {
 	struct psos_resource_holder *rh;
@@ -1524,6 +1576,7 @@ static xnsysent_t __systab[] = {
 	[__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_getpth] = {&__t_getpth, __xn_exec_any},
 };
 
 extern xntbase_t *psos_tbase;
diff --git a/src/skins/psos+/task.c b/src/skins/psos+/task.c
index 9358ea8..62e2419 100644
--- a/src/skins/psos+/task.c
+++ b/src/skins/psos+/task.c
@@ -73,6 +73,7 @@ static void *psos_task_trampoline(void *cookie)
 	struct sched_param param;
 	int policy;
 	long err;
+	struct psos_arg_bulk bulk;
 
 	policy = psos_task_set_posix_priority(iargs->prio, &param);
 	pthread_setschedparam(pthread_self(), policy, &param);
@@ -81,10 +82,14 @@ static void *psos_task_trampoline(void *cookie)
 
 	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 +177,19 @@ u_long t_shadow(const char *name, /* Xenomai extension. */
 		u_long flags,
 		u_long *tid_r)
 {
+	struct psos_arg_bulk 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,7 +206,32 @@ u_long t_start(u_long tid,
 
 u_long t_delete(u_long tid)
 {
+	long err;
+	u_long ptid;
+
+	if (tid == 0)
+		goto self_delete;
+
+	err = XENOMAI_SKINCALL2(__psos_muxid, __psos_t_getpth, tid, &ptid);
+	if (err)
+		return err;
+
+	if ((pthread_t)ptid == pthread_self())
+		goto self_delete;
+
+	err = pthread_cancel((pthread_t)ptid);
+	if (err)
+		return -err; /* differentiate from pSOS codes */
+
 	return XENOMAI_SKINCALL1(__psos_muxid, __psos_t_delete, tid);
+
+self_delete:
+
+	 /* Silently migrate to avoid raising SIGXCPU. */
+	XENOMAI_SYSCALL1(__xn_sys_migrate, XENOMAI_LINUX_DOMAIN);
+	pthread_exit(NULL);
+
+	return SUCCESS; /* not reached */
 }
 
 u_long t_suspend(u_long tid)

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-12-18 18:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-12-07  9:42 [Xenomai-core] [Fwd: Re: [Xenomai-help] How to fix t_delete call in psos skin?] Fabrice Gasnier
2009-12-15 16:54 ` Philippe Gerum
2009-12-16  9:53   ` Fabrice Gasnier
2009-12-18 18:18     ` Philippe Gerum
  -- strict thread matches above, loose matches on Subject: below --
2009-11-13 14:11 Alexandre Coffignal
2009-11-13 14:32 ` 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.