Matt Mackall wrote:
On Wed, Jun 07, 2006 at 11:05:22AM -0400, Neil Horman wrote:
Matt, any ideas on this?
Not at the moment.
how about this for a solution? It doesn't make netpoll any more robust, but I
think in the interests of efficiency it would be fair to require that, when
netpolled, a driver must receive frames on the same net device for which it was
polled. With this patch we detect that condition and handle it accordingly in
e1000_intr. This eliminates the need for us to call the clean_rx method from
the poll_controller when napi is configured, instead allowing the poll method to
be called from napi_poll, as the netpoll model currently does. This fixes the
netdump regression, and eliminates the layering violation and the potential race
that we've been discussing. I've just tested it with netdump here and it works
quite well.
Thoughts appreciated.
This looks pretty reasonable, mostly from the perspective that it
doesn't put any further ugliness in netpoll. We might want to add a
comment somewhere in netpoll of the new rule we're now observing.
I'll let the e1000 guys comment on the particulars of the driver change.
Hi,
we're not too happy with this as it puts a branch right in the regular receive
path. We haven't ran the numbers on it yet but it is likely that this will
lower performance significantly during normal receives for something that is
not common use.
Attached below a (revised) patch that adds proper locking around the rx_clean
to prevent the race.
Cheers,
Auke
---
e1000: fix netpoll with NAPI
This fixes netpoll when using NAPI, and protects against a rare race condition
in the netpoll routine, while staying out of our ways from the normal hotpath.
Signed-off-by: Mitch Williams <[EMAIL PROTECTED]>
Signed-off-by: Auke Kok <[EMAIL PROTECTED]>
---
e1000_main.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index bd709e5..876251c 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -4584,10 +4584,25 @@ static void
e1000_netpoll(struct net_device *netdev)
{
struct e1000_adapter *adapter = netdev_priv(netdev);
+#ifdef CONFIG_E1000_NAPI
+ int budget = 0;
+
+ disable_irq(adapter->pdev->irq);
+ if (likely(netif_rx_schedule_prep(&adapter->polling_netdev[0]))) {
+ if (spin_trylock(&adapter->tx_queue_lock)) {
+ e1000_clean_tx_irq(adapter, &adapter->tx_ring[0]);
+ spin_unlock(&adapter->tx_queue_lock);
+ }
+ adapter->clean_rx(adapter, adapter->rx_ring,
+ &budget, netdev->weight);
+ clear_bit(__LINK_STATE_RX_SCHED,
+ &adapter->polling_netdev[0].state);
+ }
+#else
+
disable_irq(adapter->pdev->irq);
e1000_intr(adapter->pdev->irq, netdev, NULL);
e1000_clean_tx_irq(adapter, adapter->tx_ring);
-#ifndef CONFIG_E1000_NAPI
adapter->clean_rx(adapter, adapter->rx_ring);
#endif
enable_irq(adapter->pdev->irq);
