[PATCH] Debug shared irqs.

[PATCH] Debug shared irqs.

[David Woodhouse]

Drivers registering IRQ handlers with SA_SHIRQ really ought to be able
to handle an interrupt happening before request_irq() returns. They also
ought to be able to handle an interrupt happening during the start of
their call to free_irq(). Let's test that hypothesis....

Signed-off-by: David Woodhouse <dwmw2@infradead.org>

I'm not wonderfully happy with the faked pt_regs, but I think it ought
to be OK like that.

--- linux-2.6.14/lib//Kconfig.debug~	2005-12-19 00:57:18.000000000 +0000
+++ linux-2.6.14/lib//Kconfig.debug	2005-12-22 11:37:27.000000000 +0000
@@ -176,6 +176,15 @@ config DEBUG_VM
 
 	  If unsure, say N.
 
+config SHARE_ME_HARDER
+       bool "Debug shared IRQ handlers"
+       depends on GENERIC_HARDIRQS
+       help
+         Enable this to generate a spurious interrupt as soon as a shared interrupt
+	 handler is registered, and just before one is deregistered. Drivers ought
+	 to be able to handle interrupts coming in at those some; some don't and
+	 need to be caught.
+
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	depends on DEBUG_KERNEL && (X86 || CRIS || M68K || M68KNOMMU || FRV || UML)
--- linux-2.6.14/kernel/irq/manage.c~	2005-12-19 00:57:18.000000000 +0000
+++ linux-2.6.14/kernel/irq/manage.c	2005-12-22 11:24:40.000000000 +0000
@@ -238,6 +238,10 @@ int setup_irq(unsigned int irq, struct i
 	return 0;
 }
 
+#ifdef CONFIG_SHARE_ME_HARDER
+static struct pt_regs shirq_fakeregs;
+#endif
+
 /**
  *	free_irq - free an interrupt
  *	@irq: Interrupt line to free
@@ -257,6 +261,7 @@ void free_irq(unsigned int irq, void *de
 	struct irq_desc *desc;
 	struct irqaction **p;
 	unsigned long flags;
+	irqreturn_t (*handler)(int, void *, struct pt_regs *) = NULL;
 
 	if (irq >= NR_IRQS)
 		return;
@@ -295,6 +300,8 @@ void free_irq(unsigned int irq, void *de
 
 			/* Make sure it's not being used on another CPU */
 			synchronize_irq(irq);
+			if (action->flags & SA_SHIRQ)
+				handler = action->handler;
 			kfree(action);
 			return;
 		}
@@ -302,6 +309,15 @@ void free_irq(unsigned int irq, void *de
 		spin_unlock_irqrestore(&desc->lock,flags);
 		return;
 	}
+#ifdef CONFIG_SHARE_ME_HARDER
+	if (handler) {
+		/* It's a shared IRQ -- the driver ought to be prepared for it to happen
+		   even now it's being freed, so let's make sure.... 
+		   We do this after actually deregistering it, to make sure that a 'real'
+		   IRQ doesn't run in parallel with our fake. */
+		handler(irq, dev_id, &shirq_fakeregs);
+	}
+#endif
 }
 
 EXPORT_SYMBOL(free_irq);
@@ -366,6 +382,16 @@ int request_irq(unsigned int irq,
 	action->next = NULL;
 	action->dev_id = dev_id;
 
+#ifdef CONFIG_SHARE_ME_HARDER
+	if (irqflags & SA_SHIRQ) {
+		/* It's a shared IRQ -- the driver ought to be prepared for it to happen
+		   immediately, so let's make sure.... 
+		   We do this before actually registering it, to make sure that a 'real'
+		   IRQ doesn't run in parallel with our fake. */
+		handler(irq, dev_id, &shirq_fakeregs);
+	}
+#endif
+
 	retval = setup_irq(irq, action);
 	if (retval)
 		kfree(action);


-- 
dwmw2

Re: [PATCH] Debug shared irqs.

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
>  Drivers registering IRQ handlers with SA_SHIRQ really ought to be able
>  to handle an interrupt happening before request_irq() returns. They also
>  ought to be able to handle an interrupt happening during the start of
>  their call to free_irq(). Let's test that hypothesis....

I don't think I like this patch.

diff -puN sound/oss/i810_audio.c~i810_audio-request_irq-fix sound/oss/i810_audio.c
--- 25/sound/oss/i810_audio.c~i810_audio-request_irq-fix	2006-01-05 02:16:04.000000000 -0800
+++ 25-akpm/sound/oss/i810_audio.c	2006-01-05 02:16:58.000000000 -0800
@@ -3360,12 +3360,6 @@ static int __devinit i810_probe(struct p
 		goto out_region2;
 	}
 
-	if (request_irq(card->irq, &i810_interrupt, SA_SHIRQ,
-			card_names[pci_id->driver_data], card)) {
-		printk(KERN_ERR "i810_audio: unable to allocate irq %d\n", card->irq);
-		goto out_pio;
-	}
-
 	if (card->use_mmio) {
 		if (request_mem_region(card->ac97base_mmio_phys, 512, "ich_audio MMBAR")) {
 			if ((card->ac97base_mmio = ioremap(card->ac97base_mmio_phys, 512))) { /*@FIXME can ioremap fail? don't know (jsaw) */
@@ -3396,10 +3390,8 @@ static int __devinit i810_probe(struct p
 	}
 
 	/* initialize AC97 codec and register /dev/mixer */
-	if (i810_ac97_init(card) <= 0) {
-		free_irq(card->irq, card);
+	if (i810_ac97_init(card) <= 0)
 		goto out_iospace;
-	}
 	pci_set_drvdata(pci_dev, card);
 
 	if(clocking == 0) {
@@ -3411,7 +3403,6 @@ static int __devinit i810_probe(struct p
 	if ((card->dev_audio = register_sound_dsp(&i810_audio_fops, -1)) < 0) {
 		int i;
 		printk(KERN_ERR "i810_audio: couldn't register DSP device!\n");
-		free_irq(card->irq, card);
 		for (i = 0; i < NR_AC97; i++)
 		if (card->ac97_codec[i] != NULL) {
 			unregister_sound_mixer(card->ac97_codec[i]->dev_mixer);
@@ -3420,6 +3411,13 @@ static int __devinit i810_probe(struct p
 		goto out_iospace;
 	}
 
+	if (request_irq(card->irq, &i810_interrupt, SA_SHIRQ,
+			card_names[pci_id->driver_data], card)) {
+		printk(KERN_ERR "i810_audio: unable to allocate irq %d\n", card->irq);
+		goto out_iospace;
+	}
+
+
  	card->initializing = 0;
 	return 0;
 
_


This is going to cause me a ton of grief.  How's about you put it in
Fedora for a few weeks, get all the drivers debugged first ;)

Re: [PATCH] Debug shared irqs.

[David Woodhouse]

On Thu, 2006-01-05 at 02:19 -0800, Andrew Morton wrote:
> This is going to cause me a ton of grief.  How's about you put it in
> Fedora for a few weeks, get all the drivers debugged first ;)

I'd do that normally, but it's the wrong point in the cycle -- we're
getting ready for Fedora Core 5 at the moment; it's not the time to be
doing such things. We can apply the patch... but we'd have to turn the
config option off :)

What you're seeing is the whole point of the patch, surely? And it _is_
a config option -- people aren't forced to turn it on.

Would it help if we added a printk to make it more obvious what's
happening, which gives the naïve user the opportunity to turn off the
config option just to get things working again? Somethign along the
lines of "Faking irq %d due to CONFIG_DEBUG_SHIRQ. If your machine
crashes now, don't blame akpm"?

Out of interest, does your i810 patch fix the problem which was reported
in November by Eyal Lebedinsky ("2.6.14.2: repeated oops in i810 init")?

-- 
dwmw2

Re: [PATCH] Debug shared irqs.

[Arjan van de Ven]

On Thu, 2006-01-05 at 02:19 -0800, Andrew Morton wrote:

> 
> This is going to cause me a ton of grief.  How's about you put it in
> Fedora for a few weeks, get all the drivers debugged first ;)

well it's a config option for a reason :)
Also it's something driver writers now can turn on so that THEY can
debug their driver as well.... -mm or even mainline is better for that
than fedora.

Re: [PATCH] Debug shared irqs.

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Thu, 2006-01-05 at 02:19 -0800, Andrew Morton wrote:
> > This is going to cause me a ton of grief.  How's about you put it in
> > Fedora for a few weeks, get all the drivers debugged first ;)
> 
> I'd do that normally, but it's the wrong point in the cycle -- we're
> getting ready for Fedora Core 5 at the moment; it's not the time to be
> doing such things. We can apply the patch... but we'd have to turn the
> config option off :)
> 
> What you're seeing is the whole point of the patch, surely? And it _is_
> a config option -- people aren't forced to turn it on.

Sure.  I was just regretting being the sucker again.

> Would it help if we added a printk to make it more obvious what's
> happening, which gives the naïve user the opportunity to turn off the
> config option just to get things working again? Somethign along the
> lines of "Faking irq %d due to CONFIG_DEBUG_SHIRQ. If your machine
> crashes now, don't blame akpm"?

Nah, that's all right.  I'm skilled at sending maintainers new bug reports
to ignore.

> Out of interest, does your i810 patch fix the problem which was reported
> in November by Eyal Lebedinsky ("2.6.14.2: repeated oops in i810 init")?

Looks like it.  I was oopsing at 0x00000030 too, although on x86_64, not x86.

[PATCH] reduce sizeof(percpu_data) and removes dependance against NR_CPUS

[Eric Dumazet]

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

Hi Andrew

Current sizeof(percpu_data) is NR_CPUS*sizeof(void *)

This trivial patch makes percpu_data real size depends on 
highest_possible_processor_id() instead of NR_CPUS

percpu_data allocations are not performance critical, we can spend few CPU 
cycles and save some ram.

This patch should replace remove-unused-blkp-field-in-percpu_data.patch in mm tree

Thank you

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


[-- Attachment #2: shrink_percpu_data.patch --]
[-- Type: text/plain, Size: 783 bytes --]

--- linux-2.6.15/include/linux/percpu.h	2006-01-03 04:21:10.000000000 +0100
+++ linux-2.6.15-edum/include/linux/percpu.h	2006-01-05 14:45:48.000000000 +0100
@@ -18,8 +18,7 @@
 #ifdef CONFIG_SMP
 
 struct percpu_data {
-	void *ptrs[NR_CPUS];
-	void *blkp;
+	void *ptrs[1]; /* real size depends on highest_possible_processor_id() */
 };
 
 /* 
--- linux-2.6.15/mm/slab.c	2006-01-03 04:21:10.000000000 +0100
+++ linux-2.6.15-edum/mm/slab.c	2006-01-05 14:37:13.000000000 +0100
@@ -2949,7 +2949,8 @@
 void *__alloc_percpu(size_t size, size_t align)
 {
 	int i;
-	struct percpu_data *pdata = kmalloc(sizeof (*pdata), GFP_KERNEL);
+	size_t pdsize = highest_possible_processor_id() * sizeof(void *);
+	struct percpu_data *pdata = kmalloc(pdsize, GFP_KERNEL);
 
 	if (!pdata)
 		return NULL;

Re: [PATCH] reduce sizeof(percpu_data) and removes dependance against NR_CPUS

[Andrew Morton]

Eric Dumazet <dada1@cosmosbay.com> wrote:
>
> Current sizeof(percpu_data) is NR_CPUS*sizeof(void *)
> 
>  This trivial patch makes percpu_data real size depends on 
>  highest_possible_processor_id() instead of NR_CPUS
> 
>  percpu_data allocations are not performance critical, we can spend few CPU 
>  cycles and save some ram.

hm, highest_possible_processor_id() isn't very efficient.  And it's quite
dopey that it's a macro.  We should turn it into a real function which
caches its return result and goes BUG if it's called before
cpu_possible_map is initialised.

Re: [PATCH] reduce sizeof(percpu_data) and removes dependance against NR_CPUS

[Eric Dumazet]

Andrew Morton a écrit :
> Eric Dumazet <dada1@cosmosbay.com> wrote:
>> Current sizeof(percpu_data) is NR_CPUS*sizeof(void *)
>>
>>  This trivial patch makes percpu_data real size depends on 
>>  highest_possible_processor_id() instead of NR_CPUS
>>
>>  percpu_data allocations are not performance critical, we can spend few CPU 
>>  cycles and save some ram.
> 
> hm, highest_possible_processor_id() isn't very efficient.  And it's quite
> dopey that it's a macro.  We should turn it into a real function which
> caches its return result and goes BUG if it's called before
> cpu_possible_map is initialised.

I agree, I will see if I can do that without poking in every arch :(

More over, my patch has an error on pdsize computation, it should be :

size_t pdsize = (highest_possible_processor_id() + 1) * sizeof(void *);

Eric

Re: [PATCH] reduce sizeof(percpu_data) and removes dependance against NR_CPUS

[Jan Engelhardt]

--- linux-2.6.15/include/linux/percpu.h	2006-01-03 04:21:10.000000000 +0100
+++ linux-2.6.15-edum/include/linux/percpu.h	2006-01-05 14:45:48.000000000 +0100
@@ -18,8 +18,7 @@
 #ifdef CONFIG_SMP
 
 struct percpu_data {
-	void *ptrs[NR_CPUS];
-	void *blkp;
+	void *ptrs[1]; /* real size depends on highest_possible_processor_id() */
 };
 
 /* 


I think we can use *ptrs[0] here.



Jan Engelhardt
-- 
| Alphagate Systems, http://alphagate.hopto.org/
| jengelh's site, http://jengelh.hopto.org/

Re: [PATCH] reduce sizeof(percpu_data) and removes dependance against NR_CPUS

[Andrew Morton]

Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:
>
> 
> 
> --- linux-2.6.15/include/linux/percpu.h	2006-01-03 04:21:10.000000000 +0100
> +++ linux-2.6.15-edum/include/linux/percpu.h	2006-01-05 14:45:48.000000000 +0100
> @@ -18,8 +18,7 @@
>  #ifdef CONFIG_SMP
>  
>  struct percpu_data {
> -	void *ptrs[NR_CPUS];
> -	void *blkp;
> +	void *ptrs[1]; /* real size depends on highest_possible_processor_id() */
>  };
>  
>  /* 
> 
> 
> I think we can use *ptrs[0] here.
> 

We can.  In fact with the gcc-2.95 abandonment I think we can use ptrs[].

But it doesn't matter.

Re: [PATCH] reduce sizeof(percpu_data) and removes dependance against NR_CPUS

[Eric Dumazet]

Jan Engelhardt a écrit :
> 
> --- linux-2.6.15/include/linux/percpu.h	2006-01-03 04:21:10.000000000 +0100
> +++ linux-2.6.15-edum/include/linux/percpu.h	2006-01-05 14:45:48.000000000 +0100
> @@ -18,8 +18,7 @@
>  #ifdef CONFIG_SMP
>  
>  struct percpu_data {
> -	void *ptrs[NR_CPUS];
> -	void *blkp;
> +	void *ptrs[1]; /* real size depends on highest_possible_processor_id() */
>  };
>  
>  /* 
> 
> 
> I think we can use *ptrs[0] here.
> 

Or even dont change percpu.h, or else some guys will assume percpu_data cost 
nothing...

Eric