Skip to contentSkip to conversation

MSI interrupts and disable_irq

DISCUSSION Ayaz Abdulla 7 participants 23 messages Sep 28, 2007 – Oct 17, 2007

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Ayaz Abdulla

author
#0 lore raw

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

Thanks,
Ayaz

#1 lore raw
Quoted 5 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

(added linux-kernel to CC, since I think it's more of a general kernel issue)

To be brutally frank, I always thought this disable_irq() mess was a hack both ugly and fragile. This disable_irq() work that appeared in a couple net drivers was correct at the time, so I didn't feel I had the justification to reject it, but it still gave me a bad feeling.

I think the scenario you outline is an illustration of the approach's fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

Practices like forcedeth's unique locking work for a time, but it should be a warning sign any time you stray from the normal spin_lock_irqsave() method of synchronization.

Based on your report, it is certainly possible that there is a problem with MSI's desc->chip->disable() method... but I would actually recommend working around the problem by making the forcedeth locking more standardized by removing all those disable_irq() hacks.

Using spinlocks like other net drivers (note: avoid NETIF_F_LLTX drivers) has a high probability of both fixing your current problem, and giving forcedeth a more stable foundation for the long term. In my humble opinion :)

Jeff

#2 lore raw
Quoted 41 lines · On Fri, 28 Sep 2007 22:47:16 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 15 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

Quoted 2 lines

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

(added linux-kernel to CC, since I think it's more of a general kernel issue)

To be brutally frank, I always thought this disable_irq() mess was a hack both ugly and fragile. This disable_irq() work that appeared in a couple net drivers was correct at the time, so I didn't feel I had the justification to reject it, but it still gave me a bad feeling.

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

Practices like forcedeth's unique locking work for a time, but it should be a warning sign any time you stray from the normal spin_lock_irqsave() method of synchronization.

Based on your report, it is certainly possible that there is a problem with MSI's desc->chip->disable() method... but I would actually recommend working around the problem by making the forcedeth locking more standardized by removing all those disable_irq() hacks.

Quoted 5 lines

Using spinlocks like other net drivers (note: avoid NETIF_F_LLTX drivers) has a high probability of both fixing your current problem, and giving forcedeth a more stable foundation for the long term. In my humble opinion :)

I'll try and clean it up if the author doesn't get to it first.

Signature · 1 line
Stephen Hemminger <[email protected]>
#3 lore raw
Quoted 19 lines · Stephen Hemminger
Quoted 16 lines · On Fri, 28 Sep 2007 22:47:16 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 15 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

(added linux-kernel to CC, since I think it's more of a general kernel issue)

I didn't see anything in disable_irq that would cause it to fail in the suggested way. But I couldn't quite convince myself we were race free either. I didn't see anything that was specific to MSI that would cause something. But switching from level to edge triggered, and to a lower latency delivery path may have caused some behavior changes.

Quoted 11 lines
Quoted 11 lines

To be brutally frank, I always thought this disable_irq() mess was a hack both ugly and fragile. This disable_irq() work that appeared in a couple net drivers was correct at the time, so I didn't feel I had the justification to reject it, but it still gave me a bad feeling.

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

Quoted 7 lines
Quoted 5 lines

Based on your report, it is certainly possible that there is a problem with MSI's desc->chip->disable() method... but I would actually recommend working around the problem by making the forcedeth locking more standardized by removing all those disable_irq() hacks.

I'll try and clean it up if the author doesn't get to it first.

I took a look at the underlying side of this.

I don't know if the MSI capability for the forcedeth supports a mask bit or not. Mine doesn't even have a msi capability. If it doesn't support a mask bit the pci spec provides not valid way to mask the interrupt, so what we do is actually disable the msi capability. At which point we might get weird INTx interactions.

We have a similar case with ioapics and INTx that also turns a hardware level disable into a reroute to another irq command. So I'm going to take a look and see how infrequently we can use hardware level disabled.

Since it looks like hardware level disables tend to be creatively implemented I recommend using disable_irq as little as possible.

Eric

#4 lore raw
Quoted 61 lines · Eric W. Biederman
Quoted 19 lines · Stephen Hemminger
Quoted 16 lines · On Fri, 28 Sep 2007 22:47:16 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 15 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

(added linux-kernel to CC, since I think it's more of a general kernel issue)

I didn't see anything in disable_irq that would cause it to fail in the suggested way. But I couldn't quite convince myself we were race free either. I didn't see anything that was specific to MSI that would cause something. But switching from level to edge triggered, and to a lower latency delivery path may have caused some behavior changes.

Quoted 11 lines
Quoted 11 lines

To be brutally frank, I always thought this disable_irq() mess was a hack both ugly and fragile. This disable_irq() work that appeared in a couple net drivers was correct at the time, so I didn't feel I had the justification to reject it, but it still gave me a bad feeling.

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

Quoted 7 lines
Quoted 5 lines

Based on your report, it is certainly possible that there is a problem with MSI's desc->chip->disable() method... but I would actually recommend working around the problem by making the forcedeth locking more standardized by removing all those disable_irq() hacks.

I'll try and clean it up if the author doesn't get to it first.

I took a look at the underlying side of this.

I don't know if the MSI capability for the forcedeth supports a mask bit or not. Mine doesn't even have a msi capability. If it doesn't support a mask bit the pci spec provides not valid way to mask the interrupt, so what we do is actually disable the msi capability. At which point we might get weird INTx interactions.

Quoted 4 lines

We have a similar case with ioapics and INTx that also turns a hardware level disable into a reroute to another irq command. So I'm going to take a look and see how infrequently we can use hardware level disabled.

also the driver need to have seperate handlers for ioapic, msi, msi-x... current nv_nic_irq_optimized keep checking msi_flag...

also nv_enable_hw_interrupts and nv_disable_hw_interrupts is not corresponding in MSI side..

YH

#5 lore raw
In reply to Jeff Garzik
Quoted 16 lines · Jeff Garzik
Quoted 15 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

Quoted 6 lines
Quoted 2 lines

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

(added linux-kernel to CC, since I think it's more of a general kernel issue)

I wonder if the race is between soft_timer for nv_do_nic_poll from different CPUs

YH

#6 lore raw
Quoted 17 lines · Yinghai Lu
Quoted 16 lines · Jeff Garzik
Quoted 15 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

This issue repros only with MSI interrupt and not legacy INTx interrupts. Any ideas?

(added linux-kernel to CC, since I think it's more of a general kernel issue)

I wonder if the race is between soft_timer for nv_do_nic_poll from different CPUs

Interested parties should try the forcedeth patches I just posted :)

Jeff

#7 lore raw
Quoted 3 lines · Jeff Garzik

Interested parties should try the forcedeth patches I just posted :)

The patches look good, but I would still prefer to understand why the current implementation causes crashes before rewriting everything.

I've just noticed that netpoll_send_skb() calls ->hard_start_xmit() and ->poll() within local_irq_save().
Thus it seems that spin_lock_irqsave() must be used in the poll() and hard_start_xmit() callbacks, at least in nics that support POLL_CONTROLLER :-(

--
Manfred

#8 lore raw
In reply to Jeff Garzik
Quoted 3 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

--
Manfred

#9 lore raw
Quoted 17 lines · Manfred Spraul
Quoted 6 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with

    request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
    disable_irq_lockdep(dev->irq);
Quoted 5 lines

Someone around with a MSI capable board? The forcedeth driver does

    dev->irq = pci_dev->irq

in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

the request_irq==>setup_irq will make dev->irq = pci_dev->irq.

YH

#10 lore raw
Quoted 12 lines · Yinghai Lu
Quoted 6 lines · Manfred Spraul

Someone around with a MSI capable board? The forcedeth driver does

    dev->irq = pci_dev->irq

in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

the request_irq==>setup_irq will make dev->irq = pci_dev->irq.

Where is that?
Otherwise I would propose the attached patch. My board is not MSI-capable, thus I can't test it myself.

--
Manfred

drivers/net/forcedeth.c +11 -8
@@ -988,7 +988,7 @@
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			enable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
-			enable_irq(dev->irq);
+			enable_irq(np->pci_dev->irq);
 	} else {
 		enable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_RX].vector);
 		enable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_TX].vector);

@@ -1004,7 +1004,7 @@
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			disable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
-			disable_irq(dev->irq);
+			disable_irq(np->pci_dev->irq);
 	} else {
 		disable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_RX].vector);
 		disable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_TX].vector);

@@ -1601,7 +1601,7 @@
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			disable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
-			disable_irq(dev->irq);
+			disable_irq(np->pci_dev->irq);
 	} else {
 		disable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_RX].vector);
 	}

@@ -1619,7 +1619,7 @@
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			enable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
-			enable_irq(dev->irq);
+			enable_irq(np->pci_dev->irq);
 	} else {
 		enable_irq(np->msi_x_entry[NV_MSI_X_VECTOR_RX].vector);
 	}

@@ -3557,10 +3557,12 @@
 	if (ret != 0 && np->msi_flags & NV_MSI_CAPABLE) {
 		if ((ret = pci_enable_msi(np->pci_dev)) == 0) {
 			np->msi_flags |= NV_MSI_ENABLED;
+			dev->irq = np->pci_dev->irq;
 			if (request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev) != 0) {
 				printk(KERN_INFO "forcedeth: request_irq failed %d\n", ret);
 				pci_disable_msi(np->pci_dev);
 				np->msi_flags &= ~NV_MSI_ENABLED;
+				dev->irq = np->pci_dev->irq;
 				goto out_err;
 			}
 

@@ -3623,7 +3625,7 @@
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			disable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
-			disable_irq_lockdep(dev->irq);
+			disable_irq_lockdep(np->pci_dev->irq);
 		mask = np->irqmask;
 	} else {
 		if (np->nic_poll_irq & NVREG_IRQ_RX_ALL) {

@@ -3641,6 +3643,8 @@
 	}
 	np->nic_poll_irq = 0;
 
+	/* disable_irq() contains synchronize_irq, thus no irq handler can run now */
+
 	if (np->recover_error) {
 		np->recover_error = 0;
 		printk(KERN_INFO "forcedeth: MAC in recoverable error state\n");

@@ -3677,7 +3681,6 @@
 		}
 	}
 
-	/* FIXME: Do we need synchronize_irq(dev->irq) here? */
 
 	writel(mask, base + NvRegIrqMask);
 	pci_push(base);

@@ -3690,7 +3693,7 @@
 		if (np->msi_flags & NV_MSI_X_ENABLED)
 			enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);
 		else
-			enable_irq_lockdep(dev->irq);
+			enable_irq_lockdep(np->pci_dev->irq);
 	} else {
 		if (np->nic_poll_irq & NVREG_IRQ_RX_ALL) {
 			nv_nic_irq_rx(0, dev);

@@ -4943,7 +4946,7 @@
 	np->in_shutdown = 1;
 	spin_unlock_irq(&np->lock);
 	netif_poll_disable(dev);
-	synchronize_irq(dev->irq);
+	synchronize_irq(np->pci_dev->irq);
 
 	del_timer_sync(&np->oom_kick);
 	del_timer_sync(&np->nic_poll);
#11 lore raw
Quoted 10 lines · Manfred Spraul
Quoted 9 lines · Yinghai Lu
Quoted 6 lines · Manfred Spraul

Someone around with a MSI capable board? The forcedeth driver does

    dev->irq = pci_dev->irq

in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Quoted 4 lines
Quoted 3 lines

the request_irq==>setup_irq will make dev->irq = pci_dev->irq.

Where is that?

in nv_request_irq

                if ((ret = pci_enable_msi(np->pci_dev)) == 0) {
                        np->msi_flags |= NV_MSI_ENABLED;
                        if (request_irq(np->pci_dev->irq, handler,

IRQF_SHARED, dev->name, dev) != 0) {

in request_irq

int request_irq(unsigned int irq, irq_handler_t handler,
unsigned long irqflags, const char *devname, void *dev_id)
...

        action->dev_id = dev_id;
...
        retval = setup_irq(irq, action);

in setup_irq
int setup_irq(unsigned int irq, struct irqaction *new)
....

        new->irq = irq;

it seems I missed that here new is irqaction instead of net_dev.

YH

#12 lore raw
In reply to Manfred Spraul
Quoted 10 lines · Manfred Spraul
Quoted 9 lines · Yinghai Lu
Quoted 6 lines · Manfred Spraul

Someone around with a MSI capable board? The forcedeth driver does

    dev->irq = pci_dev->irq

in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Quoted 6 lines
Quoted 3 lines

the request_irq==>setup_irq will make dev->irq = pci_dev->irq.

Where is that?
Otherwise I would propose the attached patch. My board is not MSI-capable, thus I can't test it myself.

Why not just copy pcidev->irq to dev->irq once ?

Ben.

#13 lore raw
Quoted 12 lines · Benjamin Herrenschmidt
Quoted 10 lines · Manfred Spraul
Quoted 9 lines · Yinghai Lu
Quoted 6 lines · Manfred Spraul

Someone around with a MSI capable board? The forcedeth driver does

    dev->irq = pci_dev->irq

in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

the request_irq==>setup_irq will make dev->irq = pci_dev->irq.

Where is that?
Otherwise I would propose the attached patch. My board is not MSI-capable, thus I can't test it myself.

Why not just copy pcidev->irq to dev->irq once ?

it seems e1000 is using np->pci_dev->irq directly too.

YH

#14 lore raw
Quoted 16 lines · Yinghai Lu
Quoted 15 lines · Benjamin Herrenschmidt
Quoted 13 lines · Manfred Spraul
Quoted 12 lines · Yinghai Lu
Quoted 6 lines · Manfred Spraul

Someone around with a MSI capable board? The forcedeth driver does

    dev->irq = pci_dev->irq

in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

the request_irq==>setup_irq will make dev->irq = pci_dev->irq.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Where is that?
Otherwise I would propose the attached patch. My board is not MSI-capable, thus I can't test it myself.

Why not just copy pcidev->irq to dev->irq once ?

it seems e1000 is using np->pci_dev->irq directly too.

Heh, allright, doesn't matter, I was just proposing to avoid one more indirection :-)

Ben.

#15 lore raw
In reply to Manfred Spraul
Quoted 12 lines · Manfred Spraul
Quoted 3 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Quoted 4 lines

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Remember, fundamentally MSI-X is a one-to-many relationship, when you consider a single PCI device might have multiple vectors.

Jeff

#16 lore raw
Quoted 18 lines · Jeff Garzik
Quoted 17 lines · Manfred Spraul
Quoted 6 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Quoted 8 lines
Quoted 5 lines

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Remember, fundamentally MSI-X is a one-to-many relationship, when you consider a single PCI device might have multiple vectors.

msi-x is using other entry

               if (np->msi_flags & NV_MSI_X_ENABLED)

enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);

YH

#17 lore raw
Quoted 23 lines · Yinghai Lu
Quoted 22 lines · Jeff Garzik
Quoted 21 lines · Manfred Spraul
Quoted 5 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Quoted 8 lines
Quoted 2 lines

Remember, fundamentally MSI-X is a one-to-many relationship, when you consider a single PCI device might have multiple vectors.

msi-x is using other entry

               if (np->msi_flags & NV_MSI_X_ENABLED)

enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);

Correct, but the overall point was that MSI-X conceptually conflicts with the existing "lockless" disable_irq() schedule, which was written when there was a one-one relationship between irq, PCI device, and work to be done.

Jeff

#18 lore raw
Quoted 29 lines · Jeff Garzik
Quoted 28 lines · Yinghai Lu
Quoted 17 lines · Jeff Garzik
Quoted 16 lines · Manfred Spraul
Quoted 5 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Quoted 7 lines
Quoted 5 lines

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Remember, fundamentally MSI-X is a one-to-many relationship, when you consider a single PCI device might have multiple vectors.

msi-x is using other entry

Quoted 8 lines
Quoted 3 lines
               if (np->msi_flags & NV_MSI_X_ENABLED)

enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);

Correct, but the overall point was that MSI-X conceptually conflicts with the existing "lockless" disable_irq() schedule, which was written when there was a one-one relationship between irq, PCI device, and work to be done.

Can I use your new driver with RHEL 5 or RHEL 5.1?

YH

#19 lore raw
Quoted 31 lines · Yinghai Lu
Quoted 30 lines · Jeff Garzik
Quoted 29 lines · Yinghai Lu
Quoted 17 lines · Jeff Garzik
Quoted 16 lines · Manfred Spraul
Quoted 5 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Quoted 7 lines
Quoted 5 lines

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Remember, fundamentally MSI-X is a one-to-many relationship, when you consider a single PCI device might have multiple vectors.

msi-x is using other entry

               if (np->msi_flags & NV_MSI_X_ENABLED)
Quoted 7 lines
Quoted 5 lines
Quoted 1 line

enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);

Correct, but the overall point was that MSI-X conceptually conflicts with the existing "lockless" disable_irq() schedule, which was written when there was a one-one relationship between irq, PCI device, and work to be done.

Can I use your new driver with RHEL 5 or RHEL 5.1?

Not without modification, since it depends on the napi_struct work currently in torvalds/linux-2.6.git.

But I am currently rewriting the fe-lock yet again, and most of those changes can be applied to pre-napi_struct forcedeth.

Jeff

#20 lore raw
In reply to Jeff Garzik
Quoted 29 lines · Jeff Garzik
Quoted 28 lines · Yinghai Lu
Quoted 17 lines · Jeff Garzik
Quoted 16 lines · Manfred Spraul
Quoted 5 lines · Jeff Garzik

I think the scenario you outline is an illustration of the approach's
fragility: disable_irq() is a heavy hammer that originated with INTx, and it relies on a chip-specific disable method (kernel/irq/manage.c) that practically guarantees behavior will vary across MSI/INTx/etc.

I checked the code: IRQ_DISABLE is implemented in software, i.e. handle_level_irq() only calls handle_IRQ_event() [and then the nic irq handler] if IRQ_DISABLE is not set.
OTHO: The last trace looks as if nv_do_nic_poll() is interrupted by an irq.

Perhaps something corrupts dev->irq? The irq is requested with
request_irq(np->pci_dev->irq, handler, IRQF_SHARED, dev->name, dev)
and disabled with
disable_irq_lockdep(dev->irq);

Quoted 7 lines
Quoted 5 lines

Someone around with a MSI capable board? The forcedeth driver does
dev->irq = pci_dev->irq
in nv_probe(), especially before pci_enable_msi().
Does pci_enable_msi() change pci_dev->irq? Then we would disable the wrong interrupt....

Remember, fundamentally MSI-X is a one-to-many relationship, when you consider a single PCI device might have multiple vectors.

msi-x is using other entry

Quoted 8 lines
Quoted 3 lines
               if (np->msi_flags & NV_MSI_X_ENABLED)

enable_irq_lockdep(np->msi_x_entry[NV_MSI_X_VECTOR_ALL].vector);

Correct, but the overall point was that MSI-X conceptually conflicts with the existing "lockless" disable_irq() schedule, which was written when there was a one-one relationship between irq, PCI device, and work to be done.

at this point, nic in mcp55 is using msi or INTx.

YH

#21 lore raw
Quoted 10 lines · Yinghai Lu
Quoted 6 lines

Correct, but the overall point was that MSI-X conceptually conflicts with the existing "lockless" disable_irq() schedule, which was written when there was a one-one relationship between irq, PCI device, and work to be done.

at this point, nic in mcp55 is using msi or INTx.

Correct.
For msi-x, the driver does three disable_irq() calls to the correct vectors. ugly, but nevertheless correct.
The bug only affected msi: The driver did disable_irq(<old INTx irq num>) instead of disable_irq(<new msi-x irq num>).
The patch that I've attached to the bugzilla report 9047 seems to fix the crash, thus I would propose to apply it to 2.6.23 and 2.6.24. I'll send a seperate mail.

All other problem [reduce code duplication, less ugly locking, ...] should be fixed with independant patches.

--
Manfred

Quoted 2 lines

YH

#22 lore raw
In reply to Ayaz Abdulla
Quoted 3 lines · Ayaz Abdulla

I am trying to track down a forcedeth driver issue described by bug 9047 in bugzilla (2.6.23-rc7-git1 forcedeth w/ MCP55 oops under heavy load). I added a patch to synchronize the timer handlers so that one handler doesn't accidently enable the IRQ while another timer handler is running (see attachment 'Add timer lock' in bug report) and for other processing protection.

However, the system still had an Oops. So I added a lock around the nv_rx_process_optimized() and the Oops has not happened (see attachment 'New patch for locking' in bug report). This would imply a synchronization issue. However, the only callers of that function are the IRQ handler and the timer handlers (in non-NAPI case). The timer handlers use disable_irq so that the IRQ handler does not contend with them. It looks as if disable_irq is not working properly.

Either disable_irq() is not working properly or interrupts are nested, i.e. the irq handler is called again while running. Which timer handler do you mean? I only see disable_irq() in the configuration paths (set mtu, change ring size, ...) and in the tx timeout case. Neither one should happen during normal operation.

--
Manfred