[Xenomai-core] [shirq] first test results

[Xenomai-core] [shirq] first test results

[Jan Kiszka]

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

Hi Dmitry,

some news from the testing front: It works fairly well - and it doesn't
crash =:). We set up a quite demanding test scenario which consists of
two Sick Laser scanners feeding two UART ports at 500 Kbit/s. The UARTs
are on a special PC104 card, sharing the same edge-triggered IRQ line.
We were able to get data from both devices running at the same time. But
we noticed some overruns of MAX_EDGEIRQ_COUNTER (a few per second). The
next step on Monday will be to generate a back-trace with the
ipipe-tracer to see if the system is "just" overloaded or if we are
still facing problems with the driver and/or IRQ layer. Will be very
interesting to see on that radar what's happing.

I attached two patches. One enables xeno_16550A to use the new features,
and the other improves the /proc output of your patch slightly.

Furthermore, we noticed that virtual IRQs (namely the printk forwarder)
get displayed under /proc/xenomai/irq too. Is this useful? We wondered
what IRQ 34 might be until code analysis of Xenomai and Ipipe revealed
it (__ipipe_printk_virq). If it is considered useful, we should at least
mark those irqs virtual in the output or even give them names as well.

Then I stumbled over the xnintr structure. Why do you keep a copy of the
device name? A "const char *" should be enough, we just have to demand
that it will remain valid as long as the xnintr structure itself (i.e.
during the IRQ being attached). Saves a few bytes. :)

Jan

[-- Attachment #1.2: shirq-16550A.patch --]
[-- Type: text/plain, Size: 955 bytes --]

Index: ksrc/drivers/16550A/16550A.c
===================================================================
--- ksrc/drivers/16550A/16550A.c	(revision 556)
+++ ksrc/drivers/16550A/16550A.c	(working copy)
@@ -238,7 +238,7 @@
     int                     rbytes = 0;
     int                     events = 0;
     int                     modem;
-    int                     ret = RTDM_IRQ_PROPAGATE;
+    int                     ret = RTDM_IRQ_PROPAGATE | RTDM_IRQ_NOINT;
 
 
     ctx = rtdm_irq_get_arg(irq_context, struct rt_16550_context);
@@ -446,7 +446,8 @@
     ctx = (struct rt_16550_context *)context->dev_private;
 
     ret = rtdm_irq_request(&ctx->irq_handle, irq[dev_id], rt_16550_interrupt,
-                           0, context->device->proc_name, ctx);
+                           RTDM_IRQTYPE_SHARED|RTDM_IRQTYPE_EDGE,
+                           context->device->proc_name, ctx);
     if (ret < 0)
         return ret;
 

[-- Attachment #1.3: shirq-timer-name.patch --]
[-- Type: text/plain, Size: 513 bytes --]

--- xenomai/ksrc/nucleus/pod.c.orig	2006-02-10 14:30:32.000000000 +0100
+++ xenomai/ksrc/nucleus/pod.c	2006-02-10 14:30:49.000000000 +0100
@@ -3071,7 +3071,7 @@ unlock_and_exit:
        source will be attached directly by the arch-dependent layer
        (xnarch_start_timer). */
 
-    xnintr_init(&nkclock,NULL,XNARCH_TIMER_IRQ,tickhandler,NULL,0);
+    xnintr_init(&nkclock,"[timer]",XNARCH_TIMER_IRQ,tickhandler,NULL,0);
     xnintr_clock_attach(&nkclock);
 
     __setbits(nkpod->status,XNTIMED);

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

[Xenomai-core] Re: [shirq] first test results

[Dmitry Adamushko]

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

Hi Jan,

> some news from the testing front: It works fairly well - and it doesn't
> crash =:).

Good news indeed :)

> We set up a quite demanding test scenario which consists of
> two Sick Laser scanners feeding two UART ports at 500 Kbit/s. The UARTs
> are on a special PC104 card, sharing the same edge-triggered IRQ line.
> We were able to get data from both devices running at the same time. But
> we noticed some overruns of MAX_EDGEIRQ_COUNTER (a few per second). The
> next step on Monday will be to generate a back-trace with the
> ipipe-tracer to see if the system is "just" overloaded or if we are
> still facing problems with the driver and/or IRQ layer. Will be very
> interesting to see on that radar what's happing.

Yes.

And as an additional option,
it could be interesting to print out to the log if not all "counter" values
then min,max,average (the same like for the latency :) per second or per
1000 interrupts; so to see whether tweaking the MAX_EDGEIRQ_COUNTER may make
the things better.


> I attached two patches. One enables xeno_16550A to use the new features,
> and the other improves the /proc output of your patch slightly.
>
> Furthermore, we noticed that virtual IRQs (namely the printk forwarder)
> get displayed under /proc/xenomai/irq too. Is this useful? We wondered
> what IRQ 34 might be until code analysis of Xenomai and Ipipe revealed
> it (__ipipe_printk_virq). If it is considered useful, we should at least
> mark those irqs virtual in the output or even give them names as well.

It's a virq but it can not be __ipipe_printk_virq :)
The latter one is registered in the linux domain while we are
printing out the irqs of the primary one in /proc/xenomai/irq.

It should be xnarch_escalation_virq.

Probably, BASE_VIRQ == 32 (if NR_IRQS == 16).

So,

32 -    __ipipe_printk_virq (Linux domain)
33 -    rthal_apc_virq (Linux domain)
34 -    xnarch_escalation_virq (Xeno domain)

They are ordered (that's why they have such irq numbers) as
ipipe_alloc_virq()
is called for them.

That can be confirmed by :

# cat /proc/ipipe/Xenomai

...
irq 32-33: passed, virtual
irq 34: grabbed, virtual    <--- 1 virq is in the primary domain
...

# cat /proc/ipipe/Linux

irq 32-33: grabbed, virtual    <--- 2 virqs are in the linux domain
irq 34: passed, virtual


Yep, the output is confusing. At least, we may :

1) add "virq" or "virtual" quilification, smth like 34 (virtual);

2) not print them out at all.

Maybe we'd better go for 1, just in case somebody would like to know
the actual number of virq occuried. Although, I can imagine that only
for debugging reasons and of no avail for the normal users, so maybe 2 :)


> Then I stumbled over the xnintr structure. Why do you keep a copy of the
> device name?
> A "const char *" should be enough, we just have to demand
> that it will remain valid as long as the xnintr structure itself (i.e.
> during the IRQ being attached). Saves a few bytes. :)

Could be if all the users of it are initially kernel-mode entities.

But e.g. there is a version of rt_intr_create() that may be called by
user-space
apps and I extended it to support the "name" parameter too :

int rt_intr_create(RT_INTR *intr, const char *name, unsigned irq, int mode);

In such a case, the kernel-side rt_intr_create() is called with the "name"
allocated on the stack in skins/native/syscall.c : __rt_intr_create())
so the initial copy of the name can not remain valid.


Thanks for the patches.


> Jan

--
Best regards,
Dmitry Adamushko

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

[Xenomai-core] Re: [shirq] first test results

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> Hi Jan,
> 
>> some news from the testing front: It works fairly well - and it doesn't
>> crash =:).
> 
> Good news indeed :)
> 
>> We set up a quite demanding test scenario which consists of
>> two Sick Laser scanners feeding two UART ports at 500 Kbit/s. The UARTs
>> are on a special PC104 card, sharing the same edge-triggered IRQ line.
>> We were able to get data from both devices running at the same time. But
>> we noticed some overruns of MAX_EDGEIRQ_COUNTER (a few per second). The
>> next step on Monday will be to generate a back-trace with the
>> ipipe-tracer to see if the system is "just" overloaded or if we are
>> still facing problems with the driver and/or IRQ layer. Will be very
>> interesting to see on that radar what's happing.
> 
> Yes.
> 
> And as an additional option,
> it could be interesting to print out to the log if not all "counter" values
> then min,max,average (the same like for the latency :) per second or per
> 1000 interrupts; so to see whether tweaking the MAX_EDGEIRQ_COUNTER may make
> the things better.

Yes, maybe it's too small. But this also depends on the absolute time
required for so many loops, something we should see in the traces then.
I'm afraid that we will finally have to move the UART's read-from-fifo
to task context to reduce the time spent in IRQ context.

> 
> 
>> I attached two patches. One enables xeno_16550A to use the new features,
>> and the other improves the /proc output of your patch slightly.
>>
>> Furthermore, we noticed that virtual IRQs (namely the printk forwarder)
>> get displayed under /proc/xenomai/irq too. Is this useful? We wondered
>> what IRQ 34 might be until code analysis of Xenomai and Ipipe revealed
>> it (__ipipe_printk_virq). If it is considered useful, we should at least
>> mark those irqs virtual in the output or even give them names as well.
> 
> It's a virq but it can not be __ipipe_printk_virq :)
> The latter one is registered in the linux domain while we are
> printing out the irqs of the primary one in /proc/xenomai/irq.
> 
> It should be xnarch_escalation_virq.
> 
> Probably, BASE_VIRQ == 32 (if NR_IRQS == 16).
> 
> So,
> 
> 32 -    __ipipe_printk_virq (Linux domain)
> 33 -    rthal_apc_virq (Linux domain)
> 34 -    xnarch_escalation_virq (Xeno domain)

Ok, I stopped digging deeper after noticing the first VIRQ being
registered to the dumped IRQ list.

> 
> They are ordered (that's why they have such irq numbers) as
> ipipe_alloc_virq()
> is called for them.
> 
> That can be confirmed by :
> 
> # cat /proc/ipipe/Xenomai
> 
> ...
> irq 32-33: passed, virtual
> irq 34: grabbed, virtual    <--- 1 virq is in the primary domain
> ...
> 
> # cat /proc/ipipe/Linux
> 
> irq 32-33: grabbed, virtual    <--- 2 virqs are in the linux domain
> irq 34: passed, virtual

Oh, yes. The information was so close and yet I didn't see it. :)

> 
> 
> Yep, the output is confusing. At least, we may :
> 
> 1) add "virq" or "virtual" quilification, smth like 34 (virtual);
> 
> 2) not print them out at all.
> 
> Maybe we'd better go for 1, just in case somebody would like to know
> the actual number of virq occuried. Although, I can imagine that only
> for debugging reasons and of no avail for the normal users, so maybe 2 :)

I have no final opinion yet. Having some names also behind the virtual
IRQs would be really nice for debugging, but it would also break another
bulk of API calls. Just marking those IRQs virtual would render the
related output of /proc/xenomai/irq not very useful in my eye - as long
as you do not know what's behind it. Just dropping this information is
then likely better.

> 
> 
>> Then I stumbled over the xnintr structure. Why do you keep a copy of the
>> device name?
>> A "const char *" should be enough, we just have to demand
>> that it will remain valid as long as the xnintr structure itself (i.e.
>> during the IRQ being attached). Saves a few bytes. :)
> 
> Could be if all the users of it are initially kernel-mode entities.
> 
> But e.g. there is a version of rt_intr_create() that may be called by
> user-space
> apps and I extended it to support the "name" parameter too :
> 
> int rt_intr_create(RT_INTR *intr, const char *name, unsigned irq, int mode);
> 
> In such a case, the kernel-side rt_intr_create() is called with the "name"
> allocated on the stack in skins/native/syscall.c : __rt_intr_create())
> so the initial copy of the name can not remain valid.

But you could create buffer space for a copy of the name in the same
block already allocated for RT_INTR. Should be straightforward.

Moreover, but that's a different topic, I would vote for

config XENO_OPT_NATIVE_INTR
        default n

That feature is not needed when following the (preferred) RTDM path, and
is therefore only ballast to xeno_native on most systems. Guess I have
to prepare a patch...

Jan


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

[Xenomai-core] Re: [shirq] first test results

[Dmitry Adamushko]

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

On 11/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> > And as an additional option,
> > it could be interesting to print out to the log if not all "counter"
> values
> > then min,max,average (the same like for the latency :) per second or per
> > 1000 interrupts; so to see whether tweaking the MAX_EDGEIRQ_COUNTER may
> make
> > the things better.
>
> Yes, maybe it's too small. But this also depends on the absolute time
> required for so many loops, something we should see in the traces then.
> I'm afraid that we will finally have to move the UART's read-from-fifo
> to task context to reduce the time spent in IRQ context.



Good. Keep me informed as before.

> Maybe we'd better go for 1, just in case somebody would like to know
> > the actual number of virq occuried. Although, I can imagine that only
> > for debugging reasons and of no avail for the normal users, so maybe 2
> :)
>
> I have no final opinion yet. Having some names also behind the virtual
> IRQs would be really nice for debugging, but it would also break another
> bulk of API calls. Just marking those IRQs virtual would render the
> related output of /proc/xenomai/irq not very useful in my eye - as long
> as you do not know what's behind it. Just dropping this information is
> then likely better.


Yep. Let's just avoid printing it.


>> Then I stumbled over the xnintr structure. Why do you keep a copy of the
> >> device name?
> >> A "const char *" should be enough, we just have to demand
> >> that it will remain valid as long as the xnintr structure itself (i.e.
> >> during the IRQ being attached). Saves a few bytes. :)
> >
> > Could be if all the users of it are initially kernel-mode entities.
> >
> > But e.g. there is a version of rt_intr_create() that may be called by
> > user-space
> > apps and I extended it to support the "name" parameter too :
> >
> > int rt_intr_create(RT_INTR *intr, const char *name, unsigned irq, int
> mode);
> >
> > In such a case, the kernel-side rt_intr_create() is called with the
> "name"
> > allocated on the stack in skins/native/syscall.c : __rt_intr_create())
> > so the initial copy of the name can not remain valid.
>
> But you could create buffer space for a copy of the name in the same
> block already allocated for RT_INTR. Should be straightforward.


RT_INTR does not have the "name" field any more. I have removed it after
adding the same field to the xnintr_t as it became redundant.

To access the "name", any code in native skin does "intr->intr_base.name"
(intr_base is of "xnintr_t" type).


Jan
>
>
>
>

--
Best regards,
Dmitry Adamushko

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

[Xenomai-core] Re: [shirq] first test results

[Jan Kiszka]

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

Dmitry Adamushko wrote:
>>> Then I stumbled over the xnintr structure. Why do you keep a copy of the
>>>> device name?
>>>> A "const char *" should be enough, we just have to demand
>>>> that it will remain valid as long as the xnintr structure itself (i.e.
>>>> during the IRQ being attached). Saves a few bytes. :)
>>> Could be if all the users of it are initially kernel-mode entities.
>>>
>>> But e.g. there is a version of rt_intr_create() that may be called by
>>> user-space
>>> apps and I extended it to support the "name" parameter too :
>>>
>>> int rt_intr_create(RT_INTR *intr, const char *name, unsigned irq, int
>> mode);
>>> In such a case, the kernel-side rt_intr_create() is called with the
>> "name"
>>> allocated on the stack in skins/native/syscall.c : __rt_intr_create())
>>> so the initial copy of the name can not remain valid.
>> But you could create buffer space for a copy of the name in the same
>> block already allocated for RT_INTR. Should be straightforward.
> 
> 
> RT_INTR does not have the "name" field any more. I have removed it after
> adding the same field to the xnintr_t as it became redundant.
> 
> To access the "name", any code in native skin does "intr->intr_base.name"
> (intr_base is of "xnintr_t" type).
> 

I was rather thinking of allocating RT_INTR + some additional space to
take the name passed from user-space. Then intr_base.name could point to
this buffer instead of static memory in the kernel use case.

Jan


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

Re: [Xenomai-core] Re: [shirq] first test results

[Jan Kiszka]

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

Dmitry Adamushko wrote:
> On 11/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>> Dmitry Adamushko wrote:
>>> And as an additional option,
>>> it could be interesting to print out to the log if not all "counter"
>> values
>>> then min,max,average (the same like for the latency :) per second or per
>>> 1000 interrupts; so to see whether tweaking the MAX_EDGEIRQ_COUNTER may
>> make
>>> the things better.
>> Yes, maybe it's too small. But this also depends on the absolute time
>> required for so many loops, something we should see in the traces then.
>> I'm afraid that we will finally have to move the UART's read-from-fifo
>> to task context to reduce the time spent in IRQ context.
> 
> Good. Keep me informed as before.
> 

We got it working. :)

First, we had to fix our own stupidity: the reception fifo depth of the
serial ports were set to one, thus we basically got one IRQ every 20 us.
Of course the CPU was too slow for this storm and therefore we got the
overruns. After increasing this threshold to 14, things improved
significantly - but also the worst-case IRQ-off time. Well, that's a
trade-off we likely have to make somehow between low IRQ load and long
IRQ-off times. Reducing the threshold to 4 e.g. gives shorter IRQ-off
times and still no overruns but also causes higher system load. This all
requires some more thoughts on the xeno_16550 design (IRQ handler vs.
user task for receiving and sending), will see when I find time for it.

Anyway, this works now. But we also found a bug, as usual, a clean-up
bug: You released the IRQ line based on the wrong test, see attached
patch. With this fix applied and the IRQ flags as well as the
/proc/xenomai/irq output cleaned up, I would say you great work is ready
for merge!

Thanks,
Jan

[-- Attachment #1.2: shirq-detach.patch --]
[-- Type: text/plain, Size: 589 bytes --]

--- ksrc/nucleus/intr.c.orig	2006-02-13 11:15:45.000000000 +0100
+++ ksrc/nucleus/intr.c	2006-02-13 19:22:57.000000000 +0100
@@ -393,9 +393,12 @@ int xnintr_detach (xnintr_t *intr)
 	if (e == intr)
 	    {
 	    /* Remove a given interrupt object from the list. */
-	    if ((*p = e->next) == NULL)
+	    *p = e->next;
+
+	    /* Release IRQ line if this was the last user */
+	    if (shirq->handlers == NULL)
 		err = xnarch_release_irq(intr->irq);
-    
+
 	    xnarch_critical_exit(flags);
 
 	    /* The idea here is to keep a detached interrupt object valid as long

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

Re: [Xenomai-core] Re: [shirq] first test results

[Dmitry Adamushko]

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

On 13/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
>
> Dmitry Adamushko wrote:
> > On 11/02/06, Jan Kiszka <jan.kiszka@domain.hid> wrote:
> >> Dmitry Adamushko wrote:
> >>> And as an additional option,
> >>> it could be interesting to print out to the log if not all "counter"
> >> values
> >>> then min,max,average (the same like for the latency :) per second or
> per
> >>> 1000 interrupts; so to see whether tweaking the MAX_EDGEIRQ_COUNTER
> may
> >> make
> >>> the things better.
> >> Yes, maybe it's too small. But this also depends on the absolute time
> >> required for so many loops, something we should see in the traces then.
> >> I'm afraid that we will finally have to move the UART's read-from-fifo
> >> to task context to reduce the time spent in IRQ context.
> >
> > Good. Keep me informed as before.
> >
>
> We got it working. :)


Great!


> Anyway, this works now. But we also found a bug, as usual, a clean-up
> bug: You released the IRQ line based on the wrong test, see attached
> patch.


Ah... it worked for my test cases since it depended on the order of
xnintr_detach() calls. Thanks.


With this fix applied and the IRQ flags as well as the
> /proc/xenomai/irq output cleaned up, I would say you great work is ready
> for merge!


So I'll create the final patch and submit it to Philippe for merging.
One more thing is making all this stuff optional; XENO_OPT_SHIRQ and
XENO_OPT_SHIRQ_EDGE or something like this.


Thanks,
> Jan
>
>

--
Best regards,
Dmitry Adamushko

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