[Xenomai-core] [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Xenomai-core] [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Dmitry Adamushko]

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

Hi everybody,

I have got a few synch-related problems while adding the code for supporting
the rt shared irqs to
the nucleus layer. But at first let's take a look at some adeos code that,
well, possibly has the
same problem.

let's say the primary domain is interested in irq = TEST_IRQ.

CPU 0:

-  TEST_IRQ occurs.

-  ipipe_handle_irq()    The local interrupts are off on entry.

    testbit(IPIPE_HANDLE_FLAG, ipd->irqs[irq].control) shows whether a given
domain is interested in handling the irq.

    Then cpu-local data is mainly used, e.g. cpudata->irq_hits[irq]++ and
proper changes of pending_lo/hi

    ...

CPU 1:

-  ... -> rthal_irq_release() ->  ipipe_virtualize_irq(TEST_IRQ, ... handler
= NULL, ...)

-  Here, __ipipe_pipe_lock + interrupts off.

    o  ipipe_virtualize_irq() drops the IPIPE_HANDLE_FLAG and sets up
ipd->irqs[irq].handler to NULL.

    First observations, at the same time the TEST_IRQ still can be marked as
pending
    (i.e. some_cpudata->irq_pending_lo/hi and irq_hits)!

CPU 0:

-  actually, ipipe_handle_irq() now (if not yet done before) may be calling
__ipipe_set_irq_bit(ipd,cpuid,irq)
    but noone is interested in this irq == TEST_IRQ already. But no matter,
    the fact is that cpudata->irq_* of a given cpu denotes a pending irq,
let's go further.

-  later on, ipipe_sync_stage() is called for a given domain and cpu.

    It handles all irqs which are marked for the domain and cpu. So it's
based only on the check of
    cpudata->irq_(pending_(hi,lo), hits) fields.
    Let's recall that TEST_IRQ is marked here as well...

    In some way (ipipe_call_root_*irq_handler() or directly
ipd->irqs[irq].handler()) the isr handler is called
    and boom! it's NULL.


Have I missed something that prevents this?

-----

In a sense, the synch. problem I have mentioned at the beginning of this
mail reminds this scenario.

The draft patch is enclosed just to elustrate an idea.

There are 2 problems:

1) we probably don't want to hold any lock while processing the handlers
list (xnintr_intr_handler(): for all shirq->handlers).

Here we may use the approach used by linux in manage.c::free_irq() vs.
handle.c::__do_IRQ() that calls handle_IRQ_event() without the desc->lock
being locked.
The magic is in free_irq() : it removes the "action" item from the list but
then falls into busy waiting until the IRQ_INPROGRESS flag is removed. And
only then it deletes the "action" item.
At that time, the "action" item is already not on the list but still points
to the valid tail of the list so the iteration may be proceeded even if the
current item == "deleted" one.
Blah, just I guess the presupposition here is that the operation of deletion
(free_irq():: *pp = action->next) is atomic.

2) xnintr_irq_handler() gets a cookie which is == xnshared_irq_t* (see
xnintr_attach) that may already be invalid at that time or, and that's a
problem, become invalid during the execution of xnintr_irq_handler().
To prevent that, we could add a flag like IRQ_INPROGRESS but

either we have to set/remove it on the adeos layer before the control is
passed to the xnintr_irq_handler() (to be sure that cookie is not
xnfree()'d. xnintr_detach() will do busy waiting)

or we may set/remove the flag in the xnintr_irq_handler() but have to ignore
the passed "cookie" and
get it as cookie =  rthal_irq_cookie(ipd,irq). Mmm, not very gracefully I'd
say.

Ok, it's enough for the New Year's Eve.

Happy New Year to everybody! I wish you all the best for the New Year :o)


--
Best regards,
Dmitry Adamushko

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

[-- Attachment #2: nucleus_intr.c.patch --]
[-- Type: application/octet-stream, Size: 3234 bytes --]

--- intr.c-old	2005-11-21 21:53:11.000000000 +0100
+++ intr.c	2005-12-31 11:31:27.000000000 +0100
@@ -41,6 +41,13 @@ xnintr_t nkclock;
 static void xnintr_irq_handler(unsigned irq,
 			       void *cookie);
 
+typedef struct xnshared_irq {
+
+    xnqueue_t handlers;
+
+} xnshared_irq_t;
+
+
 /*! 
  * \fn int xnintr_init (xnintr_t *intr,unsigned irq,xnisr_t isr,xniack_t iack,xnflags_t flags)
  * \brief Initialize an interrupt object.
@@ -130,6 +137,7 @@ int xnintr_init (xnintr_t *intr,
     intr->iack = iack;
     intr->cookie = NULL;
     intr->hits = 0;
+    inith(&intr->link);
 
     return 0;
 }
@@ -204,11 +212,66 @@ int xnintr_destroy (xnintr_t *intr)
 int xnintr_attach (xnintr_t *intr,
 		   void *cookie)
 {
+    rthal_irq_handler_t irq_handler;
+    unsigned irq = intr->irq;
+    xnshared_irq_t *shirq;
+    int err = 0;
+    spl_t s;
+
     intr->hits = 0;
     intr->cookie = cookie;
-    return xnarch_hook_irq(intr->irq,&xnintr_irq_handler,intr->iack,intr);
+
+    xnlock_get_irqsave(&nklock,s);
+
+    irq_handler = rthal_irq_handler(&rthal_domain,irq);
+    
+    if (irq_handler)
+	{
+	xnintr_t *old;
+	shirq = rthal_irq_cookie(&rthal_domain,irq);
+	
+	if (irq_handler != &xnintr_irq_handler)
+	    {
+	    err = -EBUSY;
+	    goto unlock_and_exit;
+	    }
+	
+	old = link2intr(getheadq(&shirq->handlers));
+
+	if (!(old->flags & intr->flags & XNISR_SHARED_MASK))
+	    {
+	    err = -EBUSY;
+	    goto unlock_and_exit;
+	    }
+	
+	appendq(&shirq->handlers,&intr->link);
+	}
+    else
+	{
+	shirq = xnmalloc(sizeof(*shirq));
+	
+	if (!shirq)
+	    {
+	    err = -ENOMEM;
+	    goto unlock_and_exit;
+	    }
+
+	initq(&shirq->handlers);
+	appendq(&shirq->handlers,&intr->link);
+
+        err = xnarch_hook_irq(irq,&xnintr_irq_handler,intr->iack,shirq);
+	
+	if (err)
+	    xnfree(shirq);
+	}
+
+unlock_and_exit:
+    
+    xnlock_put_irqrestore(&nklock,s);
+    return err;
 }
 
+
 /*! 
  * \fn int xnintr_detach (xnintr_t *intr)
  * \brief Detach an interrupt object.
@@ -241,7 +304,26 @@ int xnintr_attach (xnintr_t *intr,
 int xnintr_detach (xnintr_t *intr)
 
 {
-    return xnarch_release_irq(intr->irq);
+    unsigned irq = intr->irq;
+    xnshared_irq_t *shirq;
+    int err = 0;
+    spl_t s;
+
+    xnlock_get_irqsave(&nklock,s);
+
+    shirq = rthal_irq_cookie(&rthal_domain,irq);
+
+    removeq(&shirq->handlers,&intr->link);
+    
+    if (!countq(&shirq->handlers))
+	{
+	err = xnarch_release_irq(irq);
+	xnfree(shirq);
+	}
+
+    xnlock_put_irqrestore(&nklock,s);
+
+    return err;
 }
 
 /*! 
@@ -353,17 +435,28 @@ static void xnintr_irq_handler (unsigned
 
 {
     xnsched_t *sched = xnpod_current_sched();
-    xnintr_t *intr = (xnintr_t *)cookie;
-    int s;
+    xnshared_irq_t *shirq = (xnshared_irq_t *)cookie;
+    xnholder_t holder;
+    int s = 0;
 
     xnarch_memory_barrier();
 
     xnltt_log_event(xeno_ev_ienter,irq);
 
     ++sched->inesting;
-    s = intr->isr(intr);
+    
+    holder = getheadq(&shirq->handlers);
+    
+    while (holder)
+	{
+	xnintr_t *intr = link2intr(holder);
+	holder = nextq(&shirq->handlers,holder);
+
+	s |= intr->isr(intr);
+	++intr->hits;
+	}
+    
     --sched->inesting;
-    ++intr->hits;
 
     if (s & XN_ISR_ENABLE)
 	xnarch_enable_irq(irq);

[-- Attachment #3: nucleus_intr.h.patch --]
[-- Type: application/octet-stream, Size: 776 bytes --]

--- intr.h-old	2005-11-21 21:52:40.000000000 +0100
+++ intr.h	2005-12-31 13:53:38.000000000 +0100
@@ -32,18 +32,26 @@ struct xnintr;
 
 typedef struct xnintr {
 
-    unsigned irq;	/* !< IRQ number. */
+    xnholder_t link;
 
+#define link2intr(laddr) \
+((xnintr_t *)(((char *)laddr) - (int)(&((xnintr_t *)0)->link)))
+    
     xnisr_t isr;	/* !< Interrupt service routine. */
 
-    xniack_t iack;	/* !< Interrupt acknowledge routine. */
-
     unsigned long hits;	/* !< Number of receipts (since attachment). */
 
+    unsigned long flags;
+
+    unsigned irq;	/* !< IRQ number. */
+    
     void *cookie;	/* !< User-defined cookie value. */
 
+    xniack_t iack;	/* !< Interrupt acknowledge routine. */
+
 } xnintr_t;
 
+
 extern xnintr_t nkclock;
 
 #ifdef __cplusplus

[Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Philippe Gerum]

Dmitry Adamushko wrote:
> 
> Hi everybody,
> 
> I have got a few synch-related problems while adding the code for 
> supporting the rt shared irqs to
> the nucleus layer. But at first let's take a look at some adeos code 
> that, well, possibly has the
> same problem.
> 
> let's say the primary domain is interested in irq = TEST_IRQ.
> 
> CPU 0:
> 
> -  TEST_IRQ occurs.
> 
> -  ipipe_handle_irq()    The local interrupts are off on entry.
> 
>     testbit(IPIPE_HANDLE_FLAG, ipd->irqs[irq].control) shows whether a 
> given domain is interested in handling the irq.
> 
>     Then cpu-local data is mainly used, e.g. cpudata->irq_hits[irq]++ 
> and proper changes of pending_lo/hi
>    
>     ...
>    
> CPU 1:
> 
> -  ... -> rthal_irq_release() ->  ipipe_virtualize_irq(TEST_IRQ, ... 
> handler = NULL, ...)
> 
> -  Here, __ipipe_pipe_lock + interrupts off.
> 
>     o  ipipe_virtualize_irq() drops the IPIPE_HANDLE_FLAG and sets up 
> ipd->irqs[irq].handler to NULL.
>    
>     First observations, at the same time the TEST_IRQ still can be 
> marked as pending
>     (i.e. some_cpudata->irq_pending_lo/hi and irq_hits)!
> 
> CPU 0:
> 
> -  actually, ipipe_handle_irq() now (if not yet done before) may be 
> calling __ipipe_set_irq_bit(ipd,cpuid,irq)
>     but noone is interested in this irq == TEST_IRQ already. But no matter,
>     the fact is that cpudata->irq_* of a given cpu denotes a pending 
> irq, let's go further.
> 
> -  later on, ipipe_sync_stage() is called for a given domain and cpu.
> 
>     It handles all irqs which are marked for the domain and cpu. So it's 
> based only on the check of
>     cpudata->irq_(pending_(hi,lo), hits) fields.
>     Let's recall that TEST_IRQ is marked here as well...
>    
>     In some way (ipipe_call_root_*irq_handler() or directly 
> ipd->irqs[irq].handler()) the isr handler is called
>     and boom! it's NULL.
>    
> 
> Have I missed something that prevents this?
> 

Nope, good spot, that could indeed happen in SMP configs. The code is 
expected to shut the given interrupt source _before_ calling 
rthal_irq_release(). But, the root of the problem is that 
rthal_irq_release() doesn't make sure that all _pending_ IRQs from the 
given kind are synchronized before processing. We need the equivalent of 
Linux's synchronize_irq() here, and I would tend to implement this in 
the Adeos layer directly, in ipipe_virtualize_irq() for the NULL handler 
case, since it's a matter of general consistency.

> -----
> 
> In a sense, the synch. problem I have mentioned at the beginning of this 
> mail reminds this scenario.
> 
> The draft patch is enclosed just to elustrate an idea.
> 
> There are 2 problems:
> 
> 1) we probably don't want to hold any lock while processing the handlers 
> list (xnintr_intr_handler(): for all shirq->handlers).
> 
> Here we may use the approach used by linux in manage.c::free_irq() vs. 
> handle.c::__do_IRQ() that calls handle_IRQ_event() without the 
> desc->lock being locked.
> The magic is in free_irq() : it removes the "action" item from the list 
> but then falls into busy waiting until the IRQ_INPROGRESS flag is 
> removed. And only then it deletes the "action" item.
> At that time, the "action" item is already not on the list but still 
> points to the valid tail of the list so the iteration may be proceeded 
> even if the current item == "deleted" one.
> Blah, just I guess the presupposition here is that the operation of 
> deletion (free_irq():: *pp = action->next) is atomic.
> 
> 2) xnintr_irq_handler() gets a cookie which is == xnshared_irq_t* (see 
> xnintr_attach) that may already be invalid at that time or, and that's a 
> problem, become invalid during the execution of xnintr_irq_handler().
> To prevent that, we could add a flag like IRQ_INPROGRESS but
> 
> either we have to set/remove it on the adeos layer before the control is 
> passed to the xnintr_irq_handler() (to be sure that cookie is not 
> xnfree()'d. xnintr_detach() will do busy waiting)
>

Synchronizing the pending IRQs in ipipe_virtualize_irq() should be done 
by polling the proper irq_pending count (and _not_ the hi/lo bits which 
get cleared before the handler is run). The prerequisite is to call 
ipipe_virtualize_irq() for an unstalled domain, or at least forcibly 
unstalling it there. I would see something along these lines, which is 
already used to drain the pending IRQs before unlinking a domain from 
the pipeline:

spin_unlock_irqrestore_hw(&__ipipe_pipelock, flags);

ipipe_unstall_pipeline_from(ipd);

clear_bit(IPIPE_HANDLE_FLAG, &ipd->irqs[irq].control);
clear_bit(IPIPE_STICKY_FLAG, &ipd->irqs[irq].control);
set_bit(IPIPE_PASS_FLAG, &ipd->irqs[irq].control);
/* or tweak the modemask directly */

for (_cpuid = 0; _cpuid < nr_cpus; _cpuid++)
	while (ipd->cpudata[_cpuid].irq_hits[irq] > 0)
		cpu_relax();

spin_lock_irqsave_hw(&__ipipe_pipelock, flags);

> or we may set/remove the flag in the xnintr_irq_handler() but have to 
> ignore the passed "cookie" and
> get it as cookie =  rthal_irq_cookie(ipd,irq). Mmm, not very gracefully 
> I'd say.

Eeek...

> 
> Ok, it's enough for the New Year's Eve.
> 
> Happy New Year to everybody! I wish you all the best for the New Year :o)
>

Best wishes too!

Cheers,

> 
> -- 
> Best regards,
> Dmitry Adamushko
> 
> 


-- 

Philippe.

[Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Dmitry Adamushko]

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

>
> Nope, good spot, that could indeed happen in SMP configs. The code is
> expected to shut the given interrupt source _before_ calling
> rthal_irq_release(). But, the root of the problem is that
> rthal_irq_release() doesn't make sure that all _pending_ IRQs from the
> given kind are synchronized before processing. We need the equivalent of
> Linux's synchronize_irq() here, and I would tend to implement this in
> the Adeos layer directly, in ipipe_virtualize_irq() for the NULL handler
> case, since it's a matter of general consistency.


Yep, I thought about the analogue of Linux's synchronize_irq() too.
I guess, what we need to provide is the guarantee that the "cookie" argument
(as well as a "handler") remains valid as long as there is a given irq
handler in execution.

i.e.

o  ipipe_virtualize_irq(THIS_IRQ, handler=NULL, cookie=NULL) call _removes_
the IPIPE_HANDLE_FLAG so that new occurences of the THIS_IRQ is no more
accepted for the given domain;

o  then ipipe_virtualize_irq is buzy waiting (ipipe_synchronize_irq() or
smth like this) until the active ipd->irqs[THIS_IRQ].handler() is completed.
If there is not currently active handler we may drop all
cpudata->irq_pending_hits for THIS_IRQ to zero. But ipipe_sync_stage()
should check for IPIPE_HANDLE_FLAG then since this is a sign that a given
irq is no more handled (even if there are some pending ones).

o  as a final step, ipd->irqs[THIS_IRQ].handler/cookie are set to zero.

Ok, repeating myself, the main idea is to keep the "cookie" unchanged as
long as the corresponding handler is active. This would partialy resolve the
sync. problem I have got on the nucleus layer.

err... I have to orginize some issues next few days so I'll be out of my
notebook. Then I'll finilize the ipipe-irq-related patch so to be ready for
the .01 version.

--
Best regards,
Dmitry Adamushko

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

Re: [Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Philippe Gerum]

Dmitry Adamushko wrote:
> 
> err... I have to orginize some issues next few days so I'll be out of my 
> notebook. Then I'll finilize the ipipe-irq-related patch so to be ready 
> for the .01 version.
>

Meanwhile, I have finished merging the shared IRQ infrastructure into 
the Adeos codebase for all supported archs. The Xenomai trunk/ has been 
upgraded accordingly too. x86-wise, please make sure to rebase your 
development on 1.1-02.

-- 

Philippe.

[Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Dmitry Adamushko]

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

On 05/01/06, Philippe Gerum <rpm@xenomai.org> wrote:
>
> Dmitry Adamushko wrote:
> >
> > err... I have to orginize some issues next few days so I'll be out of my
> > notebook. Then I'll finilize the ipipe-irq-related patch so to be ready
> > for the .01 version.
> >
>
> Meanwhile, I have finished merging the shared IRQ infrastructure into
> the Adeos codebase for all supported archs. The Xenomai trunk/ has been
> upgraded accordingly too.


Thanks and sorry for aditionally taking your time on some work I shoud have
completed on my own.
Actually this infrastructure is by no means a _must_ for introducing the
shared irq feature on the nucleus layer.

I doubt that we may gain any noticeable benefits, latency-wise, having got
the one-layer-less irq processing chain. Just hope there is no regression.

So, my hope is rather that the new interface has become a bit more clear and
usefull, e.g. there is no need to implement any other per-irq tables to
introduce the "cookie" concept no matter where it has to be used (even
directly at the adeos layer).



x86-wise, please make sure to rebase your
> development on 1.1-02.


Sure.


--
>
> Philippe.
>



--
Best regards,
Dmitry Adamushko

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

[Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Dmitry Adamushko]

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

On 31/12/05, Philippe Gerum <rpm@xenomai.org> wrote:
> ...

> 2) xnintr_irq_handler() gets a cookie which is == xnshared_irq_t* (see
> > xnintr_attach) that may already be invalid at that time or, and that's a
> > problem, become invalid during the execution of xnintr_irq_handler().
> > To prevent that, we could add a flag like IRQ_INPROGRESS but
> >
> > either we have to set/remove it on the adeos layer before the control is
> > passed to the xnintr_irq_handler() (to be sure that cookie is not
> > xnfree()'d. xnintr_detach() will do busy waiting)
> >
>
> Synchronizing the pending IRQs in ipipe_virtualize_irq() should be done
> by polling the proper irq_pending count (and _not_ the hi/lo bits which
> get cleared before the handler is run).


But irq_pending counter (pending_hits in the recent adeos patch) gets
cleared before running the isr handler too.

In ipipe_sync_stage() :
...
            if (--cpudata->irq_pending_hits[irq] == 0) {
                 __clear_bit(rank,
                         &cpudata->irq_pending_lo[level]);
                 ipipe_mark_irq_delivery(ipd,irq,cpuid);
            }
(*)

// run the corresponding isr handler

This code drops the irq_pending_hits to 0 before running the handler.

Doesn't it make the following code wrong ?

for (_cpuid = 0; _cpuid < nr_cpus; _cpuid++)
>         while (ipd->cpudata[_cpuid].irq_pending_hits[irq] > 0)
>                 cpu_relax();


as the handler still can be running / or even about to be run when
ipd->cpudata[_cpuid].irq_pending_hits[irq] is already 0.

We can remove the (*) code at the end of ipipe_sync_stage() so that
irq_pending_hits[irq] may become 0 only after the corresponding isr handler
has completed.

At the same time, the irq_pending counter can't be increased anymore since
we clear the IPIPE_HANDLE_FLAG before doing the busy waiting in
ipipe_unregister_domain() or ipipe_virtualize_irq().


--
>
> Philippe.
>

--
Best regards,
Dmitry Adamushko

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

[Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Philippe Gerum]

Dmitry Adamushko wrote:
> 
> 
> On 31/12/05, *Philippe Gerum* <rpm@xenomai.org <mailto:rpm@xenomai.org>> 
> wrote:
> 
>  > ...
> 
>      > 2) xnintr_irq_handler() gets a cookie which is == xnshared_irq_t*
>     (see
>      > xnintr_attach) that may already be invalid at that time or, and
>     that's a
>      > problem, become invalid during the execution of xnintr_irq_handler().
>      > To prevent that, we could add a flag like IRQ_INPROGRESS but
>      >
>      > either we have to set/remove it on the adeos layer before the
>     control is
>      > passed to the xnintr_irq_handler() (to be sure that cookie is not
>      > xnfree()'d. xnintr_detach() will do busy waiting)
>      >
> 
>     Synchronizing the pending IRQs in ipipe_virtualize_irq() should be done
>     by polling the proper irq_pending count (and _not_ the hi/lo bits which
>     get cleared before the handler is run). 
> 
> 
> But irq_pending counter (pending_hits in the recent adeos patch) gets 
> cleared before running the isr handler too.
>

I was referring to __ipipe_lock_irq() which clears the pending bits but 
keeps the irq count intact to emulate a PIC masking for a given interrupt.

> In ipipe_sync_stage() :
> ...
>             if (--cpudata->irq_pending_hits[irq] == 0) {
>                  __clear_bit(rank,
>                          &cpudata->irq_pending_lo[level]);
>                  ipipe_mark_irq_delivery(ipd,irq,cpuid);
>             }
> (*)
> 
> // run the corresponding isr handler
> 
> This code drops the irq_pending_hits to 0 before running the handler.
>

> Doesn't it make the following code wrong ?
> 
>     for (_cpuid = 0; _cpuid < nr_cpus; _cpuid++)
>             while (ipd->cpudata[_cpuid].irq_pending_hits[irq] > 0)
>                     cpu_relax();
> 
> 
> as the handler still can be running / or even about to be run when 
> ipd->cpudata[_cpuid].irq_pending_hits[irq] is already 0.
> 
> We can remove the (*) code at the end of ipipe_sync_stage() so that 
> irq_pending_hits[irq] may become 0 only after the corresponding isr 
> handler has completed.
> 

I don't like the idea of having the interrupt bookkeeping stuff 
incompletely updated while calling a handler that might sleep and/or 
migrate to another CPU.

Another option would be to check for SYNC flag; the syncer sets it 
before entering the dispatch loop.

> At the same time, the irq_pending counter can't be increased anymore 
> since we clear the IPIPE_HANDLE_FLAG before doing the busy waiting in 
> ipipe_unregister_domain() or ipipe_virtualize_irq().
>  
> 
>     --
> 
>     Philippe.
> 
> 
> -- 
> Best regards,
> Dmitry Adamushko
> 


-- 

Philippe.

[Xenomai-core] Re: [PATCH, RFC] nucleus:shared irq and possible ipipe problem

[Dmitry Adamushko]

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

On 08/01/06, Philippe Gerum <rpm@xenomai.org> wrote:
>
> > [skiped]

> In ipipe_sync_stage() :

> > ...
> >             if (--cpudata->irq_pending_hits[irq] == 0) {
> >                  __clear_bit(rank,
> >                          &cpudata->irq_pending_lo[level]);
> >                  ipipe_mark_irq_delivery(ipd,irq,cpuid);
> >             }
> > (*)
> >
> > // run the corresponding isr handler
> >
> > This code drops the irq_pending_hits to 0 before running the handler.
> >
>
> > Doesn't it make the following code wrong ?
> >
> >     for (_cpuid = 0; _cpuid < nr_cpus; _cpuid++)
> >             while (ipd->cpudata[_cpuid].irq_pending_hits[irq] > 0)
> >                     cpu_relax();
> >
> >
> > as the handler still can be running / or even about to be run when
> > ipd->cpudata[_cpuid].irq_pending_hits[irq] is already 0.
> >
> > We can remove the (*) code at the end of ipipe_sync_stage() so that
> > irq_pending_hits[irq] may become 0 only after the corresponding isr
> > handler has completed.
> >
>
> I don't like the idea of having the interrupt bookkeeping stuff
> incompletely updated while calling a handler that might sleep and/or
> migrate to another CPU.
>
> Another option would be to check for SYNC flag; the syncer sets it
> before entering the dispatch loop.


It's unset for all domains but root when the corresponding irq handler is
running.

ipipe_sync_stage() :
...
            __clear_bit(IPIPE_SYNC_FLAG, &cpudata->status);
            ipd->irqs[irq].handler(irq, ipd->irqs[irq].cookie);
            __set_bit(IPIPE_SYNC_FLAG, &cpudata->status);

So it doesn't help here as it is used now.

Ok, so far I have built all synchronization completely on the nucleus layer,
provided that the "cookie" param is not used as it is passed to the handler
but is taken via the rthal_irq_cookie() service being in a protected
section.

The disadvantage so far is that the recently extended adeos irq interface is
not used as it is supposed to be (the "cookie" argument of the handler).

I'll post the current version of the patch in the separate mail.

--
>
> Philippe.
>



--
Best regards,
Dmitry Adamushko

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