[Adeos-main] [PATCH] fix irq statistics

[Adeos-main] [PATCH] fix irq statistics

[Jan Kiszka]

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

Hi,

IRQ hit counters are broken in latest I-pipe 1.10-x for the wired path. 
This patch moves the counter maintenance out of __ipipe_set_irq_pending 
and instead makes it explicit at the required spots.

Note that this patch also unexports __ipipe_set_irq_pending because I 
found no reason why it should be used outside the i-pipe core. Please 
correct me if I'm wrong on this.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

[-- Attachment #2: fix-and-refactor-irqstats.patch --]
[-- Type: text/x-patch, Size: 2384 bytes --]

---
 arch/i386/kernel/ipipe.c |    1 +
 kernel/ipipe/core.c      |    7 +++----
 2 files changed, 4 insertions(+), 4 deletions(-)

Index: linux-2.6.22-ipipe/arch/i386/kernel/ipipe.c
===================================================================
--- linux-2.6.22-ipipe.orig/arch/i386/kernel/ipipe.c
+++ linux-2.6.22-ipipe/arch/i386/kernel/ipipe.c
@@ -727,6 +727,7 @@ int __ipipe_handle_irq(struct pt_regs re
 			 * _first_ in the domain's status flags before
 			 * the PIC is unlocked.
 			 */
+			ipipe_cpudom_var(next_domain, irqall)[irq]++;
 			__ipipe_set_irq_pending(next_domain, irq);
 
 			if (!m_ack && next_domain->irqs[irq].acknowledge != NULL)
Index: linux-2.6.22-ipipe/kernel/ipipe/core.c
===================================================================
--- linux-2.6.22-ipipe.orig/kernel/ipipe/core.c
+++ linux-2.6.22-ipipe/kernel/ipipe/core.c
@@ -493,8 +493,6 @@ void fastcall __ipipe_set_irq_pending(st
 		__set_bit(level,&ipipe_cpudom_var(ipd, irqpend_himask));
 	} else
 		__set_bit(rank, &ipipe_cpudom_var(ipd, irqheld_mask)[level]);
-
-	ipipe_cpudom_var(ipd, irqall)[irq]++;
 }
 
 /* Must be called hw IRQs off. */
@@ -873,12 +871,13 @@ int fastcall __ipipe_dispatch_wired(stru
 {
 	struct ipipe_domain *old;
 
+	ipipe_cpudom_var(head_domain, irqall)[irq]++;
+
 	if (test_bit(IPIPE_LOCK_FLAG, &head_domain->irqs[irq].control)) {
 		/* If we can't process this IRQ right now, we must
 		 * mark it as held, so that it will get played during
 		 * normal log sync when the corresponding interrupt
 		 * source is eventually unlocked. */
-		ipipe_cpudom_var(head_domain, irqall)[irq]++;
 		__set_bit(irq & IPIPE_IRQ_IMASK, &ipipe_cpudom_var(head_domain, irqheld_mask)[irq >> IPIPE_IRQ_ISHIFT]);
 		return 0;
 	}
@@ -1199,6 +1198,7 @@ int fastcall __ipipe_schedule_irq(unsign
 		ipd = list_entry(ln, struct ipipe_domain, p_link);
 
 		if (test_bit(IPIPE_HANDLE_FLAG, &ipd->irqs[irq].control)) {
+			ipipe_cpudom_var(ipd, irqall)[irq]++;
 			__ipipe_set_irq_pending(ipd, irq);
 			local_irq_restore_hw(flags);
 			return 1;
@@ -1597,7 +1597,6 @@ EXPORT_SYMBOL(__ipipe_spin_unlock_irq);
 EXPORT_SYMBOL(__ipipe_spin_lock_irqsave);
 EXPORT_SYMBOL(__ipipe_spin_unlock_irqrestore);
 EXPORT_SYMBOL(__ipipe_pipeline);
-EXPORT_SYMBOL(__ipipe_set_irq_pending);
 EXPORT_SYMBOL(__ipipe_lock_irq);
 EXPORT_SYMBOL(__ipipe_unlock_irq);
 EXPORT_SYMBOL(ipipe_register_domain);

Re: [Adeos-main] [PATCH] fix irq statistics

[Philippe Gerum]

On Wed, 2007-09-19 at 10:51 +0200, Jan Kiszka wrote:
> Hi,
> 
> IRQ hit counters are broken in latest I-pipe 1.10-x for the wired path. 
> This patch moves the counter maintenance out of __ipipe_set_irq_pending 
> and instead makes it explicit at the required spots.

I would rather add the missing accounting code as below. Marking an
interrupt as pending using the __ipipe_set_irq_pending() interface
should implicitely be paired with proper accounting.

--- a/kernel/ipipe/core.c
+++ b/kernel/ipipe/core.c
@@ -786,6 +786,7 @@ int fastcall __ipipe_dispatch_wired(struct ipipe_domain *head_domain, unsigned i
 	old = ipipe_current_domain;
 	ipipe_current_domain = head_domain; /* Switch to the head domain. */
 
+	ipipe_cpudom_var(head_domain, irqall)[irq]++;
 	__set_bit(IPIPE_STALL_FLAG, &ipipe_cpudom_var(head_domain, status));
 	head_domain->irqs[irq].handler(irq, head_domain->irqs[irq].cookie); /* Call the ISR. */
 	__ipipe_run_irqtail();

> 
> Note that this patch also unexports __ipipe_set_irq_pending because I 
> found no reason why it should be used outside the i-pipe core. Please 
> correct me if I'm wrong on this.
> 

That's correct. This export became useless as soon as
__ipipe_unlock_irq() and friends moved out of line, and started fiddling
directly with the interrupt bitmap internally.

> Jan
> 
-- 
Philippe.

Re: [Adeos-main] [PATCH] fix irq statistics

[Jan Kiszka]

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

Philippe Gerum wrote:
> On Wed, 2007-09-19 at 10:51 +0200, Jan Kiszka wrote:
>> Hi,
>>
>> IRQ hit counters are broken in latest I-pipe 1.10-x for the wired path. 
>> This patch moves the counter maintenance out of __ipipe_set_irq_pending 
>> and instead makes it explicit at the required spots.
> 
> I would rather add the missing accounting code as below. Marking an
> interrupt as pending using the __ipipe_set_irq_pending() interface
> should implicitely be paired with proper accounting.

My original intention was to keep __ipipe_dispatch_wired compact.

But thinking about this again, I wonder if the whole thing makes sense
as it is implemented right now: Given that I-pipe users like Xenomai now
use this irqall counter as high-level hit counter, I'm not that sure we
should also account for IRQ events on locked lines or in case of stalled
receiver domains. Only _delivery_, ie. handler invocation, should be
counted IMO (=>__ipipe_run_isr). What do you think?

Jan


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

Re: [Adeos-main] [PATCH] fix irq statistics

[Philippe Gerum]

On Sun, 2007-09-23 at 09:00 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Wed, 2007-09-19 at 10:51 +0200, Jan Kiszka wrote:
> >> Hi,
> >>
> >> IRQ hit counters are broken in latest I-pipe 1.10-x for the wired path. 
> >> This patch moves the counter maintenance out of __ipipe_set_irq_pending 
> >> and instead makes it explicit at the required spots.
> > 
> > I would rather add the missing accounting code as below. Marking an
> > interrupt as pending using the __ipipe_set_irq_pending() interface
> > should implicitely be paired with proper accounting.
> 
> My original intention was to keep __ipipe_dispatch_wired compact.

Yes, but my intention in moving this code out of line is to gather
bitmap management and accounting.

> 
> But thinking about this again, I wonder if the whole thing makes sense
> as it is implemented right now: Given that I-pipe users like Xenomai now
> use this irqall counter as high-level hit counter, I'm not that sure we
> should also account for IRQ events on locked lines or in case of stalled
> receiver domains. Only _delivery_, ie. handler invocation, should be
> counted IMO (=>__ipipe_run_isr). What do you think?
> 

Now that the interrupt log is flat, we do not have any pending IRQ
counter anymore. So the only way to determine how many interrupts are
actually waiting for being processed is to substract the irq_all value
from a private dispatch counter maintained by the client code from its
own handlers. So regardless of what Xenomai does now, I would rather
keep this option open, so that domains may compute this information on
their own.

> Jan
> 
-- 
Philippe.