[Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Jan Kiszka]

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

Hi all,

I've just uploaded my rebased patch stack, including specifically the
last bits that should go in before -rc1, namely

- only-setsched-in-trampoline.patch: missing bits for rt-caps-groups
- clean-nktbase-init.patch: nitpicking, but more consistent
- fix-intr-locking-part-ii-v2.patch: basically my first version but
  addressing the NULL issue Dmitry pointed out and adding one - as I
  think - important barrier. Dmitry, please have another look if I
  messed something up or if I missed something from your proposal.
- refactor-timer-modes-v2.patch: the final(?) version
- rtdm-timer-v3.patch: rebased over timer-modes-v2

Compile-tested, at least booting under qemu/smp, another real box is
about to boot in a few seconds.

Jan


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

Re: [Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Dmitry Adamushko]

On 02/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> Dmitry Adamushko wrote:
> > On 01/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >> Hi all,
> >>
> >> I've just uploaded my rebased patch stack, including specifically the
> >> last bits that should go in before -rc1,

a few comments:

(1) in xnintr_irq_handler()

+	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);
+	if (unlikely(!intr)) {
+		s = 0;
+		goto unlock_and_exit;
+	}
+#else
+	intr = cookie;
+#endif

I think, it would be better to check 'intr' for NULL at this point so
to cover 'cookie == NULL' (who knows.. and it's better not to rely on
the lower layer here), I guess.


(2) use cases of xnintr_sync_stat_references() in xnintr_irq_detach()

+
+	xnlock_get(&xnirqs[irq].lock);
+
+	err = xnarch_release_irq(intr->irq);
+	xnintr_sync_stat_references(intr);
+	xnlock_put(&xnirqs[irq].lock);

Why do you call xnintr_sync_stat_references() inside the locked section here?
Does xnintr_sync_stat_references() really need to be protected by
'intr->lock' (it's been already detached) ?


>
> Jan
>

-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 02/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Dmitry Adamushko wrote:
>> > On 01/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> >> Hi all,
>> >>
>> >> I've just uploaded my rebased patch stack, including specifically the
>> >> last bits that should go in before -rc1,
> 
> a few comments:
> 
> (1) in xnintr_irq_handler()
> 
> +    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);
> +    if (unlikely(!intr)) {
> +        s = 0;
> +        goto unlock_and_exit;
> +    }
> +#else
> +    intr = cookie;
> +#endif
> 
> I think, it would be better to check 'intr' for NULL at this point so
> to cover 'cookie == NULL' (who knows.. and it's better not to rely on
> the lower layer here), I guess.

The lower layer just passes what we hand over - and when we do so. Keep
in mind: this is the non-SMP case, thus we can rely on the atomicity of
this handler like the one of registering a handler + its cookie (like we
always did -- unfortunately also for SMP...). For me, checking for NULL
on UP is redundant code in a hotpath. Should I spread some comment
regarding this assumption?

> 
> (2) use cases of xnintr_sync_stat_references() in xnintr_irq_detach()
> 
> +
> +    xnlock_get(&xnirqs[irq].lock);
> +
> +    err = xnarch_release_irq(intr->irq);
> +    xnintr_sync_stat_references(intr);
> +    xnlock_put(&xnirqs[irq].lock);
> 
> Why do you call xnintr_sync_stat_references() inside the locked section
> here?
> Does xnintr_sync_stat_references() really need to be protected by
> 'intr->lock' (it's been already detached) ?

That's now my own overcautiousness :). I think we can move it out.
Someone must sync on some xnintr object while someone else tries to
re-register an object with very same address -- classical user error,
nothing we need to catch here.

Thanks,
Jan


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

Re: [Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Jan Kiszka]

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

Jan Kiszka wrote:
> Dmitry Adamushko wrote:
>> On 02/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>> Dmitry Adamushko wrote:
>>>> On 01/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>>>>> Hi all,
>>>>>
>>>>> I've just uploaded my rebased patch stack, including specifically the
>>>>> last bits that should go in before -rc1,
>> a few comments:
>>
>> (1) in xnintr_irq_handler()
>>
>> +    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);
>> +    if (unlikely(!intr)) {
>> +        s = 0;
>> +        goto unlock_and_exit;
>> +    }
>> +#else
>> +    intr = cookie;
>> +#endif
>>
>> I think, it would be better to check 'intr' for NULL at this point so
>> to cover 'cookie == NULL' (who knows.. and it's better not to rely on
>> the lower layer here), I guess.
> 
> The lower layer just passes what we hand over - and when we do so. Keep
> in mind: this is the non-SMP case, thus we can rely on the atomicity of
> this handler like the one of registering a handler + its cookie (like we
> always did -- unfortunately also for SMP...). For me, checking for NULL
> on UP is redundant code in a hotpath. Should I spread some comment
> regarding this assumption?
> 
>> (2) use cases of xnintr_sync_stat_references() in xnintr_irq_detach()
>>
>> +
>> +    xnlock_get(&xnirqs[irq].lock);
>> +
>> +    err = xnarch_release_irq(intr->irq);
>> +    xnintr_sync_stat_references(intr);
>> +    xnlock_put(&xnirqs[irq].lock);
>>
>> Why do you call xnintr_sync_stat_references() inside the locked section
>> here?
>> Does xnintr_sync_stat_references() really need to be protected by
>> 'intr->lock' (it's been already detached) ?
> 
> That's now my own overcautiousness :). I think we can move it out.
> Someone must sync on some xnintr object while someone else tries to
> re-register an object with very same address -- classical user error,
> nothing we need to catch here.

Modified version just went online, see

http://www.rts.uni-hannover.de/rtaddon/patches/xenomai/fix-intr-locking-part-ii-v3.patch

whitespace-damaged interdiff:

--- xenomai/ksrc/nucleus/intr.c
+++ xenomai/ksrc/nucleus/intr.c
@@ -124,6 +124,7 @@
                goto unlock_and_exit;
        }
 #else
+       /* cookie always valid, attach/detach happens with IRQs disabled */
        intr = cookie;
 #endif
        s = intr->isr(intr);
@@ -438,9 +439,10 @@
                        /* Remove the given interrupt object from the list. */
                        xnlock_get(&shirq->lock);
                        *p = e->next;
-                       xnintr_sync_stat_references(intr);
                        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);
@@ -468,12 +470,11 @@
        int err;
 
        xnlock_get(&xnirqs[irq].lock);
-
        err = xnarch_release_irq(intr->irq);
-       xnintr_sync_stat_references(intr);
-
        xnlock_put(&xnirqs[irq].lock);
 
+       xnintr_sync_stat_references(intr);
+
        return err;
 }
 

Jan


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

Re: [Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Dmitry Adamushko]

On 03/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Modified version just went online, see
> http://www.rts.uni-hannover.de/rtaddon/patches/xenomai/fix-intr-locking-part-ii-v3.patch
>

Should be ok now, I think.

Please, add your "Copyright notice" to intr.c (I think, Philippe
has no objections here).


>
> Jan
>

-- 
Best regards,
Dmitry Adamushko

Re: [Xenomai-core] [PATCH-STACK] Missing bits for -rc1

[Philippe Gerum]

On Wed, 2007-07-04 at 10:37 +0200, Dmitry Adamushko wrote:
> On 03/07/07, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >
> > Modified version just went online, see
> > http://www.rts.uni-hannover.de/rtaddon/patches/xenomai/fix-intr-locking-part-ii-v3.patch
> >
> 
> Should be ok now, I think.
> 
> Please, add your "Copyright notice" to intr.c (I think, Philippe
> has no objections here).
> 

Of course not. Responsability for bugs is nice to share. Eh.

> 
> >
> > Jan
> >
> 
-- 
Philippe.