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

[Jan Kiszka <jan.kiszka@domain.hid>]

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