On Mon, 2007-05-21 at 16:43 +0200, Jan Kiszka wrote:
> Hi Philippe,
> 
> Jan Kiszka wrote:
> > Just ran into this with CONFIG_IPIPE_DEBUG_CONTEXT (maybe due to some
> > bug of my own):
> 
> Here is some code to trigger the issue reliably:
> 
> #include <sys/mman.h>
> #include <native/task.h>
> 
> void task_fnct(void *arg)
> {
>         rt_task_delete(NULL);
> }
> 
> main()
> {
>         RT_TASK task;
>         mlockall(MCL_CURRENT|MCL_FUTURE);
>         rt_task_spawn(&task, "task", 0, 10, 0, task_fnct, NULL);
> }
> 
> 
> > 
> > [  102.616000] I-pipe: Detected illicit call from domain 'Xenomai'
> > [  102.616000]         into a service reserved for domain 'Linux' and below.
> > [  102.616000]        c741bdc8 00000000 00000000 c8860ef8 c741bdec c0105683 
> > c032c200 c13fe22c
> > [  102.616000]        c0361f00 c741be08 c01519ed c032f5b8 c032c742 c03380b3 
> > c8885100 c78beac0
> > [  102.616000]        c741be14 c0142ce9 c7a80b30 c741be3c c884d075 c885f150 
> > c8860ef8 c741be3c
> > [  102.616000] Call Trace:
> > [  102.616000]  [<c0104d9f>] show_trace_log_lvl+0x1f/0x40
> > [  102.616000]  [<c0104e71>] show_stack_log_lvl+0xb1/0xe0
> > [  102.616000]  [<c0105683>] show_stack+0x33/0x40
> > [  102.616000]  [<c01519ed>] ipipe_check_context+0xad/0xc0
> > [  102.616000]  [<c0142ce9>] module_put+0x19/0x90
> > [  102.616000]  [<c884d075>] xnshadow_unmap+0xb5/0x130 [xeno_nucleus]
> > [  102.616000]  [<c8871dc5>] __shadow_delete_hook+0x25/0x30 [xeno_native]
> > [  102.616000]  [<c8842f78>] xnpod_schedule+0xb58/0x12f0 [xeno_nucleus]
> > [  102.616000]  [<c8844bfb>] xnpod_delete_thread+0x2cb/0x3d0 [xeno_nucleus]
> > [  102.616000]  [<c886f5bd>] rt_task_delete+0x20d/0x220 [xeno_native]
> > 
> > I would dare to say that module_put in xnshadow_unmap is not well placed
> > as it can wakeup a Linux process. The module ref-counter maintenance
> > needs some postponing, I guess.
> 

Mm, I should definitely read mails entirely.

> Attached is a patch proposal. It solves the issue by postponing the
> module_put via a new schedule_linux_call. Note that this approach issues
> LO_WAKEUP_REQ where the old test (p->state != TASK_RUNNING) would not
> have done so. I don't see negative side effects yet,

This code has to work with an awful lot of runtime situations, including
those raised by ptracing/GDB, and thread signaling in general, so test
harnessing is key here.

>  and I'm furthermore
> not sure of the old code was handling SMP scenarios safely (What if the
> thread to be unmapped was running on different CPU than xnshadow_unmap?
> How to ensure test-atomicity then?).
> 

This wakeup call is there to release emerging shadows waiting on their
start barrier that get killed before start. Other regular cases imply
that either the current thread is exiting - in which case this is a no
brainer, or it has been sent a group kill signal, in which case it has
to wake up anyway.

> 
> Well, this thing seems to work,

It does, actually.

>  but it doesn't leave me with a good
> feeling. IMHO, there are far too many cleanup cases with all the
> combinations of non-shadow termination, shadow self-termination,
> termination on task exit, various SMP scenarios, etc.
>  Moreover, quite a
> lot of stuff is executed in atomic scheduling hooks (namely on cleanup).
> At least for shadow threads, I don't think all of this is necessary.
> Actually, deferring most of the uncritical cleanup work into Linux
> context would make things more regular, easier to review, and likely
> less disturbing for overall system latencies. I know that atomic
> scheduling hooks are required to implement certain skins, but I don't
> see why janitor work should be done there as well. And we also still
> have that tiny uncleanness on shadow threads termination around TCB
> release vs. hook execution.

Talking about xnshadow_unmap, and as your patch illustrates, what you
could postpone is basically the first part of the routine, which updates
the reference counts. The rest _must_ run over the deletion hook in
atomic context (already runs fine all Linux debug switches on). The
latency hit is certainly not seen there; the real issues are left in the
native and POSIX skin thread deletion routines (heap management and
other housekeeping calls under nklock and so on).

> 
> As we are already at it: LO_MAX_REQUESTS is a hidden limitation that
> may (theoretically) bite large setups when e.g. too many threads gets
> killed from the "wrong" context. At least some overflow detection is
> required here to warn the unfortunate user that he reached hard-coded
> scalability limits.

Yes.

> 
> Jan
> 
> 
> ---
>  ksrc/nucleus/shadow.c |   61 
> +++++++++++++++++++++++++++-----------------------
>  1 file changed, 34 insertions(+), 27 deletions(-)
> 
> Index: xenomai/ksrc/nucleus/shadow.c
> ===================================================================
> --- xenomai.orig/ksrc/nucleus/shadow.c
> +++ xenomai/ksrc/nucleus/shadow.c
> @@ -92,6 +92,7 @@ static struct __lostagerq {
>  #define LO_RENICE_REQ 2
>  #define LO_SIGGRP_REQ 3
>  #define LO_SIGTHR_REQ 4
> +#define LO_UNMAP_REQ  5
>               int type;
>               struct task_struct *task;
>               int arg;
> @@ -778,6 +779,28 @@ static inline void unlock_timers(void)
>               clrbits(nktbase.status, XNTBLCK);
>  }
>  
> +static void xnshadow_dereference_skin(unsigned magic)
> +{
> +     unsigned muxid;
> +
> +     for (muxid = 0; muxid < XENOMAI_MUX_NR; muxid++) {
> +             if (muxtable[muxid].magic == magic) {
> +                     if (xnarch_atomic_dec_and_test(&muxtable[0].refcnt))
> +                             xnarch_atomic_dec(&muxtable[0].refcnt);
> +                     if (xnarch_atomic_dec_and_test(&muxtable[muxid].refcnt))
> +
> +                             /* We were the last thread, decrement the 
> counter,
> +                                since it was incremented by the xn_sys_bind
> +                                operation. */
> +                             xnarch_atomic_dec(&muxtable[muxid].refcnt);
> +                     if (muxtable[muxid].module)
> +                             module_put(muxtable[muxid].module);
> +
> +                     break;
> +             }
> +     }
> +}
> +
>  static void lostage_handler(void *cookie)
>  {
>       int cpuid = smp_processor_id(), reqnum, sig;
> @@ -790,6 +813,12 @@ static void lostage_handler(void *cookie
>               xnltt_log_event(xeno_ev_lohandler, reqnum, p->comm, p->pid);
>  
>               switch (rq->req[reqnum].type) {
> +             case LO_UNMAP_REQ:
> +
> +                     xnshadow_dereference_skin(
> +                             (unsigned)rq->req[reqnum].arg);
> +
> +                     /* fall through */
>               case LO_WAKEUP_REQ:
>  
>  #ifdef CONFIG_SMP
> @@ -1248,7 +1277,6 @@ int xnshadow_map(xnthread_t *thread, xnc
>  void xnshadow_unmap(xnthread_t *thread)
>  {
>       struct task_struct *p;
> -     unsigned muxid, magic;
>  
>       if (XENO_DEBUG(NUCLEUS) &&
>           !testbits(xnpod_current_sched()->status, XNKCOUT))
> @@ -1256,25 +1284,6 @@ void xnshadow_unmap(xnthread_t *thread)
>  
>       p = xnthread_archtcb(thread)->user_task;        /* May be != current */
>  
> -     magic = xnthread_get_magic(thread);
> -
> -     for (muxid = 0; muxid < XENOMAI_MUX_NR; muxid++) {
> -             if (muxtable[muxid].magic == magic) {
> -                     if (xnarch_atomic_dec_and_test(&muxtable[0].refcnt))
> -                             xnarch_atomic_dec(&muxtable[0].refcnt);
> -                     if (xnarch_atomic_dec_and_test(&muxtable[muxid].refcnt))
> -
> -                             /* We were the last thread, decrement the 
> counter,
> -                                since it was incremented by the xn_sys_bind
> -                                operation. */
> -                             xnarch_atomic_dec(&muxtable[muxid].refcnt);
> -                     if (muxtable[muxid].module)
> -                             module_put(muxtable[muxid].module);
> -
> -                     break;
> -             }
> -     }
> -
>       xnthread_clear_state(thread, XNMAPPED);
>       rpi_pop(thread);
>  
> @@ -1285,13 +1294,7 @@ void xnshadow_unmap(xnthread_t *thread)
>  
>       xnshadow_thrptd(p) = NULL;
>  
> -     if (p->state != TASK_RUNNING)
> -             /* If the shadow is being unmapped in primary mode or blocked
> -                in secondary mode, the associated Linux task should also
> -                die. In the former case, the zombie Linux side returning to
> -                user-space will be trapped and exited inside the pod's
> -                rescheduling routines. */
> -             schedule_linux_call(LO_WAKEUP_REQ, p, 0);
> +     schedule_linux_call(LO_UNMAP_REQ, p, xnthread_get_magic(thread));
>  }
>  
>  int xnshadow_wait_barrier(struct pt_regs *regs)
> @@ -1984,6 +1987,7 @@ RTHAL_DECLARE_EVENT(losyscall_event);
>  static inline void do_taskexit_event(struct task_struct *p)
>  {
>       xnthread_t *thread = xnshadow_thread(p); /* p == current */
> +     unsigned magic;
>       spl_t s;
>  
>       if (!thread)
> @@ -1992,6 +1996,8 @@ static inline void do_taskexit_event(str
>       if (xnpod_shadow_p())
>               xnshadow_relax(0);
>  
> +     magic = xnthread_get_magic(thread);
> +
>       xnlock_get_irqsave(&nklock, s);
>       /* Prevent wakeup call from xnshadow_unmap(). */
>       xnshadow_thrptd(p) = NULL;
> @@ -2002,6 +2008,7 @@ static inline void do_taskexit_event(str
>       xnlock_put_irqrestore(&nklock, s);
>       xnpod_schedule();
>  
> +     xnshadow_dereference_skin(magic);
>       xnltt_log_event(xeno_ev_shadowexit, thread->name);
>  }
>  
> 
> 
-- 
Philippe.



_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to