[PATCH] igb: completely disable interrupts during netpoll

[PATCH] igb: completely disable interrupts during netpoll

[Andy Gospodarek]

Globally disable the interrupt during netpoll rather than simply
disabling it on the hardware.  This will ensure all pending operations
are complete and no more will execute.  I've seen reports of deadlocks
with the current driver and this patch resolves them.
---
 drivers/net/igb/igb_main.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index ae398f0..5ddd1b4 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -4024,7 +4024,7 @@ static void igb_netpoll(struct net_device *netdev)
 	int i;
 	int work_done = 0;
 
-	igb_irq_disable(adapter);
+	disable_irq(adapter->pdev->irq);
 	for (i = 0; i < adapter->num_tx_queues; i++)
 		igb_clean_tx_irq(adapter, &adapter->tx_ring[i]);
 
@@ -4033,7 +4033,7 @@ static void igb_netpoll(struct net_device *netdev)
 				     &work_done,
 				     adapter->rx_ring[i].napi.weight);
 
-	igb_irq_enable(adapter);
+	enable_irq(adapter->pdev->irq);
 }
 #endif /* CONFIG_NET_POLL_CONTROLLER */
 
-- 
1.5.2.1

RE: [PATCH] igb: completely disable interrupts during netpoll

[Brandeburg, Jesse]

Andy Gospodarek wrote:
> Globally disable the interrupt during netpoll rather than simply
> disabling it on the hardware.  This will ensure all pending operations
> are complete and no more will execute.  I've seen reports of deadlocks
> with the current driver and this patch resolves them.

I agree this patch is probably okay, and if it fixes a specific problem
should be accepted however I have a question that didn't apply when we
originally created code like this.

> @@ -4024,7 +4024,7 @@ static void igb_netpoll(struct net_device
>  	*netdev) int i;
>  	int work_done = 0;
> 
> -	igb_irq_disable(adapter);
> +	disable_irq(adapter->pdev->irq);

how does changing to disable_irq/enable_irq work with MSI and MSI-X
interrupts?

Re: [PATCH] igb: completely disable interrupts during netpoll

[Matthew Wilcox]

On Wed, Jun 25, 2008 at 01:29:19PM -0700, Brandeburg, Jesse wrote:
> > -	igb_irq_disable(adapter);
> > +	disable_irq(adapter->pdev->irq);
> 
> how does changing to disable_irq/enable_irq work with MSI and MSI-X
> interrupts?

Works just fine.  There's a disable_irq method in the irq_chip that
knows how to disable a particular type of interrupt.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] igb: completely disable interrupts during netpoll

[Andy Gospodarek]

On Wed, Jun 25, 2008 at 02:35:19PM -0600, Matthew Wilcox wrote:
> On Wed, Jun 25, 2008 at 01:29:19PM -0700, Brandeburg, Jesse wrote:
> > > -	igb_irq_disable(adapter);
> > > +	disable_irq(adapter->pdev->irq);
> > 
> > how does changing to disable_irq/enable_irq work with MSI and MSI-X
> > interrupts?
> 
> Works just fine.  There's a disable_irq method in the irq_chip that
> knows how to disable a particular type of interrupt.
> 

Sure does.  Just for the record, most of the other net-drivers do it
this way too.

Re: [PATCH] igb: completely disable interrupts during netpoll

[David Miller]

From: Andy Gospodarek <andy@greyhouse.net>
Date: Wed, 25 Jun 2008 16:39:31 -0400

> Just for the record, most of the other net-drivers do it
> this way too.

Ummm, really?

No NAPI based driver I ever wrote does this.

Re: [PATCH] igb: completely disable interrupts during netpoll

[Andy Gospodarek]

On Wed, Jun 25, 2008 at 05:29:11PM -0700, David Miller wrote:
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Wed, 25 Jun 2008 16:39:31 -0400
> 
> > Just for the record, most of the other net-drivers do it
> > this way too.
> 
> Ummm, really?
> 
> No NAPI based driver I ever wrote does this.

For the poll_controller routines (which is what this patch is about),
quite a few do (bnx2, bnx2x, b44, e1000, and e1000e to name a few).

Much like the other drivers for Intel hardware, the function with
'netpoll' in the name is used for the poll_controller routine whereas
the napi poll routines all have 'clean' in the name.

Sorry if 'most' was misleading, but there are quite a few drivers that
do.

Re: [PATCH] igb: completely disable interrupts during netpoll

[David Miller]

From: Andy Gospodarek <andy@greyhouse.net>
Date: Wed, 25 Jun 2008 22:40:40 -0400

> For the poll_controller routines (which is what this patch is about),
> quite a few do (bnx2, bnx2x, b44, e1000, and e1000e to name a few).
> 
> Much like the other drivers for Intel hardware, the function with
> 'netpoll' in the name is used for the poll_controller routine whereas
> the napi poll routines all have 'clean' in the name.

Thanks for the clarification.

I'm still curious what the deadlock case is.  It might be something
we should fix elsewhere too.

Re: [PATCH] igb: completely disable interrupts during netpoll

[Jesse Barnes]

On Wednesday, June 25, 2008 7:43 pm David Miller wrote:
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Wed, 25 Jun 2008 22:40:40 -0400
>
> > For the poll_controller routines (which is what this patch is about),
> > quite a few do (bnx2, bnx2x, b44, e1000, and e1000e to name a few).
> >
> > Much like the other drivers for Intel hardware, the function with
> > 'netpoll' in the name is used for the poll_controller routine whereas
> > the napi poll routines all have 'clean' in the name.
>
> Thanks for the clarification.
>
> I'm still curious what the deadlock case is.  It might be something
> we should fix elsewhere too.

I'm curious too... Also, won't using disable_irq() rather than a device 
specific method break the shared irq case?  It looks like free_irq() handles 
that, but not disable_irq().

Jesse

Re: [PATCH] igb: completely disable interrupts during netpoll

[David Miller]

From: Andy Gospodarek <andy@greyhouse.net>
Date: Wed, 25 Jun 2008 10:15:11 -0400

> 
> Globally disable the interrupt during netpoll rather than simply
> disabling it on the hardware.  This will ensure all pending operations
> are complete and no more will execute.  I've seen reports of deadlocks
> with the current driver and this patch resolves them.

I don't want to see network drivers start getting into the habit
of doing this.  This operation could be extremely expensive with
certain IRQ controllers.

It's better to have a good understanding of why the deadlocks
happen, and this was decidedly missing from your commit message.

NAPI is designed in such a way that if pending interrupts arrive
after NAPI disables chip interrupts it should be handled just
fine and in fact I see this happen all the time on some of my
test systems.

Re: [PATCH] igb: completely disable interrupts during netpoll

[Andy Gospodarek]

On Wed, Jun 25, 2008 at 03:27:51PM -0700, David Miller wrote:
> From: Andy Gospodarek <andy@greyhouse.net>
> Date: Wed, 25 Jun 2008 10:15:11 -0400
> 
> > 
> > Globally disable the interrupt during netpoll rather than simply
> > disabling it on the hardware.  This will ensure all pending operations
> > are complete and no more will execute.  I've seen reports of deadlocks
> > with the current driver and this patch resolves them.
> 
> I don't want to see network drivers start getting into the habit
> of doing this.  This operation could be extremely expensive with
> certain IRQ controllers.
> 
> It's better to have a good understanding of why the deadlocks
> happen, and this was decidedly missing from your commit message.
> 

I thought this was a pretty common problem that cannot simply be solved
with a call to synchronize_irq since you might get one that pops on
another CPU and doesn't set the in-progress bit before your
synchronize_irq call completes?

RE: [PATCH] igb: completely disable interrupts during netpoll

[Brandeburg, Jesse]

David Miller wrote:
> It's better to have a good understanding of why the deadlocks
> happen, and this was decidedly missing from your commit message.
> 
> NAPI is designed in such a way that if pending interrupts arrive
> after NAPI disables chip interrupts it should be handled just
> fine and in fact I see this happen all the time on some of my
> test systems.

I think the deadlocks occur (only when using netpoll) because the tx
cleanup (igb_clean_tx_irq) is being called simultaneously from two
routines, which generally is guaranteed not to happen in regular
everyday napi mode because of the bh_disable before calling dev->poll.

Re: [PATCH] igb: completely disable interrupts during netpoll

[David Miller]

From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
Date: Thu, 26 Jun 2008 00:23:59 -0700

> I think the deadlocks occur (only when using netpoll) because the tx
> cleanup (igb_clean_tx_irq) is being called simultaneously from two
> routines, which generally is guaranteed not to happen in regular
> everyday napi mode because of the bh_disable before calling dev->poll.

Thanks for the info, it's clear what the IGB driver bug is.

It's that the TX reclaim is not happening in the ->poll() handler.  If
you did that, all of this would work out correctly and you wouldn't
need to disable the IRQ with disable_irq() to prevent deadlocks.

Instead IGB what does is do TX reclaim inside of it's hw interrupt
handler.

And that is actually making much more work than if you did it properly
from a NAPI ->poll() context.  When igb_unmap_and_free_tx_resource()
runs from the HW interrupt it is going to queue these packet frees up
into the mid-layer's per-cpu TX free queue, and that will schedule yet
another software interrupt to actually run the kfree_skb()'s on all of
these liberated TX frames.

I moved the napi context out of the generic net device, use it! :-)
And then you can call dev_kfree_skb() directly from
igb_unmap_and_free_tx_resource() instead of dev_kfree_skb_any().

Re: [PATCH] igb: completely disable interrupts during netpoll

[Andy Gospodarek]

On Thu, Jun 26, 2008 at 02:16:58AM -0700, David Miller wrote:
> From: "Brandeburg, Jesse" <jesse.brandeburg@intel.com>
> Date: Thu, 26 Jun 2008 00:23:59 -0700
> 
> > I think the deadlocks occur (only when using netpoll) because the tx
> > cleanup (igb_clean_tx_irq) is being called simultaneously from two
> > routines, which generally is guaranteed not to happen in regular
> > everyday napi mode because of the bh_disable before calling dev->poll.
> 
> Thanks for the info, it's clear what the IGB driver bug is.
> 
> It's that the TX reclaim is not happening in the ->poll() handler.  If
> you did that, all of this would work out correctly and you wouldn't
> need to disable the IRQ with disable_irq() to prevent deadlocks.
> 
> Instead IGB what does is do TX reclaim inside of it's hw interrupt
> handler.
> 
> And that is actually making much more work than if you did it properly
> from a NAPI ->poll() context.  When igb_unmap_and_free_tx_resource()
> runs from the HW interrupt it is going to queue these packet frees up
> into the mid-layer's per-cpu TX free queue, and that will schedule yet
> another software interrupt to actually run the kfree_skb()'s on all of
> these liberated TX frames.
> 
> I moved the napi context out of the generic net device, use it! :-)
> And then you can call dev_kfree_skb() directly from
> igb_unmap_and_free_tx_resource() instead of dev_kfree_skb_any().

This is basically the same problem that I fixed with e1000 and posted
last week.  I remember that igb and ixgbe had the same possible, bug but
hadn't gotten around to posting it yet.