shirq locking rework

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,

Hello Jan,

Quoted 5 lines

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 :)

[ ... ]

Quoted 8 lines

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. :->

Jan

Quoted 10 lines · Jan Kiszka

[ ... ]

Quoted 8 lines

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 :)

Quoted 7 lines

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)".

[ ... ]

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.

Sigh, the never ending IRQ story...

Jan

Quoted 6 lines · Jan Kiszka

[ ... ]

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) :-/

Quoted 2 lines

Sigh, the never ending IRQ story...

should be reviewed.

Quoted 3 lines

[ .. ]
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.

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

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

Quoted 5 lines · Jan Kiszka

Quoted 3 lines

[ .. ]
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".

Quoted 2 lines

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

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

Quoted 9 lines

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.

Quoted 23 lines · Dmitry Adamushko

Quoted 4 lines · Jan Kiszka

Quoted 3 lines

[ .. ]
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".

Quoted 2 lines

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

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

Quoted 9 lines

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...

Quoted 3 lines

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...

[ ... ]

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

ksrc/nucleus/intr.c +2 -1

@@ -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;
+

Quoted 17 lines · Jan Kiszka

[ ... ]

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

ksrc/nucleus/intr.c +1 -1

@@ -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);
 

ksrc/nucleus/intr.c static void xnintr_irq_handler(unsigned · line 125

Quoted 13 lines

ksrc/nucleus/intr.c +5 -0

        ++sched->inesting;
+
+       xnlock_get(&xnirqs[irq].lock);
+
+#ifdef CONFIG_SMP
+       /* In SMP case, we have to reload the cookie under the per-IRQ

lock

ksrc/nucleus/intr.c +5 -0

+          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.

Quoted 2 lines · Jan Kiszka

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

Quoted 1 line

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

Quoted 3 lines · Jan Kiszka

Quoted 2 lines · Jan Kiszka

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.

Quoted 3 lines

(The

Quoted 1 line

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...