[Xenomai-core] [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Xenomai-core] [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Dmitry Adamushko]

Hi,

the following question/suggestion :

it could be good to eliminate the use of rthal_critical_enter/exit()
from rthal_irq_request() if it's not
strictly necessary.


the proposal :

int rthal_irq_request (unsigned irq,
		       rthal_irq_handler_t handler,
		       rthal_irq_ackfn_t ackfn,
		       void *cookie)
{
-    unsigned long flags;
    int err = 0;

    if (handler == NULL || irq >= IPIPE_NR_IRQS)
	return -EINVAL;

-    flags = rthal_critical_enter(NULL);
-
-    if (rthal_irq_handler(&rthal_domain, irq) != NULL)
-	{
-	err = -EBUSY;
-	goto unlock_and_exit;
-	}
-
    err = rthal_virtualize_irq(&rthal_domain,
			       irq,
			       handler,
			       cookie,
			       ackfn,
-			       IPIPE_DYNAMIC_MASK);
+				IPIPE_DYNAMIC_MASK|IPIPE_EXCLUSIVE_MASK);

- unlock_and_exit:
-
-    rthal_critical_exit(flags);
-
    return err;
}

IPIPE_EXCLUSIVE_MASK causes a -EBUZY error to be returned by
ipipe_virtualize_irq() when
handler != NULL and the current ipd->irqs[irq].handler != NULL.

(IPIPE_EXCLUSIVE_MASK is actually not used at the moment anywere,
though ipipe_catch_event() is mentioned in comments).

Another variant :

ipipe_virtualize_irq() should always return -EBUZY when handler !=
NULL and the current ipd->irqs[irq].handler != NULL,
not taking into account the IPIPE_EXCLUSIVE_FLAG.

should work if :

o  all the ipipe_domain structs are "zeroed" upon initialization (ok,
in case of static or global);

o  ipipe_virtualize_irq(..., handler=NULL,...) is always called
between possible consecutive ipipe_virtualize_irq(..., handler!=NULL,
...) calls.

But, yep, this way we enforce a policy for ipipe_virtualize_irq() so
that the use of IPIPE_EXCLUSIVE_FLAG is likely better.
esp. for the nucleus where every rthal_irq_request() has a conforming
rthal_irq_release() call.


Why do I want to eliminate it?

o  any function that make use of critical_enter/exit() must not be
called when a lock (e.g. "nklock") is held.

    ok, xnintr_attach() was always the case and it's used properly
e.g. in native::rt_intr_create().

    but xnintr_detach() is now the case too (heh.. I have overlooked
it in the first instance) because
    xnintr_shirq_detach() is synchronized wrt xnintr_shirq_attach()
using critical_enter/exit(). This is
    only because xnintr_shirq_attach() make use of rthal_irq_request()
---> rthal_critical_enter/exit(), hence
    the nklock can't be used in xnintr_shirq_*.

    Yep, another approach is to enforce the policy that both
xnintr_attach() and xnintr_detach() must never be
    called when the nklock is held (say, rt_intr_delete() should be
rewritten)...

    but I guess, the better solution is to eliminate the
critical_enter/exit() from rthal_irq_request().

o  there is no need to use cirtical_enter/exit() in xnintr_shirq_*
anymore, the nklock can be used instead.


this would solve one synch. problem in xnintr_* code, though there is
yet another and more complex one I'm banging my head on now :o)

--
Best regards,
Dmitry Adamushko

[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Philippe Gerum]

Dmitry Adamushko wrote:
> Hi,
> 
> the following question/suggestion :
> 
> it could be good to eliminate the use of rthal_critical_enter/exit()
> from rthal_irq_request() if it's not
> strictly necessary.
> 
> 
> the proposal :
> 
> int rthal_irq_request (unsigned irq,
> 		       rthal_irq_handler_t handler,
> 		       rthal_irq_ackfn_t ackfn,
> 		       void *cookie)
> {
> -    unsigned long flags;
>     int err = 0;
> 
>     if (handler == NULL || irq >= IPIPE_NR_IRQS)
> 	return -EINVAL;
> 
> -    flags = rthal_critical_enter(NULL);
> -
> -    if (rthal_irq_handler(&rthal_domain, irq) != NULL)
> -	{
> -	err = -EBUSY;
> -	goto unlock_and_exit;
> -	}
> -
>     err = rthal_virtualize_irq(&rthal_domain,
> 			       irq,
> 			       handler,
> 			       cookie,
> 			       ackfn,
> -			       IPIPE_DYNAMIC_MASK);
> +				IPIPE_DYNAMIC_MASK|IPIPE_EXCLUSIVE_MASK);
> 
> - unlock_and_exit:
> -
> -    rthal_critical_exit(flags);
> -
>     return err;
> }
> 
> IPIPE_EXCLUSIVE_MASK causes a -EBUZY error to be returned by
> ipipe_virtualize_irq() when
> handler != NULL and the current ipd->irqs[irq].handler != NULL.
> 

Sounds reasonable. Using the synchronized inter-processor lock is most 
often a bad idea except in very specific initialization/shutdown stuff 
at Adeos level anyway, and this does introduce deadlock issues at 
Xenomai level when inadvertently using it with the nklock held, so let's 
get rid of this. In the rthal_irq_request() case, it's even useless indeed.

> (IPIPE_EXCLUSIVE_MASK is actually not used at the moment anywere,
> though ipipe_catch_event() is mentioned in comments).
>

The existing flag was aimed to implement self-directed events, but I 
eventually this support differently, so we can recycle it safely.

> Another variant :
> 
> ipipe_virtualize_irq() should always return -EBUZY when handler !=
> NULL and the current ipd->irqs[irq].handler != NULL,
> not taking into account the IPIPE_EXCLUSIVE_FLAG.
> 
>

<snip>

> --
> Best regards,
> Dmitry Adamushko
> 


-- 

Philippe.

[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Dmitry Adamushko]

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

Hi,

I haven't had access to my laptop during this week so the patches
follow only now.

Yet another issue remains that may lead to a deadlock under some circumstances:

- rt_intr_delete() calls xnintr_detach() while holding the "nklock".

- xnintr_detach() (with CONFIG_SMP) spins in xnintr_shirq_spin()
when a corresponding ISR is currently running.

- now this ISR calls any function that uses "nklock" (everything make
use of it) ... Bum!

I first thought about moving xnintr_detach() out of the locked section
in rt_intr_delete() but it somewhat breaks interface consistency ---
e.g. xeno_mark_delete() is called before the object is completely
destroyed.

Another approach would be to introduce a service like
xnintr_synchronize() and enfource the upper interfaces (e.g. skins) to
make use of it in their _delete() methods.

The problem here is that the xnintr_shirq - interface is not complete
and safe without xnintr_shirq_spin() on detaching step and it becomes
somewhat blured with the enforcement like "on SMP systems one should
always call xnintr_synchronize() right after calling xnintr_detach()".

So I'm still thinking how to fix it better...


--
Best regards,
Dmitry Adamushko

[-- Attachment #2: hal.c-irqrequest-noglock.patch --]
[-- Type: application/octet-stream, Size: 779 bytes --]

--- hal-CVS.c	2006-02-28 17:57:22.000000000 +0100
+++ hal.c	2006-04-22 12:09:20.000000000 +0200
@@ -160,30 +160,17 @@ int rthal_irq_request (unsigned irq,
 		       rthal_irq_ackfn_t ackfn,
 		       void *cookie)
 {
-    unsigned long flags;
     int err = 0;
 
     if (handler == NULL || irq >= IPIPE_NR_IRQS)
 	return -EINVAL;
 
-    flags = rthal_critical_enter(NULL);
-
-    if (rthal_irq_handler(&rthal_domain, irq) != NULL)
-	{
-	err = -EBUSY;
-	goto unlock_and_exit;
-	}
-
     err = rthal_virtualize_irq(&rthal_domain,
 			       irq,
 			       handler,
 			       cookie,
 			       ackfn,
-			       IPIPE_DYNAMIC_MASK);
-
- unlock_and_exit:
-
-    rthal_critical_exit(flags);
+			       IPIPE_DYNAMIC_MASK | IPIPE_EXCLUSIVE_MASK);
 
     return err;
 }













[-- Attachment #3: core.c-virtualizeirq.patch --]
[-- Type: application/octet-stream, Size: 400 bytes --]

--- core-CVS.c	2006-02-27 15:10:41.000000000 +0100
+++ core.c	2006-04-22 11:40:43.000000000 +0200
@@ -412,7 +412,7 @@ int ipipe_virtualize_irq(struct ipipe_do
 	} else
 		modemask &=
 		    ~(IPIPE_HANDLE_MASK | IPIPE_STICKY_MASK |
-		      IPIPE_SHARED_MASK);
+		      IPIPE_SHARED_MASK | IPIPE_EXCLUSIVE_MASK);
 
 	if (acknowledge == NULL) {
 		if ((modemask & IPIPE_SHARED_MASK) == 0)













[-- Attachment #4: intr.c-noglock.patch --]
[-- Type: application/octet-stream, Size: 1832 bytes --]

--- intr-CVS.c	2006-03-18 18:29:42.000000000 +0100
+++ intr.c	2006-04-22 11:50:35.000000000 +0200
@@ -576,13 +576,13 @@ static int xnintr_shirq_attach (xnintr_t
 {
     xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
     xnintr_t *prev, **p = &shirq->handlers;
-    unsigned long flags;
     int err = 0;
+    spl_t s;
 
     if (intr->irq >= RTHAL_NR_IRQS)
 	return -EINVAL;
 
-    flags = rthal_critical_enter(NULL);
+    xnlock_get_irqsave(&nklock,s);
 
     if (__testbits(intr->flags,XN_ISR_ATTACHED))
 	{
@@ -637,7 +637,7 @@ static int xnintr_shirq_attach (xnintr_t
 
 unlock_and_exit:
 
-    rthal_critical_exit(flags);
+    xnlock_put_irqrestore(&nklock,s);
     return err;
 }
 
@@ -645,17 +645,17 @@ int xnintr_shirq_detach (xnintr_t *intr)
 {
     xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
     xnintr_t *e, **p = &shirq->handlers;
-    unsigned long flags;
     int err = 0;
+    spl_t s;
 
     if (intr->irq >= RTHAL_NR_IRQS)
 	return -EINVAL;
 
-    flags = rthal_critical_enter(NULL);
+    xnlock_get_irqsave(&nklock,s);
 
     if (!__testbits(intr->flags,XN_ISR_ATTACHED))
 	{
-	rthal_critical_exit(flags);
+	xnlock_put_irqrestore(&nklock,s);
 	return -EPERM;
 	}
 
@@ -672,7 +672,7 @@ int xnintr_shirq_detach (xnintr_t *intr)
 	    if (shirq->handlers == NULL)
 		err = xnarch_release_irq(intr->irq);
 
-	    rthal_critical_exit(flags);
+	    xnlock_put_irqrestore(&nklock,s);
 
 	    /* The idea here is to keep a detached interrupt object valid as long
 	       as the corresponding irq handler is running. This is one of the requirements
@@ -685,7 +685,7 @@ int xnintr_shirq_detach (xnintr_t *intr)
 	p = &e->next;
 	}
 
-    rthal_critical_exit(flags);
+    xnlock_put_irqrestore(&nklock,s);
 
     xnlogerr("attempted to detach a non previously attached interrupt object.\n");
     return err;

[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Philippe Gerum]

Dmitry Adamushko wrote:
> Hi,
> 
> I haven't had access to my laptop during this week so the patches
> follow only now.
>

Merged, thanks.

> Yet another issue remains that may lead to a deadlock under some circumstances:
> 
> - rt_intr_delete() calls xnintr_detach() while holding the "nklock".
>
> - xnintr_detach() (with CONFIG_SMP) spins in xnintr_shirq_spin()
> when a corresponding ISR is currently running.
> 
> - now this ISR calls any function that uses "nklock" (everything make
> use of it) ... Bum!
> 
> I first thought about moving xnintr_detach() out of the locked section
> in rt_intr_delete() but it somewhat breaks interface consistency ---
> e.g. xeno_mark_delete() is called before the object is completely
> destroyed.
>

That's not a problem, the deletion marker will never be anything else 
than a pure magic word stored in some object's descriptor, so we could 
indeed first mark the object as deleted, then release the lock before 
proceeding to the actual deletion.

> Another approach would be to introduce a service like
> xnintr_synchronize() and enfource the upper interfaces (e.g. skins) to
> make use of it in their _delete() methods.

That would be useful too for solving the "concurrent ISR while deleting 
issue", but would not enforce single deletion in the SMP case, I guess.

> 
> The problem here is that the xnintr_shirq - interface is not complete
> and safe without xnintr_shirq_spin() on detaching step and it becomes
> somewhat blured with the enforcement like "on SMP systems one should
> always call xnintr_synchronize() right after calling xnintr_detach()".
> 
> So I'm still thinking how to fix it better...
> 
> 
> --
> Best regards,
> Dmitry Adamushko


-- 

Philippe.

[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Dmitry Adamushko]

> > Another approach would be to introduce a service like
> > xnintr_synchronize() and enfource the upper interfaces (e.g. skins) to
> > make use of it in their _delete() methods.
>
> That would be useful too for solving the "concurrent ISR while deleting
> issue", but would not enforce single deletion in the SMP case, I guess.

I actually want to introduce this call anyway. The nucleus then
provides single xnintr_disabe (nosync) interface +
xnintr_synchronize() and a skin is free to introduce both sync and
nosync versions on its own. Otherwise, there will be the same problem
as with xnintr_detach() - i.e. xnintr_disable (sync) can't be called
from a locked section as it's actually done in rt_intr_delete() (of
course, if we do really need the atomicy in the later one).


> --
>
> Philippe.
>

--
Best regards,
Dmitry Adamushko

[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Dmitry Adamushko]

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

Hello,

this fix prevents possible deadlocks in rt_intr_delete() vs. ISR
handlers that I have mentioned earlier.

--
Best regards,
Dmitry Adamushko

[-- Attachment #2: native-intr.c-delete-rework.patch --]
[-- Type: application/octet-stream, Size: 1172 bytes --]

--- intr-CVS.c	2006-04-27 10:29:16.000000000 +0200
+++ intr.c	2006-04-27 10:36:58.000000000 +0200
@@ -331,33 +331,32 @@ int rt_intr_delete (RT_INTR *intr)
     if (!intr)
         {
         err = xeno_handle_error(intr,XENO_INTR_MAGIC,RT_INTR);
-        goto unlock_and_exit;
+	xnlock_put_irqrestore(&nklock,s);
+	return err;
         }
     
     removeq(&__xeno_intr_q,&intr->link);
 #if defined(__KERNEL__) && defined(CONFIG_XENO_OPT_PERVASIVE)
     rc = xnsynch_destroy(&intr->synch_base);
 #endif /* __KERNEL__ && CONFIG_XENO_OPT_PERVASIVE */
-    xnintr_detach(&intr->intr_base);
 
 #ifdef CONFIG_XENO_OPT_REGISTRY
     if (intr->handle)
         xnregistry_remove(intr->handle);
 #endif /* CONFIG_XENO_OPT_REGISTRY */
 
-    xnintr_destroy(&intr->intr_base);
-
     xeno_mark_deleted(intr);
 
+    xnlock_put_irqrestore(&nklock,s);
+
+    err = xnintr_detach(&intr->intr_base);
+    xnintr_destroy(&intr->intr_base);
+
     if (rc == XNSYNCH_RESCHED)
         /* Some task has been woken up as a result of the deletion:
            reschedule now. */
         xnpod_schedule();
 
- unlock_and_exit:
-
-    xnlock_put_irqrestore(&nklock,s);
-
     return err;
 }
 

[-- Attachment #3: posix-intr.c-delete-rework.patch --]
[-- Type: application/octet-stream, Size: 739 bytes --]

--- intr-CVS.c	2006-04-27 10:39:26.000000000 +0200
+++ intr.c	2006-04-27 10:41:17.000000000 +0200
@@ -174,18 +174,19 @@ int pthread_intr_detach_np (pthread_intr
 #if defined(__KERNEL__) && defined(CONFIG_XENO_OPT_PERVASIVE)
     rc = xnsynch_destroy(&intr->synch_base);
 #endif /* __KERNEL__ && CONFIG_XENO_OPT_PERVASIVE */
-    xnintr_detach(&intr->intr_base);
-
-    xnintr_destroy(&intr->intr_base);
 
     pse51_mark_deleted(intr);
 
     removeq(&pse51_intrq, &intr->link);
 
+    xnlock_put_irqrestore(&nklock, s);
+
+    xnintr_detach(&intr->intr_base);
+    xnintr_destroy(&intr->intr_base);
+
     if (rc == XNSYNCH_RESCHED)
         xnpod_schedule();
 
-    xnlock_put_irqrestore(&nklock, s);
     xnfree(intr);
 
     return 0;

[-- Attachment #4: Changelog-delete-rework.patch --]
[-- Type: application/octet-stream, Size: 465 bytes --]

--- ChangeLog	2006-04-06 22:37:32.000000000 +0200
+++ ChangeLog-new	2006-04-27 10:56:03.000000000 +0200
@@ -1,3 +1,10 @@
+
+2006-04-27  Dmitry Adamushko  <dmitry.adamushko@domain.hid>
+
+	* ksrc/skins/native/intr.c (rt_intr_delete):
+	  ksrc/skins/posix/intr.c (pthread_intr_detach_np): Call xnintr_detach()
+	  with nklock unlocked.
+
 2006-04-06  Rodrigo Rosenfeld Rosas <lbocseg@domain.hid>
 
 	* ksrc/skins/native/snippets/sigxcpu.c (task_body): Fix parameters

[Xenomai-core] Re: [REQUEST] eliminate the rthal_critical_enter/exit() from rthal_irq_request()

[Philippe Gerum]

Dmitry Adamushko wrote:
> Hello,
> 
> this fix prevents possible deadlocks in rt_intr_delete() vs. ISR
> handlers that I have mentioned earlier.

Applied, thanks.

-- 

Philippe.