[Xenomai-core] [RFC][PATCH] shirq locking rework

[Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Hi Dmitry,

I had a look at the shared IRQ thing /wrt to my idea of switching
between two list revisions. It turned out to be as nice as I expected
when marvelling at the new xnintr_edge_shirq_handler() -- but it became
horribly complex for setup/cleanup :(. So forget it, also the ring idea
which took quite some part in the complexity increase.

Looking at xnintr_shirq_lock again: We have an atomic op, we have a
memory barrier -- what do we actually gain by not using a usual xnlock
here? I mean one that is per-IRQ, embedded in xnintr_shirq_t?

Well, I hate nested locks when it comes to real-time, but in this case I
really found no simpler approach. There is the risk of deadlocks via

IRQ:		xnintr_shirq::lock -> handler -> nklock vs.
Application:	nklock -> xnintr_attach/detach -> xnintr_shirq::lock,

but we have no such case in-tree, nor should anyone try to do so
elsewhere I assume (restriction is documented now). Note that this code
is only compile-tested, and my brain still suffers from serious knots
due to the first approach, thus no guarantee that I didn't messed
things up.

Comments?

Jan


---
 ksrc/nucleus/intr.c |   70 +++++++++++++++-------------------------------------
 1 file changed, 21 insertions(+), 49 deletions(-)

Index: xenomai/ksrc/nucleus/intr.c
===================================================================
--- xenomai.orig/ksrc/nucleus/intr.c
+++ xenomai/ksrc/nucleus/intr.c
@@ -169,40 +169,12 @@ typedef struct xnintr_shirq {
 
 	xnintr_t *handlers;
 	int unhandled;
-#ifdef CONFIG_SMP
-	atomic_counter_t active;
-#endif
+	DECLARE_XNLOCK(lock);
 
 } xnintr_shirq_t;
 
 static xnintr_shirq_t xnshirqs[RTHAL_NR_IRQS];
 
-static inline void xnintr_shirq_lock(xnintr_shirq_t *shirq)
-{
-#ifdef CONFIG_SMP
-	xnarch_atomic_inc(&shirq->active);
-	xnarch_memory_barrier();
-#endif
-}
-
-static inline void xnintr_shirq_unlock(xnintr_shirq_t *shirq)
-{
-#ifdef CONFIG_SMP
-	xnarch_memory_barrier();
-	xnarch_atomic_dec(&shirq->active);
-#endif
-}
-
-void xnintr_synchronize(xnintr_t *intr)
-{
-#ifdef CONFIG_SMP
-	xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
-
-	while (xnarch_atomic_get(&shirq->active))
-		cpu_relax();
-#endif
-}
-
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
 /*
  * Low-level interrupt handler dispatching the user-defined ISRs for
@@ -226,7 +198,7 @@ static void xnintr_shirq_handler(unsigne
 
 	++sched->inesting;
 
-	xnintr_shirq_lock(shirq);
+	xnlock_get(&shirq->lock);
 	intr = shirq->handlers;
 
 	while (intr) {
@@ -247,7 +219,7 @@ static void xnintr_shirq_handler(unsigne
 		intr = intr->next;
 	}
 
-	xnintr_shirq_unlock(shirq);
+	xnlock_put(&shirq->lock);
 
 	if (unlikely(s == XN_ISR_NONE)) {
 		if (++shirq->unhandled == XNINTR_MAX_UNHANDLED) {
@@ -297,7 +269,7 @@ static void xnintr_edge_shirq_handler(un
 
 	++sched->inesting;
 
-	xnintr_shirq_lock(shirq);
+	xnlock_get(&shirq->lock);
 	intr = shirq->handlers;
 
 	while (intr != end) {
@@ -328,7 +300,7 @@ static void xnintr_edge_shirq_handler(un
 			intr = shirq->handlers;
 	}
 
-	xnintr_shirq_unlock(shirq);
+	xnlock_put(&shirq->lock);
 
 	if (counter > MAX_EDGEIRQ_COUNTER)
 		xnlogerr
@@ -411,9 +383,12 @@ static inline int xnintr_irq_attach(xnin
 
 	__setbits(intr->flags, XN_ISR_ATTACHED);
 
-	/* Add a given interrupt object. */
 	intr->next = NULL;
+
+	/* Add a given interrupt object. */
+	xnlock_get(&shirq->lock);
 	*p = intr;
+	xnlock_put(&shirq->lock);
 
 	return 0;
 }
@@ -434,8 +409,10 @@ static inline int xnintr_irq_detach(xnin
 
 	while ((e = *p) != NULL) {
 		if (e == intr) {
-			/* Remove a given interrupt object from the list. */
+			/* Remove the given interrupt object from the list. */
+			xnlock_get(&shirq->lock);
 			*p = e->next;
+			xnlock_put(&shirq->lock);
 
 			/* Release the IRQ line if this was the last user */
 			if (shirq->handlers == NULL)
@@ -454,12 +431,8 @@ static inline int xnintr_irq_detach(xnin
 int xnintr_mount(void)
 {
 	int i;
-	for (i = 0; i < RTHAL_NR_IRQS; ++i) {
-		xnshirqs[i].handlers = NULL;
-#ifdef CONFIG_SMP
-		xnarch_atomic_set(&xnshirqs[i].active, 0);
-#endif
-	}
+	for (i = 0; i < RTHAL_NR_IRQS; ++i)
+		xnlock_init(&xnshirqs[i].lock);
 	return 0;
 }
 
@@ -475,7 +448,6 @@ static inline int xnintr_irq_detach(xnin
 	return xnarch_release_irq(intr->irq);
 }
 
-void xnintr_synchronize(xnintr_t *intr) {}
 int xnintr_mount(void) { return 0; }
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
@@ -651,6 +623,9 @@ int xnintr_destroy(xnintr_t *intr)
  * a low-level error occurred while attaching the interrupt. -EBUSY is
  * specifically returned if the interrupt object was already attached.
  *
+ * @note The caller <b>must not</b> hold nklock when invoking this service,
+ * this would cause deadlocks.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -682,7 +657,7 @@ int xnintr_attach(xnintr_t *intr, void *
 
 	if (!err)
 		xnintr_stat_counter_inc();
-	
+
 	xnlock_put_irqrestore(&intrlock, s);
 
 	return err;
@@ -706,6 +681,9 @@ int xnintr_attach(xnintr_t *intr, void *
  * a non-attached interrupt object leads to a null-effect and returns
  * 0.
  *
+ * @note The caller <b>must not</b> hold nklock when invoking this service,
+ * this would cause deadlocks.
+ *
  * Environments:
  *
  * This service can be called from:
@@ -731,12 +709,6 @@ int xnintr_detach(xnintr_t *intr)
 
 	xnlock_put_irqrestore(&intrlock, 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 to iterate over the xnintr_shirq_t::handlers list in
-	   xnintr_irq_handler() in a lockless way. */
-	xnintr_synchronize(intr);
-
 	return err;
 }
 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

Hello Jan,

> Well, I hate nested locks when it comes to real-time, but in this case I
> really found no simpler approach. There is the risk of deadlocks via
>
> IRQ:            xnintr_shirq::lock -> handler -> nklock vs.
> Application:    nklock -> xnintr_attach/detach -> xnintr_shirq::lock,

it's also relevant for the current code - xnintr_attach/detach() must
not be called while holding the 'nklock'.

I think, your approach should work as well.. provided we have only a
single reader vs. a single writter contention case, which seems to be
the case here ('intrlock' is responsible for synchronization between
xnintr_attach/detach()).. your approach does increase a worst case
length of the lock(&intrlock) --> unlock(&intrlock) section... but
that's unlikely to be critical.

I think, the patch I sent before addresses a reported earlier case
with xnintr_edge_shirq_handler().. but your approach does make code
shorter (and likely simpler), right? I don't see any problems it would
possibly cause (maybe I'm missing smth yet :)


-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> Hello Jan,
> 
>> Well, I hate nested locks when it comes to real-time, but in this case I
>> really found no simpler approach. There is the risk of deadlocks via
>>
>> IRQ:            xnintr_shirq::lock -> handler -> nklock vs.
>> Application:    nklock -> xnintr_attach/detach -> xnintr_shirq::lock,
> 
> it's also relevant for the current code - xnintr_attach/detach() must
> not be called while holding the 'nklock'.

That's good, no new restriction (the existing one will be documented now).

> 
> I think, your approach should work as well.. provided we have only a
> single reader vs. a single writter contention case, which seems to be
> the case here ('intrlock' is responsible for synchronization between

Single writer is ensured by intrlock, single reader comes from the
per-IRQ scope of the inner lock.

> xnintr_attach/detach()).. your approach does increase a worst case
> length of the lock(&intrlock) --> unlock(&intrlock) section... but
> that's unlikely to be critical.
> 
> I think, the patch I sent before addresses a reported earlier case
> with xnintr_edge_shirq_handler().. but your approach does make code
> shorter (and likely simpler), right? I don't see any problems it would
> possibly cause (maybe I'm missing smth yet :)

I must confess I didn't get your idea immediately. Later on (cough,
after hacking my own patch, cough) I had a closer look, and it first
appeared fairly nice, despite the additional "ifs". But then I realised
that "end == old_end" may produce false positives in case we have
several times the same (and only the same) IRQ in a row. So, I'm afraid
we have to live with only one candidate. :->

OK, will point our bug reporter to that patch now and ask for testing.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

On 20/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> [ ... ]
> > xnintr_attach/detach()).. your approach does increase a worst case
> > length of the lock(&intrlock) --> unlock(&intrlock) section... but
> > that's unlikely to be critical.
> >
> > I think, the patch I sent before addresses a reported earlier case
> > with xnintr_edge_shirq_handler().. but your approach does make code
> > shorter (and likely simpler), right? I don't see any problems it would
> > possibly cause (maybe I'm missing smth yet :)
>
> I must confess I didn't get your idea immediately. Later on (cough,
> after hacking my own patch, cough) I had a closer look, and it first
> appeared fairly nice, despite the additional "ifs". But then I realised
> that "end == old_end" may produce false positives in case we have
> several times the same (and only the same) IRQ in a row.
> So, I'm afraid
> we have to live with only one candidate. :->

It's not clear, can you elaborate your (non-working) scenario in more
details? :-)

Note: I sent the second patch with the following correction :

...
                } else if (code == XN_ISR_NONE && end == NULL) {
                        end = intr;
+                        old_end = NULL;
                }
...

I don't see why this idea can't work (even if I made yet another error).
Under some circumstances the following code will be triggered to end a
loop even when 'end' is still valid

                        if (end && old_end == end)
                                intr = NULL;

_but_ it'd be exactly a case when "intr == end" and hence, the loop
would be finished anyway by the "while (intr && intr != end)"
condition.. So sometimes it acts as yet _another_ check to exit the
loop, IOW "while (intr && intr != end)" may be converted to just
"while (intr)".


>
> Jan
>

-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 20/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> [ ... ]
>> > xnintr_attach/detach()).. your approach does increase a worst case
>> > length of the lock(&intrlock) --> unlock(&intrlock) section... but
>> > that's unlikely to be critical.
>> >
>> > I think, the patch I sent before addresses a reported earlier case
>> > with xnintr_edge_shirq_handler().. but your approach does make code
>> > shorter (and likely simpler), right? I don't see any problems it would
>> > possibly cause (maybe I'm missing smth yet :)
>>
>> I must confess I didn't get your idea immediately. Later on (cough,
>> after hacking my own patch, cough) I had a closer look, and it first
>> appeared fairly nice, despite the additional "ifs". But then I realised
>> that "end == old_end" may produce false positives in case we have
>> several times the same (and only the same) IRQ in a row.
>> So, I'm afraid
>> we have to live with only one candidate. :->
> 
> It's not clear, can you elaborate your (non-working) scenario in more
> details? :-)
> 
> Note: I sent the second patch with the following correction :
> 
> ...
>                } else if (code == XN_ISR_NONE && end == NULL) {
>                        end = intr;
> +                        old_end = NULL;
>                }
> ...
> 
> I don't see why this idea can't work (even if I made yet another error).
> Under some circumstances the following code will be triggered to end a
> loop even when 'end' is still valid
> 
>                        if (end && old_end == end)
>                                intr = NULL;
> 
> _but_ it'd be exactly a case when "intr == end" and hence, the loop
> would be finished anyway by the "while (intr && intr != end)"
> condition.. So sometimes it acts as yet _another_ check to exit the
> loop, IOW "while (intr && intr != end)" may be converted to just
> "while (intr)".

Yeah, looks like you are right again, should work as well.


Unfortunately, that whole thing make me meditate about the whole issue
again. And now I wonder why we make such a fuss about locking for shared
IRQs (where it should be correct with any of the new patches) while we
do not care about the non-shared, standard scenario. I currently find no
reason why we shouldn't race when some non-shared IRQ pops up on one CPU
and deregistration takes place on another. Also in this case, the xnintr
object must remain valid for the whole handler execution time. Do we need

struct xnintr_irq {
	xnintr_t *handler;
	<whatever-lock>;
} xnirqs[RTHAL_NR_IRQS];

unconditionally? Or should we better move the whole locking thing into
i-pipe somehow? Argh.

Well, and I wonder what this xnarch_memory_barrier() at each handler
entry is for. Seems to be there for ages, don't see why right now. (The
kernel has a golden rule for this: no barrier without comments!)

Sigh, the never ending IRQ story...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

On 21/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> [ ... ]
>
> Unfortunately, that whole thing make me meditate about the whole issue
> again. And now I wonder why we make such a fuss about locking for shared
> IRQs (where it should be correct with any of the new patches) while we
> do not care about the non-shared, standard scenario.

surprise-surprise.. sure, one way or another it must be fixed. heh..
too many bugs (and I don't even want to say who's responsible) :-/

> Sigh, the never ending IRQ story...

should be reviewed.


>
> Jan
>

-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 21/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> [ ... ]
>>
>> Unfortunately, that whole thing make me meditate about the whole issue
>> again. And now I wonder why we make such a fuss about locking for shared
>> IRQs (where it should be correct with any of the new patches) while we
>> do not care about the non-shared, standard scenario.
> 
> surprise-surprise.. sure, one way or another it must be fixed. heh..
> too many bugs (and I don't even want to say who's responsible) :-/

I wouldn't accept all the responsibility if I were you. We all went
through this code several times. It's a sign that the design might be
too complex now, and I feel like having some share in this.

> 
>> Sigh, the never ending IRQ story...
> 
> should be reviewed.
> 

Things get worse, at least with XENO_OPT_STATS: Due to the runtime
statistics collection, we may end up with dangling references to our
xnintr object even after xnintr_shirq_unlock().

We would actually need some kind of IRQ_INPROGRESS + synchronize_irq()
at i-pipe level. But we have the problem, in contrast to the kernel,
that we reschedule from inside the handler (more precisely: at nucleus
level), thus synchronize_irq() would not just wait on some simple
handler to exit...

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

On 21/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> > [ .. ]
> > surprise-surprise.. sure, one way or another it must be fixed. heh..
> > too many bugs (and I don't even want to say who's responsible) :-/
>
> I wouldn't accept all the responsibility if I were you.

I have no problems in this respect. I was just a bit sarcastic with
the way to say "it's my fault".


> It's a sign that the design might be
> too complex now

frankly speaking, I don't think it's really complex :)


> Things get worse, at least with XENO_OPT_STATS: Due to the runtime
> statistics collection, we may end up with dangling references to our
> xnintr object even after xnintr_shirq_unlock().
>
> We would actually need some kind of IRQ_INPROGRESS + synchronize_irq()
> at i-pipe level. But we have the problem, in contrast to the kernel,
> that we reschedule from inside the handler (more precisely: at nucleus
> level), thus synchronize_irq() would not just wait on some simple
> handler to exit...

Yeah.. we had already conversations on this topic (I think with
Philippe) and, if I recall right, that was one of the reasons. That's
why synchronize_irq() is in the nucleus layer.


>
> Jan
>

-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 21/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> > [ .. ]
>> > surprise-surprise.. sure, one way or another it must be fixed. heh..
>> > too many bugs (and I don't even want to say who's responsible) :-/
>>
>> I wouldn't accept all the responsibility if I were you.
> 
> I have no problems in this respect. I was just a bit sarcastic with
> the way to say "it's my fault".
> 
> 
>> It's a sign that the design might be
>> too complex now
> 
> frankly speaking, I don't think it's really complex :)
> 
> 
>> Things get worse, at least with XENO_OPT_STATS: Due to the runtime
>> statistics collection, we may end up with dangling references to our
>> xnintr object even after xnintr_shirq_unlock().
>>
>> We would actually need some kind of IRQ_INPROGRESS + synchronize_irq()
>> at i-pipe level. But we have the problem, in contrast to the kernel,
>> that we reschedule from inside the handler (more precisely: at nucleus
>> level), thus synchronize_irq() would not just wait on some simple
>> handler to exit...
> 
> Yeah.. we had already conversations on this topic (I think with
> Philippe) and, if I recall right, that was one of the reasons. That's
> why synchronize_irq() is in the nucleus layer.

Hmm, then we may be forced to get the cookie out of ipipe's hands again
and put it back into a nucleus irq array, i.e. move the cookie
dereferencing under a nucleus managed per-irq lock. But there is still
the issue with xnsched_t::current_account...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Philippe Gerum]

On Thu, 2007-06-21 at 13:05 +0200, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Well, and I wonder what this xnarch_memory_barrier() at each handler
> > entry is for. Seems to be there for ages, don't see why right now.

AFAICT, this probably dates back to Xenomai 1.x, when we used to have a
threaded interrupt model. The actual code looked like as follows, and
the barrier was likely here to make sure that any change to the
interrupt hit counter was visible from any other CPU which would run the
interrupt service thread. The funny thing is that we did not have SMP
support at that time, anyway...

static void xnintr_irq_handler (unsigned irq, void *cookie)

{
    xnintr_t *intr = (xnintr_t *)cookie;
    int s = XN_ISR_SCHED_T;

    intr->hits++;

    xnarch_memory_barrier();


In short, I don't see any reason to keep this membar.

>  (The
> > kernel has a golden rule for this: no barrier without comments!)

Yeah, right. It looks like the kernel has a slew of very official golden
rules it basically does not care one dime to enforce in practice.
Looking at the code, commenting membars is surely one of them...

-- 
Philippe.

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Thu, 2007-06-21 at 13:05 +0200, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Well, and I wonder what this xnarch_memory_barrier() at each handler
>>> entry is for. Seems to be there for ages, don't see why right now.
> 
> AFAICT, this probably dates back to Xenomai 1.x, when we used to have a
> threaded interrupt model. The actual code looked like as follows, and
> the barrier was likely here to make sure that any change to the
> interrupt hit counter was visible from any other CPU which would run the
> interrupt service thread. The funny thing is that we did not have SMP
> support at that time, anyway...
> 
> static void xnintr_irq_handler (unsigned irq, void *cookie)
> 
> {
>     xnintr_t *intr = (xnintr_t *)cookie;
>     int s = XN_ISR_SCHED_T;
> 
>     intr->hits++;
> 
>     xnarch_memory_barrier();
> 
> 
> In short, I don't see any reason to keep this membar.

Fascinating: So many people came along this place, but no one dared to
touch it. :)

> 
>>  (The
>>> kernel has a golden rule for this: no barrier without comments!)
> 
> Yeah, right. It looks like the kernel has a slew of very official golden
> rules it basically does not care one dime to enforce in practice.
> Looking at the code, commenting membars is surely one of them...
> 

I saw Andrew Morton being strictly after uncommented ones in new code at
least.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Jan Kiszka wrote:
> Dmitry Adamushko wrote:
>> On 21/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>> [ .. ]
>>>> surprise-surprise.. sure, one way or another it must be fixed. heh..
>>>> too many bugs (and I don't even want to say who's responsible) :-/
>>> I wouldn't accept all the responsibility if I were you.
>> I have no problems in this respect. I was just a bit sarcastic with
>> the way to say "it's my fault".
>>
>>
>>> It's a sign that the design might be
>>> too complex now
>> frankly speaking, I don't think it's really complex :)
>>
>>
>>> Things get worse, at least with XENO_OPT_STATS: Due to the runtime
>>> statistics collection, we may end up with dangling references to our
>>> xnintr object even after xnintr_shirq_unlock().
>>>
>>> We would actually need some kind of IRQ_INPROGRESS + synchronize_irq()
>>> at i-pipe level. But we have the problem, in contrast to the kernel,
>>> that we reschedule from inside the handler (more precisely: at nucleus
>>> level), thus synchronize_irq() would not just wait on some simple
>>> handler to exit...
>> Yeah.. we had already conversations on this topic (I think with
>> Philippe) and, if I recall right, that was one of the reasons. That's
>> why synchronize_irq() is in the nucleus layer.
> 
> Hmm, then we may be forced to get the cookie out of ipipe's hands again
> and put it back into a nucleus irq array, i.e. move the cookie
> dereferencing under a nucleus managed per-irq lock. But there is still
> the issue with xnsched_t::current_account...
> 

Hell, what a mess...

Here is a first attempt to address the remaining issues. Take it with a
huge grain of salt, I haven't yet made up my mind if it really solves
our races and if it isn't too ugly.

Please direct special attention to

 - xnintr_sync_stat_references (the comment says it all)

 - xnintr_edge_shirq_handler (unrelated change, but I somehow felt like
   this is nicer)

Only compile-tested under various .configs. Any comment welcome.

Thanks,
Jan

[-- Attachment #1.2: fix-intr-locking-part-ii.patch --]
[-- Type: text/plain, Size: 6743 bytes --]

Index: xenomai/ksrc/nucleus/intr.c
===================================================================
--- xenomai.orig/ksrc/nucleus/intr.c
+++ xenomai/ksrc/nucleus/intr.c
@@ -41,6 +41,18 @@
 
 DEFINE_PRIVATE_XNLOCK(intrlock);
 
+typedef struct xnintr_irq {
+
+	DECLARE_XNLOCK(lock);
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+	xnintr_t *handlers;
+	int unhandled;
+#endif
+} ____cacheline_aligned_in_smp xnintr_irq_t;
+
+static xnintr_irq_t xnirqs[RTHAL_NR_IRQS];
+
 #ifdef CONFIG_XENO_OPT_STATS
 xnintr_t nkclock;	/* Only for statistics */
 int xnintr_count = 1;	/* Number of attached xnintr objects + nkclock */
@@ -63,9 +75,23 @@ static inline void xnintr_stat_counter_d
 	xnarch_memory_barrier();
 	xnintr_list_rev++;
 }
+
+static inline void xnintr_sync_stat_references(xnintr_t *intr)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu) {
+		xnsched_t *sched = xnpod_sched_slot(cpu);
+
+		/* I don't feel very well... hacking this. */
+		while (sched->current_account == &intr->stat[cpu].account)
+			cpu_relax();
+	}
+}
 #else
 static inline void xnintr_stat_counter_inc(void) {}
 static inline void xnintr_stat_counter_dec(void) {}
+static inline void xnintr_sync_stat_references(xnintr_t *intr) {}
 #endif /* CONFIG_XENO_OPT_STATS */
 
 /*
@@ -76,7 +102,7 @@ static inline void xnintr_stat_counter_d
 static void xnintr_irq_handler(unsigned irq, void *cookie)
 {
 	xnsched_t *sched = xnpod_current_sched();
-	xnintr_t *intr = (xnintr_t *)cookie;
+	xnintr_t *intr;
 	xnstat_runtime_t *prev;
 	xnticks_t start;
 	int s;
@@ -86,6 +112,16 @@ static void xnintr_irq_handler(unsigned 
 	xnltt_log_event(xeno_ev_ienter, irq);
 
 	++sched->inesting;
+
+	xnlock_get(&xnirqs[irq].lock);
+
+#ifdef CONFIG_SMP
+	/* In SMP case, we have to reload the cookie under the per-IRQ lock
+	   to avoid racing with xnintr_detach. */
+	intr = rthal_irq_cookie(&rthal_domain, irq);
+#else
+	intr = cookie;
+#endif
 	s = intr->isr(intr);
 
 	if (unlikely(s == XN_ISR_NONE)) {
@@ -102,6 +138,8 @@ static void xnintr_irq_handler(unsigned 
 		intr->unhandled = 0;
 	}
 
+	xnlock_put(&xnirqs[irq].lock);
+
 	if (s & XN_ISR_PROPAGATE)
 		xnarch_chain_irq(irq);
 	else if (!(s & XN_ISR_NOENABLE))
@@ -161,18 +199,6 @@ void xnintr_clock_handler(void)
 
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
 
-typedef struct xnintr_shirq {
-
-	xnintr_t *handlers;
-	int unhandled;
-#ifdef CONFIG_SMP
-	xnlock_t lock;
-#endif
-
-} xnintr_shirq_t;
-
-static xnintr_shirq_t xnshirqs[RTHAL_NR_IRQS];
-
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
 /*
  * Low-level interrupt handler dispatching the user-defined ISRs for
@@ -184,7 +210,7 @@ static void xnintr_shirq_handler(unsigne
 	xnsched_t *sched = xnpod_current_sched();
 	xnstat_runtime_t *prev;
 	xnticks_t start;
-	xnintr_shirq_t *shirq = &xnshirqs[irq];
+	xnintr_irq_t *shirq = &xnirqs[irq];
 	xnintr_t *intr;
 	int s = 0;
 
@@ -253,7 +279,7 @@ static void xnintr_edge_shirq_handler(un
 	xnsched_t *sched = xnpod_current_sched();
 	xnstat_runtime_t *prev;
 	xnticks_t start;
-	xnintr_shirq_t *shirq = &xnshirqs[irq];
+	xnintr_irq_t *shirq = &xnirqs[irq];
 	xnintr_t *intr, *end = NULL;
 	int s = 0, counter = 0;
 
@@ -277,15 +303,15 @@ static void xnintr_edge_shirq_handler(un
 		s |= ret;
 
 		if (code == XN_ISR_HANDLED) {
-			end = NULL;
+			if (!(end = (intr->next)))
+				end = shirq->handlers;
 			xnstat_counter_inc(
 				&intr->stat[xnsched_cpu(sched)].hits);
 			xnstat_runtime_lazy_switch(sched,
 				&intr->stat[xnsched_cpu(sched)].account,
 				start);
 			start = xnstat_runtime_now();
-		} else if (code == XN_ISR_NONE && end == NULL)
-			end = intr;
+		}
 
 		if (counter++ > MAX_EDGEIRQ_COUNTER)
 			break;
@@ -326,7 +352,7 @@ static void xnintr_edge_shirq_handler(un
 
 static inline int xnintr_irq_attach(xnintr_t *intr)
 {
-	xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+	xnintr_irq_t *shirq = &xnirqs[intr->irq];
 	xnintr_t *prev, **p = &shirq->handlers;
 	int err;
 
@@ -379,17 +405,16 @@ static inline int xnintr_irq_attach(xnin
 
 	intr->next = NULL;
 
-	/* Add a given interrupt object. */
-	xnlock_get(&shirq->lock);
+	/* Add the given interrupt object. No need to synchronise with the IRQ
+	   handler, we are only extending the chain. */
 	*p = intr;
-	xnlock_put(&shirq->lock);
 
 	return 0;
 }
 
 static inline int xnintr_irq_detach(xnintr_t *intr)
 {
-	xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+	xnintr_irq_t *shirq = &xnirqs[intr->irq];
 	xnintr_t *e, **p = &shirq->handlers;
 	int err = 0;
 
@@ -406,6 +431,7 @@ static inline int xnintr_irq_detach(xnin
 			/* Remove the given interrupt object from the list. */
 			xnlock_get(&shirq->lock);
 			*p = e->next;
+			xnintr_sync_stat_references(intr);
 			xnlock_put(&shirq->lock);
 
 			/* Release the IRQ line if this was the last user */
@@ -422,14 +448,6 @@ static inline int xnintr_irq_detach(xnin
 	return err;
 }
 
-int xnintr_mount(void)
-{
-	int i;
-	for (i = 0; i < RTHAL_NR_IRQS; ++i)
-		xnlock_init(&xnshirqs[i].lock);
-	return 0;
-}
-
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
 
 static inline int xnintr_irq_attach(xnintr_t *intr)
@@ -439,13 +457,29 @@ static inline int xnintr_irq_attach(xnin
 
 static inline int xnintr_irq_detach(xnintr_t *intr)
 {
-	return xnarch_release_irq(intr->irq);
-}
+	int irq = intr->irq;
+	int err;
+
+	xnlock_get(&xnirqs[irq].lock);
 
-int xnintr_mount(void) { return 0; }
+	err = xnarch_release_irq(intr->irq);
+	xnintr_sync_stat_references(intr);
+
+	xnlock_put(&xnirqs[irq].lock);
+
+	return err;
+}
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
+int xnintr_mount(void)
+{
+	int i;
+	for (i = 0; i < RTHAL_NR_IRQS; ++i)
+		xnlock_init(&xnirqs[i].lock);
+	return 0;
+}
+
 /*!
  * \fn int xnintr_init (xnintr_t *intr,const char *name,unsigned irq,xnisr_t isr,xniack_t iack,xnflags_t flags)
  * \brief Initialize an interrupt object.
@@ -809,7 +843,7 @@ int xnintr_irq_proc(unsigned int irq, ch
 	xnlock_get_irqsave(&intrlock, s);
 
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
-	intr = xnshirqs[irq].handlers;
+	intr = xnirqs[irq].handlers;
 	if (intr) {
 		strcpy(p, "        "); p += 8;
 
@@ -862,7 +896,7 @@ int xnintr_query(int irq, int *cpu, xnin
 	else if (irq == XNARCH_TIMER_IRQ)
 		intr = &nkclock;
 	else
-		intr = xnshirqs[irq].handlers;
+		intr = xnirqs[irq].handlers;
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
 	if (*prev)
 		intr = NULL;

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

On 22/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> [ ... ]
>
> Only compile-tested under various .configs. Any comment welcome.
>

> @@ -76,7 +102,7 @@ static inline void xnintr_stat_counter_d
>  static void xnintr_irq_handler(unsigned irq, void *cookie)
>  {
>         xnsched_t *sched = xnpod_current_sched();
> -       xnintr_t *intr = (xnintr_t *)cookie;
> +       xnintr_t *intr;
>         xnstat_runtime_t *prev;
>         xnticks_t start;
>         int s;
> @@ -86,6 +112,16 @@ static void xnintr_irq_handler(unsigned
>         xnltt_log_event(xeno_ev_ienter, irq);
>
>         ++sched->inesting;
> +
> +       xnlock_get(&xnirqs[irq].lock);
> +
> +#ifdef CONFIG_SMP
> +       /* In SMP case, we have to reload the cookie under the per-IRQ lock
> +          to avoid racing with xnintr_detach. */
> +       intr = rthal_irq_cookie(&rthal_domain, irq);
> +#else
> +       intr = cookie;
> +#endif
>         s = intr->isr(intr);

I guess, 'intr' can be NULL here.

Could you please send me attached (non-inlined) a combo patch on top
of the trunk version (as I see this one seems to be on top of your
previous one)? I'll try to come up with some solution during this
weekend.


-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 22/06/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> [ ... ]
>>
>> Only compile-tested under various .configs. Any comment welcome.
>>
> 
>> @@ -76,7 +102,7 @@ static inline void xnintr_stat_counter_d
>>  static void xnintr_irq_handler(unsigned irq, void *cookie)
>>  {
>>         xnsched_t *sched = xnpod_current_sched();
>> -       xnintr_t *intr = (xnintr_t *)cookie;
>> +       xnintr_t *intr;
>>         xnstat_runtime_t *prev;
>>         xnticks_t start;
>>         int s;
>> @@ -86,6 +112,16 @@ static void xnintr_irq_handler(unsigned
>>         xnltt_log_event(xeno_ev_ienter, irq);
>>
>>         ++sched->inesting;
>> +
>> +       xnlock_get(&xnirqs[irq].lock);
>> +
>> +#ifdef CONFIG_SMP
>> +       /* In SMP case, we have to reload the cookie under the per-IRQ
>> lock
>> +          to avoid racing with xnintr_detach. */
>> +       intr = rthal_irq_cookie(&rthal_domain, irq);
>> +#else
>> +       intr = cookie;
>> +#endif
>>         s = intr->isr(intr);
> 
> I guess, 'intr' can be NULL here.

Yeah, needs to be caught as well.

> 
> Could you please send me attached (non-inlined) a combo patch on top
> of the trunk version (as I see this one seems to be on top of your
> previous one)? I'll try to come up with some solution during this
> weekend.

My patch was an attachment (though I wonder we everyone has so much
problems with inlines - broken mailers all over the place), and it was
already against trunk (#2654). Philippe merged my preliminary fix this
morning.

Looking forward to your suggestions!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

Hello Jan,

well, take it with 2 huge grains of salt.. it's even not compile-tested :-)

I don't think I've got it really nicer than your first patch.. the only additional point is that
'prev = xnstat_get_current()' reference is also tracked as reference accounting becomes
a part of the xnstat interface (not sure we do need it though).

Well (#2), I have to say that the whole intr subsystem looks not that nice (maybe partly due
to being overloaded with xnstat details).. so I think, it'd require a closer look.

Anyway, here is what I've got out of your patch so far.. the main issue is whether we go this way
or it just makes things even more uglier.

(a side effect : I transformed xnstat_* macroses into 'static inline' function.. I guess, this way it's a bit nicer)

----------

diff -upr xenomai-orig/include/nucleus/stat.h xenomai-intr/include/nucleus/stat.h
--- xenomai-orig/include/nucleus/stat.h	2007-06-23 14:03:58.000000000 +0200
+++ xenomai-intr/include/nucleus/stat.h	2007-06-24 15:25:37.000000000 +0200
@@ -31,46 +31,72 @@ typedef struct xnstat_runtime {
 
 	xnticks_t total; /* Accumulated execution time */
 
+	atomic_counter_t refs;
+
 } xnstat_runtime_t;
 
 /* Return current date which can be passed to other xnstat services for
    immediate or lazy accounting. */
-#define xnstat_runtime_now() xnarch_get_cpu_tsc()
+static inline xnticks_t xnstat_runtime_now(void)
+{
+	return xnarch_get_cpu_tsc();
+}
 
 /* Accumulate runtime of the current account until the given date. */
-#define xnstat_runtime_update(sched, start) \
-do { \
-	(sched)->current_account->total += \
-		start - (sched)->last_account_switch; \
-	(sched)->last_account_switch = start; \
-} while (0)
+static inline void xnstat_runtime_update(xnsched_t *sched, xnticks_t start)
+{
+	sched->current_account->total += start - sched->last_account_switch;
+	sched->last_account_switch = start;
+}
 
 /* Update the current account reference, returning the previous one. */
-#define xnstat_runtime_set_current(sched, new_account) \
-({ \
-	xnstat_runtime_t *__prev; \
-	__prev = xnarch_atomic_xchg(&(sched)->current_account, (new_account)); \
-	__prev; \
-})
+static inline void xnstat_runtime_set_current(xnsched_t *sched, xnstat_runtime_t *new_account)
+{
+	xnstat_runtime_t *prev;
+
+	if (likely(new_account))
+		xnarch_atomic_inc(&new_account->refs);
+	prev = xnarch_atomic_xchg(&sched->current_account, new_account);
+	if (likely(prev))
+		xnarch_atomic_dec(&prev->refs);
+}
 
 /* Return the currently active accounting entity. */
-#define xnstat_runtime_get_current(sched) ((sched)->current_account)
+static inline xnstat_runtime_t * xnstat_runtime_get_current(xnsched_t *sched)
+{
+	xnstat_runtime_t *curr = sched->current_account;
+
+	if (likely(curr))
+		xnarch_atomic_inc(&curr->refs);
+}
+
+static inline void xnstat_runtime_put(xnstat_runtime_t *stat)
+{
+	if (likely(stat))
+		xnarch_atomic_dec(&stat->refs);
+}
 
 /* Finalize an account (no need to accumulate the runtime, just mark the
    switch date and set the new account). */
-#define xnstat_runtime_finalize(sched, new_account) \
-do { \
-	(sched)->last_account_switch = xnarch_get_cpu_tsc(); \
-	(sched)->current_account = (new_account); \
-} while (0)
+static inline void xnstat_runtime_finalize(xnsched_t *sched, xnstat_runtime_t *new_account)
+{
+	sched->last_account_switch = xnarch_get_cpu_tsc();
+	xnstat_runtime_set_current(sched, new_account);
+}
 
 /* Reset statistics from inside the accounted entity (e.g. after CPU
    migration). */
-#define xnstat_runtime_reset_stats(stat) \
-do { \
-	(stat)->total = 0; \
-	(stat)->start = xnarch_get_cpu_tsc(); \
-} while (0)
+static inline void xnstat_runtime_reset_stats(xnstat_runtime_t *stat)
+{
+	stat->total = 0;
+	stat->start = xnarch_get_cpu_tsc();
+}
+
+static void xnstat_runtime_sync(xnstat_runtime_t *stat)
+{
+	while (xnarch_atomic_get(&stat->refs))
+		cpu_relax();
+}
 
 
 typedef struct xnstat_counter {
@@ -101,10 +127,12 @@ typedef struct xnstat_runtime {
 
 #define xnstat_runtime_now()					({ 0; })
 #define xnstat_runtime_update(sched, start)			do { } while (0)
-#define xnstat_runtime_set_current(sched, new_account)	({ (void)sched; NULL; })
+#define xnstat_runtime_set_current(sched, new_account)		do { } while (0)
 #define xnstat_runtime_get_current(sched)			({ (void)sched; NULL; })
+#define xnstat_runtime_put(stat)				do { } while (0)
 #define xnstat_runtime_finalize(sched, new_account)		do { } while (0)
 #define xnstat_runtime_reset_stats(account)			do { } while (0)
+#define xnstat_runtime_sync(account)				do { } while (0)
 
 typedef struct xnstat_counter {
 #ifdef __XENO_SIM__
@@ -119,18 +147,20 @@ typedef struct xnstat_counter {
 
 /* Account the runtime of the current account until now, switch to
    new_account, and return the previous one. */
-#define xnstat_runtime_switch(sched, new_account) \
-({ \
-	xnstat_runtime_update(sched, xnstat_runtime_now()); \
-	xnstat_runtime_set_current(sched, new_account); \
-})
+static inline void
+xnstat_runtime_switch(xnsched_t *sched, xnstat_runtime_t *new_account)
+{
+	xnstat_runtime_update(sched, xnstat_runtime_now());
+	xnstat_runtime_set_current(sched, new_account);
+}
 
 /* Account the runtime of the current account until given start time, switch
    to new_account, and return the previous one. */
-#define xnstat_runtime_lazy_switch(sched, new_account, start) \
-({ \
-	xnstat_runtime_update(sched, start); \
-	xnstat_runtime_set_current(sched, new_account); \
-})
+static inline void
+xnstat_runtime_lazy_switch(xnsched_t *sched, xnstat_runtime_t *new_account, xnticks_t start)
+{
+	xnstat_runtime_update(sched, start);
+	xnstat_runtime_set_current(sched, new_account);
+}
 
 #endif /* !_XENO_NUCLEUS_STAT_H */
diff -upr xenomai-orig/ksrc/nucleus/intr.c xenomai-intr/ksrc/nucleus/intr.c
--- xenomai-orig/ksrc/nucleus/intr.c	2007-06-23 15:13:43.000000000 +0200
+++ xenomai-intr/ksrc/nucleus/intr.c	2007-06-24 15:47:58.000000000 +0200
@@ -41,6 +41,18 @@
 
 DEFINE_PRIVATE_XNLOCK(intrlock);
 
+typedef struct xnintr_irq {
+
+	DECLARE_XNLOCK(lock);
+
+#if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
+	xnintr_t *handlers;
+	int unhandled;
+#endif
+} ____cacheline_aligned_in_smp xnintr_irq_t;
+
+static xnintr_irq_t xnirqs[RTHAL_NR_IRQS];
+
 #ifdef CONFIG_XENO_OPT_STATS
 xnintr_t nkclock;	/* Only for statistics */
 int xnintr_count = 1;	/* Number of attached xnintr objects + nkclock */
@@ -63,9 +75,18 @@ static inline void xnintr_stat_counter_d
 	xnarch_memory_barrier();
 	xnintr_list_rev++;
 }
+
+static inline void xnintr_sync_stat_refs(xnintr_t *intr)
+{
+	int cpu;
+
+	for_each_online_cpu(cpu)
+		xnstat_runtime_sync(&intr->stat[cpu].account);
+}
 #else
 static inline void xnintr_stat_counter_inc(void) {}
 static inline void xnintr_stat_counter_dec(void) {}
+static inline void xnintr_sync_stat_refs(xnintr_t *intr) {}
 #endif /* CONFIG_XENO_OPT_STATS */
 
 /*
@@ -76,31 +97,44 @@ static inline void xnintr_stat_counter_d
 static void xnintr_irq_handler(unsigned irq, void *cookie)
 {
 	xnsched_t *sched = xnpod_current_sched();
-	xnintr_t *intr = (xnintr_t *)cookie;
+	xnintr_t *intr;
 	xnstat_runtime_t *prev;
 	xnticks_t start;
-	int s;
+	int s = 0;
 
 	prev  = xnstat_runtime_get_current(sched);
 	start = xnstat_runtime_now();
 	xnltt_log_event(xeno_ev_ienter, irq);
 
 	++sched->inesting;
-	s = intr->isr(intr);
 
-	if (unlikely(s == XN_ISR_NONE)) {
-		if (++intr->unhandled == XNINTR_MAX_UNHANDLED) {
-			xnlogerr("%s: IRQ%d not handled. Disabling IRQ "
-				 "line.\n", __FUNCTION__, irq);
-			s |= XN_ISR_NOENABLE;
+	xnlock_get(&xnirqs[irq].lock);
+
+#ifdef CONFIG_SMP
+	/* In SMP case, we have to reload the cookie under the per-IRQ lock
+	   to avoid racing with xnintr_detach. */
+	intr = rthal_irq_cookie(&rthal_domain, irq);
+#else
+	intr = cookie;
+#endif
+	if (likely(intr)) {
+		s = intr->isr(intr);
+
+		if (unlikely(s == XN_ISR_NONE)) {
+			if (++intr->unhandled == XNINTR_MAX_UNHANDLED) {
+				xnlogerr("%s: IRQ%d not handled. Disabling IRQ "
+					 "line.\n", __FUNCTION__, irq);
+				s |= XN_ISR_NOENABLE;
+			}
+		} else {
+			xnstat_counter_inc(&intr->stat[xnsched_cpu(sched)].hits);
+			xnstat_runtime_lazy_switch(sched,
+				&intr->stat[xnsched_cpu(sched)].account,
+				start);
+			intr->unhandled = 0;
 		}
-	} else {
-		xnstat_counter_inc(&intr->stat[xnsched_cpu(sched)].hits);
-		xnstat_runtime_lazy_switch(sched,
-			&intr->stat[xnsched_cpu(sched)].account,
-			start);
-		intr->unhandled = 0;
 	}
+	xnlock_put(&xnirqs[irq].lock);
 
 	if (s & XN_ISR_PROPAGATE)
 		xnarch_chain_irq(irq);
@@ -112,6 +146,7 @@ static void xnintr_irq_handler(unsigned 
 
 	xnltt_log_event(xeno_ev_iexit, irq);
 	xnstat_runtime_switch(sched, prev);
+	xnstat_runtime_put(prev);
 }
 
 /* Low-level clock irq handler. */
@@ -155,24 +190,13 @@ void xnintr_clock_handler(void)
 
 	xnltt_log_event(xeno_ev_iexit, XNARCH_TIMER_IRQ);
 	xnstat_runtime_switch(sched, prev);
+	xnstat_runtime_put(prev);
 }
 
 /* Optional support for shared interrupts. */
 
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
 
-typedef struct xnintr_shirq {
-
-	xnintr_t *handlers;
-	int unhandled;
-#ifdef CONFIG_SMP
-	xnlock_t lock;
-#endif
-
-} xnintr_shirq_t;
-
-static xnintr_shirq_t xnshirqs[RTHAL_NR_IRQS];
-
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL)
 /*
  * Low-level interrupt handler dispatching the user-defined ISRs for
@@ -184,7 +208,7 @@ static void xnintr_shirq_handler(unsigne
 	xnsched_t *sched = xnpod_current_sched();
 	xnstat_runtime_t *prev;
 	xnticks_t start;
-	xnintr_shirq_t *shirq = &xnshirqs[irq];
+	xnintr_irq_t *shirq = &xnirqs[irq];
 	xnintr_t *intr;
 	int s = 0;
 
@@ -236,6 +260,7 @@ static void xnintr_shirq_handler(unsigne
 
 	xnltt_log_event(xeno_ev_iexit, irq);
 	xnstat_runtime_switch(sched, prev);
+	xnstat_runtime_put(prev);
 }
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL */
@@ -253,7 +278,7 @@ static void xnintr_edge_shirq_handler(un
 	xnsched_t *sched = xnpod_current_sched();
 	xnstat_runtime_t *prev;
 	xnticks_t start;
-	xnintr_shirq_t *shirq = &xnshirqs[irq];
+	xnintr_irq_t *shirq = &xnirqs[irq];
 	xnintr_t *intr, *end = NULL;
 	int s = 0, counter = 0;
 
@@ -277,15 +302,15 @@ static void xnintr_edge_shirq_handler(un
 		s |= ret;
 
 		if (code == XN_ISR_HANDLED) {
-			end = NULL;
+			if (!(end = (intr->next)))
+				end = shirq->handlers;
 			xnstat_counter_inc(
 				&intr->stat[xnsched_cpu(sched)].hits);
 			xnstat_runtime_lazy_switch(sched,
 				&intr->stat[xnsched_cpu(sched)].account,
 				start);
 			start = xnstat_runtime_now();
-		} else if (code == XN_ISR_NONE && end == NULL)
-			end = intr;
+		}
 
 		if (counter++ > MAX_EDGEIRQ_COUNTER)
 			break;
@@ -320,13 +345,14 @@ static void xnintr_edge_shirq_handler(un
 
 	xnltt_log_event(xeno_ev_iexit, irq);
 	xnstat_runtime_switch(sched, prev);
+	xnstat_runtime_put(prev);
 }
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_EDGE */
 
 static inline int xnintr_irq_attach(xnintr_t *intr)
 {
-	xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+	xnintr_irq_t *shirq = &xnirqs[intr->irq];
 	xnintr_t *prev, **p = &shirq->handlers;
 	int err;
 
@@ -379,17 +405,16 @@ static inline int xnintr_irq_attach(xnin
 
 	intr->next = NULL;
 
-	/* Add a given interrupt object. */
-	xnlock_get(&shirq->lock);
+	/* Add the given interrupt object. No need to synchronise with the IRQ
+	   handler, we are only extending the chain. */
 	*p = intr;
-	xnlock_put(&shirq->lock);
 
 	return 0;
 }
 
 static inline int xnintr_irq_detach(xnintr_t *intr)
 {
-	xnintr_shirq_t *shirq = &xnshirqs[intr->irq];
+	xnintr_irq_t *shirq = &xnirqs[intr->irq];
 	xnintr_t *e, **p = &shirq->handlers;
 	int err = 0;
 
@@ -408,6 +433,8 @@ static inline int xnintr_irq_detach(xnin
 			*p = e->next;
 			xnlock_put(&shirq->lock);
 
+			xnintr_sync_stat_references(intr);
+
 			/* Release the IRQ line if this was the last user */
 			if (shirq->handlers == NULL)
 				err = xnarch_release_irq(intr->irq);
@@ -422,14 +449,6 @@ static inline int xnintr_irq_detach(xnin
 	return err;
 }
 
-int xnintr_mount(void)
-{
-	int i;
-	for (i = 0; i < RTHAL_NR_IRQS; ++i)
-		xnlock_init(&xnshirqs[i].lock);
-	return 0;
-}
-
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
 
 static inline int xnintr_irq_attach(xnintr_t *intr)
@@ -439,13 +458,30 @@ static inline int xnintr_irq_attach(xnin
 
 static inline int xnintr_irq_detach(xnintr_t *intr)
 {
-	return xnarch_release_irq(intr->irq);
-}
+	int irq = intr->irq;
+	int err;
+
+	xnlock_get(&xnirqs[irq].lock);
+
+	err = xnarch_release_irq(intr->irq);
 
-int xnintr_mount(void) { return 0; }
+	xnlock_put(&xnirqs[irq].lock);
+
+	xnintr_sync_stat_references(intr);
+
+	return err;
+}
 
 #endif /* CONFIG_XENO_OPT_SHIRQ_LEVEL || CONFIG_XENO_OPT_SHIRQ_EDGE */
 
+int xnintr_mount(void)
+{
+	int i;
+	for (i = 0; i < RTHAL_NR_IRQS; ++i)
+		xnlock_init(&xnirqs[i].lock);
+	return 0;
+}
+
 /*!
  * \fn int xnintr_init (xnintr_t *intr,const char *name,unsigned irq,xnisr_t isr,xniack_t iack,xnflags_t flags)
  * \brief Initialize an interrupt object.
@@ -809,7 +845,7 @@ int xnintr_irq_proc(unsigned int irq, ch
 	xnlock_get_irqsave(&intrlock, s);
 
 #if defined(CONFIG_XENO_OPT_SHIRQ_LEVEL) || defined(CONFIG_XENO_OPT_SHIRQ_EDGE)
-	intr = xnshirqs[irq].handlers;
+	intr = xnirqs[irq].handlers;
 	if (intr) {
 		strcpy(p, "        "); p += 8;
 
@@ -862,7 +898,7 @@ int xnintr_query(int irq, int *cpu, xnin
 	else if (irq == XNARCH_TIMER_IRQ)
 		intr = &nkclock;
 	else
-		intr = xnshirqs[irq].handlers;
+		intr = xnirqs[irq].handlers;
 #else /* !CONFIG_XENO_OPT_SHIRQ_LEVEL && !CONFIG_XENO_OPT_SHIRQ_EDGE */
 	if (*prev)
 		intr = NULL;

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Jan Kiszka]

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

Hi Dmitry,

Dmitry Adamushko wrote:
> Hello Jan,
> 
> well, take it with 2 huge grains of salt.. it's even not compile-tested :-)

Yeah, that might explain while already trying to parse it manually
failed: What is xnintr_sync_stat_references? :)

> 
> I don't think I've got it really nicer than your first patch.. the only additional point is that
> 'prev = xnstat_get_current()' reference is also tracked as reference accounting becomes
> a part of the xnstat interface (not sure we do need it though).

Mind to elaborate on _why_ you think we need this, specifically if it
adds new atomic counters? I'm too lazy looking for the probably valid
reason on my own. Apropos atomic use counters: already considered all
memory barrier issues for your locking scheme?

> 
> Well (#2), I have to say that the whole intr subsystem looks not that nice (maybe partly due
> to being overloaded with xnstat details).. so I think, it'd require a closer look.

Let's get it right first, then think about aesthetic or even concrete
optimisations again.

> 
> Anyway, here is what I've got out of your patch so far.. the main issue is whether we go this way
> or it just makes things even more uglier.

Again, correctness, race-avoidance worries me first right now.

> 
> (a side effect : I transformed xnstat_* macroses into 'static inline' function.. I guess, this way it's a bit nicer)

Uhh, be careful, I burned my fingers with similar things recently as
well. You have to make sure that all types are resolvable for _all_
includers of that header. Otherwise, I'm fine with cleanups like this.
But I think there was once a reason for #define.

Thanks,
Jan


PS: Please note my latest refactoring check-in to SVN, touching stat.h.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [Xenomai-core] [RFC][PATCH] shirq locking rework

[Dmitry Adamushko]

Hello Jan,

I appologize for the huge reply latency.

>
> Yeah, that might explain while already trying to parse it manually
> failed: What is xnintr_sync_stat_references? :)

yeah.. it was supposed to be xnintr_sync_stat_refs()


> > 'prev = xnstat_get_current()' reference is also tracked as reference accounting becomes
> > a part of the xnstat interface (not sure we do need it though).
>
> Mind to elaborate on _why_ you think we need this, specifically if it
> adds new atomic counters?

Forget about it, it was a wrong approach. We do reschedule in
xnintr_*_handler() and if 'prev->refs' is non-zero and a newly
scheduled thread calls xnstat_runtime_synch() (well, how it could be
in theory with this interfcae) before deleting the first thread..
oops. so this 'referencing' scheme is bad anyway.

Note, that if the real re-schedule took place in xnpod_schedule() , we
actually don't need to _restore_ 'prev' when we get control back.. it
must be already restored by xnpod_schedule() when the preempted thread
('prev' is normally a thread in which context an interrupt occurs)
gets CPU back. if I'm not missing something. hum?

...
        if (--sched->inesting == 0 && xnsched_resched_p())
                xnpod_schedule();

(*) <---- 'sched->current_account' should be already == 'prev' in case
xnpod_schedule() took place

        xnltt_log_event(xeno_ev_iexit, irq);
        xnstat_runtime_switch(sched, prev);
...

The simpler scheme with xnstat_ accounting would be if we account only
time spent in intr->isr() to corresponding intr->stat[cpu].account...
This way, all accesses to the later one would be inside
xnlock_{get,put}(&xnirqs[irq].lock) sections [*].

It's preciceness (although, it's arguable to some extent) vs.
simplicity (e.g. no need for any xnintr_sync_stat_references()). I
would still prefer this approach :-)

Otherwise, so far I don't see any much nicer solution that the one
illustrated by your first patch.


> Uhh, be careful, I burned my fingers with similar things recently as
> well. You have to make sure that all types are resolvable for _all_
> includers of that header. Otherwise, I'm fine with cleanups like this.
> But I think there was once a reason for #define.

yeah.. now I recall it as well :-)


>
> Thanks,
> Jan
>

-- 
Best regards,
Dmitry Adamushko