[Adeos-main] [SMP-BUG?] inconsistent cpuid after dispatch_wired

[Adeos-main] [SMP-BUG?] inconsistent cpuid after dispatch_wired

[Jan Kiszka]

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

Hi Philippe,

actually, I was trying to apply a micro-optimisation on
__ipipe_handle_irq, but I think I also found a bug on SMP systems (at
least on x86):

After __ipipe_handle_irq forwarded some IRQ to __ipipe_dispatch_wired
and the latter returned 1 (i.e. "not deferred"), a pipeline sync is
triggered using the cpuid read on IRQ entry. But I think that
__ipipe_dispatch_wired may very well have caused some CPU migration of
the current context so that reloading the cpuid is required here. This
is what the first attached patch does.

Jan


PS: The optimisation I was looking at deals with the assumption that a
wired IRQ may never be also a sticky one, correct? If yes, the second
(alternative) patch might be interesting as it avoids to touch the root
domain data structure and to test for stickiness in the wired case.
Should shorten the wired IRQ path by a few cycles (or more on cache
miss...).

[-- Attachment #1.2: reload-cpuid-after-wired-dispatch.patch --]
[-- Type: text/plain, Size: 686 bytes --]

Index: linux-2.6.19/arch/i386/kernel/ipipe.c
===================================================================
--- linux-2.6.19.orig/arch/i386/kernel/ipipe.c
+++ linux-2.6.19/arch/i386/kernel/ipipe.c
@@ -790,9 +790,10 @@ int __ipipe_handle_irq(struct pt_regs re
 		if (likely(test_bit(IPIPE_WIRED_FLAG, &next_domain->irqs[irq].control))) {
 			if (!m_ack && next_domain->irqs[irq].acknowledge != NULL)
 				next_domain->irqs[irq].acknowledge(irq);
-			if (likely(__ipipe_dispatch_wired(next_domain, irq)))
+			if (likely(__ipipe_dispatch_wired(next_domain, irq))) {
+				ipipe_load_cpuid();
 				goto finalize;
-			else
+			} else
 				goto finalize_nosync;
 		}
 	}

[-- Attachment #1.3: optimise-wired-irq-path.patch --]
[-- Type: text/plain, Size: 1504 bytes --]

Index: linux-2.6.19/arch/i386/kernel/ipipe.c
===================================================================
--- linux-2.6.19.orig/arch/i386/kernel/ipipe.c
+++ linux-2.6.19/arch/i386/kernel/ipipe.c
@@ -772,30 +772,30 @@ int __ipipe_handle_irq(struct pt_regs re
 	ipipe_declare_cpuid;
 	int m_ack;
 
-	ipipe_load_cpuid();
-
 	if (regs.orig_eax < 0) {
 		irq = ~irq;
 		m_ack = 0;
 	} else
 		m_ack = 1;
 
+	head = __ipipe_pipeline.next;
+	next_domain = list_entry(head, struct ipipe_domain, p_link);
+	if (likely(test_bit(IPIPE_WIRED_FLAG, &next_domain->irqs[irq].control))) {
+		if (!m_ack && next_domain->irqs[irq].acknowledge != NULL)
+			next_domain->irqs[irq].acknowledge(irq);
+		if (likely(__ipipe_dispatch_wired(next_domain, irq))) {
+			ipipe_load_cpuid();
+			goto finalize;
+		} else
+			goto finalize_nosync;
+	}
+
+	ipipe_load_cpuid();
+
 	this_domain = per_cpu(ipipe_percpu_domain, cpuid);
 
 	if (test_bit(IPIPE_STICKY_FLAG, &this_domain->irqs[irq].control))
 		head = &this_domain->p_link;
-	else {
-		head = __ipipe_pipeline.next;
-		next_domain = list_entry(head, struct ipipe_domain, p_link);
-		if (likely(test_bit(IPIPE_WIRED_FLAG, &next_domain->irqs[irq].control))) {
-			if (!m_ack && next_domain->irqs[irq].acknowledge != NULL)
-				next_domain->irqs[irq].acknowledge(irq);
-			if (likely(__ipipe_dispatch_wired(next_domain, irq)))
-				goto finalize;
-			else
-				goto finalize_nosync;
-		}
-	}
 
 	/* Ack the interrupt. */
 

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

[Adeos-main] Re: [SMP-BUG?] inconsistent cpuid after dispatch_wired

[Philippe Gerum]

On Mon, 2006-12-18 at 13:10 +0100, Jan Kiszka wrote:
> Hi Philippe,
> 
> actually, I was trying to apply a micro-optimisation on
> __ipipe_handle_irq, but I think I also found a bug on SMP systems (at
> least on x86):
> 
> After __ipipe_handle_irq forwarded some IRQ to __ipipe_dispatch_wired
> and the latter returned 1 (i.e. "not deferred"), a pipeline sync is
> triggered using the cpuid read on IRQ entry. But I think that
> __ipipe_dispatch_wired may very well have caused some CPU migration of
> the current context so that reloading the cpuid is required here. This
> is what the first attached patch does.
> 

Correct. Applied, thanks.

> Jan
> 
> 
> PS: The optimisation I was looking at deals with the assumption that a
> wired IRQ may never be also a sticky one, correct?

Correct.

>  If yes, the second
> (alternative) patch might be interesting as it avoids to touch the root
> domain data structure and to test for stickiness in the wired case.
> Should shorten the wired IRQ path by a few cycles (or more on cache
> miss...).

Applied, thanks.

-- 
Philippe.