[PATCH] tg3 locking update (with NAPI overtones)

[PATCH] tg3 locking update (with NAPI overtones)

[Jeff Garzik]

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

The attached patch is based on tg3.c as found in 2.4.20-rc3 or the 
latest Linus 2.5.x kernel, and originates from two motivations:

* When net drivers move TX completion from interrupt to dev->poll(), 
this allows the rethinking of some traditional locking, namely 
eliminating a lock in dev->start_xmit() that most drivers implement 
these days.

* Specific to the tg3 implementation, spin-lock-irq is held during NIC 
hardware halt and re-initialization (and a few other operations).  In 
general a goal is to change this; the driver may wind up holding 
interrupts disabled for longer than prudence dictates.  The attached 
patch does not address this issue directly, but it does begin to lay the 
groundwork for my upcoming solution :)

I mention "NAPI" in the subject because other NAPI drivers may wish to 
re-examine their locking as well, and this patch could serve as a basis 
for discussion.


Overall, I believe the attached patch (after testing/debugging/review) 
will make the tg3 driver faster due to less locking in dev->start_xmit, 
and more friendly system interaction because interrupts are being 
disabled->enabled less frequently by this driver.


Here is a function-by-function description of changes, which gives one a 
better idea of the locking changes that may be a found in a driver:

all functions:  s/spin_lock_irq/spin_lock_irqsave/ to be more 
conservative and "obviously safe"

all functions:  locking ordering now reversed:  dev->xmit_lock obtained 
before tp->lock.

tg3_set_power_state: Do not call tg3_halt(), the one case where this 
code path is hit, the caller has already called tg3_halt()

tg3_poll: hold dev->xmit_lock when checking for TX work, and running TX 
completion cycle.  tp->tx_lock GC'd.

tg3_tx_timeout: net stack already holds dev->xmit_lock.  tx_lock GC'd.

tg3_{4gbug}_start_xmit: net stack already holds dev->xmit_lock, no other 
locks needed (whee!), so:  tp->lock GC'd.

tg3_change_mtu: get dev->xmit_lock instead of tp->tx_lock. wrap locking 
in netif_start_queue ... netif_wake_queue.

tg3_timer: get xmit_lock instead of tx_lock.  wrap NIC reset in 
netif_start_queue ... netif_wake_queue.

tg3_open: no need for tx locking, just move netif_start_queue down to 
the bottom of the function.

tg3_close: s/tp->tx_lock/dev->xmit_lock/

tg3_get_regs: likewise

tg3_ethtool_ioctl: likewise, plus netif_{start,wake}_queue wrap around 
NIC reset

tg3_vlan_rx_register: remove TX lock

tg3_vlan_rx_kill_vid: remove TX lock

tg3_suspend: s/tp->tx_lock/dev->xmit_lock/, netif_{start,wake}_queue wrap

tg3_resume: likewise

Th-th-th-th-that's all, folks!

Questions/comments/flames requested.

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 14617 bytes --]

diff -Nru a/drivers/net/tg3.c b/drivers/net/tg3.c
--- a/drivers/net/tg3.c	Tue Nov 26 18:05:17 2002
+++ b/drivers/net/tg3.c	Tue Nov 26 18:05:17 2002
@@ -63,7 +63,7 @@
 
 #define DRV_MODULE_NAME		"tg3"
 #define PFX DRV_MODULE_NAME	": "
-#define DRV_MODULE_VERSION	"1.2"
+#define DRV_MODULE_VERSION	"1.2txlock"
 #define DRV_MODULE_RELDATE	"Nov 14, 2002"
 
 #define TG3_DEF_MAC_MODE	0
@@ -416,7 +416,6 @@
 }
 
 static int tg3_setup_phy(struct tg3 *);
-static int tg3_halt(struct tg3 *);
 
 static int tg3_set_power_state(struct tg3 *tp, int state)
 {
@@ -487,8 +486,6 @@
 		tg3_setup_phy(tp);
 	}
 
-	tg3_halt(tp);
-
 	pci_read_config_word(tp->pdev, pm + PCI_PM_PMC, &power_caps);
 
 	if (tp->tg3_flags & TG3_FLAG_WOL_ENABLE) {
@@ -2073,8 +2070,14 @@
 	struct tg3 *tp = netdev->priv;
 	struct tg3_hw_status *sblk = tp->hw_status;
 	int done;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
+	spin_lock(&tp->dev->xmit_lock);
+	if (sblk->idx[0].tx_consumer != tp->tx_cons)
+		tg3_tx(tp);
+	spin_unlock(&tp->dev->xmit_lock);
+
+	spin_lock_irqsave(&tp->lock, flags);
 
 	if (!(tp->tg3_flags &
 	      (TG3_FLAG_USE_LINKCHG_REG |
@@ -2086,12 +2089,6 @@
 		}
 	}
 
-	if (sblk->idx[0].tx_consumer != tp->tx_cons) {
-		spin_lock(&tp->tx_lock);
-		tg3_tx(tp);
-		spin_unlock(&tp->tx_lock);
-	}
-
 	done = 1;
 	if (sblk->idx[0].rx_producer != tp->rx_rcb_ptr) {
 		int orig_budget = *budget;
@@ -2114,7 +2111,7 @@
 		tg3_unmask_ints(tp);
 	}
 
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	return (done ? 0 : 1);
 }
@@ -2175,23 +2172,23 @@
 
 static void tg3_init_rings(struct tg3 *);
 static int tg3_init_hw(struct tg3 *);
+static int tg3_halt(struct tg3 *tp);
 
 static void tg3_tx_timeout(struct net_device *dev)
 {
 	struct tg3 *tp = dev->priv;
+	unsigned long flags;
 
 	printk(KERN_ERR PFX "%s: transmit timed out, resetting\n",
 	       dev->name);
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tg3_halt(tp);
 	tg3_init_rings(tp);
 	tg3_init_hw(tp);
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	netif_wake_queue(dev);
 }
@@ -2371,35 +2368,12 @@
 	unsigned int i;
 	u32 len, entry, base_flags, mss;
 	int would_hit_hwbug;
-	unsigned long flags;
 
 	len = (skb->len - skb->data_len);
 
-	/* No BH disabling for tx_lock here.  We are running in BH disabled
-	 * context and TX reclaim runs via tp->poll inside of a software
-	 * interrupt.  Rejoice!
-	 *
-	 * Actually, things are not so simple.  If we are to take a hw
-	 * IRQ here, we can deadlock, consider:
-	 *
-	 *       CPU1		CPU2
-	 *   tg3_start_xmit
-	 *   take tp->tx_lock
-	 *			tg3_timer
-	 *			take tp->lock
-	 *   tg3_interrupt
-	 *   spin on tp->lock
-	 *			spin on tp->tx_lock
-	 *
-	 * So we really do need to disable interrupts when taking
-	 * tx_lock here.
-	 */
-	spin_lock_irqsave(&tp->tx_lock, flags);
-
 	/* This is a hard error, log it. */
 	if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
 		netif_stop_queue(dev);
-		spin_unlock_irqrestore(&tp->tx_lock, flags);
 		printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
 		       dev->name);
 		return 1;
@@ -2551,8 +2525,6 @@
 		netif_stop_queue(dev);
 
 out_unlock:
-	spin_unlock_irqrestore(&tp->tx_lock, flags);
-
 	dev->trans_start = jiffies;
 
 	return 0;
@@ -2563,35 +2535,12 @@
 	struct tg3 *tp = dev->priv;
 	dma_addr_t mapping;
 	u32 len, entry, base_flags, mss;
-	unsigned long flags;
 
 	len = (skb->len - skb->data_len);
 
-	/* No BH disabling for tx_lock here.  We are running in BH disabled
-	 * context and TX reclaim runs via tp->poll inside of a software
-	 * interrupt.  Rejoice!
-	 *
-	 * Actually, things are not so simple.  If we are to take a hw
-	 * IRQ here, we can deadlock, consider:
-	 *
-	 *       CPU1		CPU2
-	 *   tg3_start_xmit
-	 *   take tp->tx_lock
-	 *			tg3_timer
-	 *			take tp->lock
-	 *   tg3_interrupt
-	 *   spin on tp->lock
-	 *			spin on tp->tx_lock
-	 *
-	 * So we really do need to disable interrupts when taking
-	 * tx_lock here.
-	 */
-	spin_lock_irqsave(&tp->tx_lock, flags);
-
 	/* This is a hard error, log it. */
 	if (unlikely(TX_BUFFS_AVAIL(tp) <= (skb_shinfo(skb)->nr_frags + 1))) {
 		netif_stop_queue(dev);
-		spin_unlock_irqrestore(&tp->tx_lock, flags);
 		printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
 		       dev->name);
 		return 1;
@@ -2697,8 +2646,6 @@
 	if (TX_BUFFS_AVAIL(tp) <= (MAX_SKB_FRAGS + 1))
 		netif_stop_queue(dev);
 
-	spin_unlock_irqrestore(&tp->tx_lock, flags);
-
 	dev->trans_start = jiffies;
 
 	return 0;
@@ -2707,6 +2654,7 @@
 static int tg3_change_mtu(struct net_device *dev, int new_mtu)
 {
 	struct tg3 *tp = dev->priv;
+	unsigned long flags;
 
 	if (new_mtu < TG3_MIN_MTU || new_mtu > TG3_MAX_MTU)
 		return -EINVAL;
@@ -2719,8 +2667,9 @@
 		return 0;
 	}
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	netif_stop_queue(tp->dev);
+	spin_lock_bh(&tp->dev->xmit_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tg3_halt(tp);
 
@@ -2734,8 +2683,10 @@
 	tg3_init_rings(tp);
 	tg3_init_hw(tp);
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_bh(&tp->dev->xmit_lock);
+
+	netif_wake_queue(tp->dev);
 
 	return 0;
 }
@@ -4500,9 +4451,10 @@
 static void tg3_timer(unsigned long __opaque)
 {
 	struct tg3 *tp = (struct tg3 *) __opaque;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock(&tp->dev->xmit_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	/* All of this garbage is because when using non-tagged
 	 * IRQ status the mailbox/status_block protocol the chip
@@ -4517,9 +4469,11 @@
 	}
 
 	if (!(tr32(WDMAC_MODE) & WDMAC_MODE_ENABLE)) {
+		netif_stop_queue(tp->dev);
 		tg3_halt(tp);
 		tg3_init_rings(tp);
 		tg3_init_hw(tp);
+		netif_wake_queue(tp->dev);
 	}
 
 	/* This part only runs once per second. */
@@ -4582,8 +4536,8 @@
 		tp->asf_counter = tp->asf_multiplier;
 	}
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock(&tp->dev->xmit_lock);
 
 	tp->timer.expires = jiffies + tp->timer_offset;
 	add_timer(&tp->timer);
@@ -4593,15 +4547,14 @@
 {
 	struct tg3 *tp = dev->priv;
 	int err;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tg3_disable_ints(tp);
 	tp->tg3_flags &= ~TG3_FLAG_INIT_COMPLETE;
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	/* If you move this call, make sure TG3_FLAG_HOST_TXDS in
 	 * tp->tg3_flags is accurate at that new place.
@@ -4618,8 +4571,7 @@
 		return err;
 	}
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tg3_init_rings(tp);
 
@@ -4641,8 +4593,7 @@
 		tp->tg3_flags |= TG3_FLAG_INIT_COMPLETE;
 	}
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	if (err) {
 		free_irq(dev->irq, dev);
@@ -4650,15 +4601,13 @@
 		return err;
 	}
 
-	netif_start_queue(dev);
-
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tg3_enable_ints(tp);
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+
+	netif_start_queue(dev);
 
 	return 0;
 }
@@ -4913,13 +4862,14 @@
 static int tg3_close(struct net_device *dev)
 {
 	struct tg3 *tp = dev->priv;
+	unsigned long flags;
 
 	netif_stop_queue(dev);
 
 	del_timer_sync(&tp->timer);
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_bh(&tp->dev->xmit_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 #if 0
 	tg3_dump_state(tp);
 #endif
@@ -4933,8 +4883,8 @@
 		  TG3_FLAG_GOT_SERDES_FLOWCTL);
 	netif_carrier_off(tp->dev);
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_bh(&tp->dev->xmit_lock);
 
 	free_irq(dev->irq, dev);
 
@@ -5143,6 +5093,7 @@
 
 static u8 *tg3_get_regs(struct tg3 *tp)
 {
+	unsigned long flags;
 	u8 *orig_p = kmalloc(TG3_REGDUMP_LEN, GFP_KERNEL);
 	u8 *p;
 	int i;
@@ -5152,8 +5103,8 @@
 
 	memset(orig_p, 0, TG3_REGDUMP_LEN);
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_bh(&tp->dev->xmit_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 #define __GET_REG32(reg)	(*((u32 *)(p))++ = tr32(reg))
 #define GET_REG32_LOOP(base,len)		\
@@ -5202,8 +5153,8 @@
 #undef GET_REG32_LOOP
 #undef GET_REG32_1
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_bh(&tp->dev->xmit_lock);
 
 	return orig_p;
 }
@@ -5213,6 +5164,7 @@
 	struct tg3 *tp = dev->priv;
 	struct pci_dev *pci_dev = tp->pdev;
 	u32 ethcmd;
+	unsigned long flags;
 
 	if (copy_from_user (&ethcmd, useraddr, sizeof (ethcmd)))
 		return -EFAULT;
@@ -5300,8 +5252,7 @@
 				return -EINVAL;
 		}
 
-		spin_lock_irq(&tp->lock);
-		spin_lock(&tp->tx_lock);
+		spin_lock_irqsave(&tp->lock, flags);
 
 		tp->link_config.autoneg = cmd.autoneg;
 		if (cmd.autoneg == AUTONEG_ENABLE) {
@@ -5314,8 +5265,7 @@
 		}
 
 		tg3_setup_phy(tp);
-		spin_unlock(&tp->tx_lock);
-		spin_unlock_irq(&tp->lock);
+		spin_unlock_irqrestore(&tp->lock, flags);
 
 		return 0;
 	}
@@ -5453,8 +5403,9 @@
 		    (ering.tx_pending > TG3_TX_RING_SIZE - 1))
 			return -EINVAL;
 
-		spin_lock_irq(&tp->lock);
-		spin_lock(&tp->tx_lock);
+		netif_stop_queue(tp->dev);
+		spin_lock_bh(&tp->dev->xmit_lock);
+		spin_lock_irqsave(&tp->lock, flags);
 
 		tp->rx_pending = ering.rx_pending;
 #if TG3_MINI_RING_WORKS
@@ -5466,9 +5417,9 @@
 		tg3_halt(tp);
 		tg3_init_rings(tp);
 		tg3_init_hw(tp);
+		spin_unlock_irqrestore(&tp->lock, flags);
+		spin_unlock_bh(&tp->dev->xmit_lock);
 		netif_wake_queue(tp->dev);
-		spin_unlock(&tp->tx_lock);
-		spin_unlock_irq(&tp->lock);
 
 		return 0;
 	}
@@ -5491,8 +5442,9 @@
 		if (copy_from_user(&epause, useraddr, sizeof(epause)))
 			return -EFAULT;
 
-		spin_lock_irq(&tp->lock);
-		spin_lock(&tp->tx_lock);
+		netif_stop_queue(tp->dev);
+		spin_lock_bh(&tp->dev->xmit_lock);
+		spin_lock_irqsave(&tp->lock, flags);
 		if (epause.autoneg)
 			tp->tg3_flags |= TG3_FLAG_PAUSE_AUTONEG;
 		else
@@ -5508,8 +5460,9 @@
 		tg3_halt(tp);
 		tg3_init_rings(tp);
 		tg3_init_hw(tp);
-		spin_unlock(&tp->tx_lock);
-		spin_unlock_irq(&tp->lock);
+		spin_unlock_irqrestore(&tp->lock, flags);
+		spin_unlock_bh(&tp->dev->xmit_lock);
+		netif_wake_queue(tp->dev);
 
 		return 0;
 	}
@@ -5644,29 +5597,27 @@
 static void tg3_vlan_rx_register(struct net_device *dev, struct vlan_group *grp)
 {
 	struct tg3 *tp = dev->priv;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tp->vlgrp = grp;
 
 	/* Update RX_MODE_KEEP_VLAN_TAG bit in RX_MODE register. */
 	__tg3_set_rx_mode(dev);
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 }
 
 static void tg3_vlan_rx_kill_vid(struct net_device *dev, unsigned short vid)
 {
 	struct tg3 *tp = dev->priv;
+	unsigned long flags;
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 	if (tp->vlgrp)
 		tp->vlgrp->vlan_devices[vid] = NULL;
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 }
 #endif
 
@@ -6852,7 +6803,6 @@
 	tp->grc_mode |= GRC_MODE_BSWAP_NONFRM_DATA;
 #endif
 	spin_lock_init(&tp->lock);
-	spin_lock_init(&tp->tx_lock);
 	spin_lock_init(&tp->indirect_lock);
 
 	tp->regs = (unsigned long) ioremap(tg3reg_base, tg3reg_len);
@@ -6994,36 +6944,37 @@
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct tg3 *tp = dev->priv;
 	int err;
+	unsigned long flags;
 
 	if (!netif_running(dev))
 		return 0;
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 	tg3_disable_ints(tp);
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
 
 	netif_device_detach(dev);
 
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_bh(&dev->xmit_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 	tg3_halt(tp);
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_bh(&dev->xmit_lock);
 
 	err = tg3_set_power_state(tp, state);
 	if (err) {
-		spin_lock_irq(&tp->lock);
-		spin_lock(&tp->tx_lock);
+		spin_lock_bh(&dev->xmit_lock);
+		spin_lock_irqsave(&tp->lock, flags);
 
 		tg3_init_rings(tp);
 		tg3_init_hw(tp);
+		tg3_enable_ints(tp);
 
-		spin_unlock(&tp->tx_lock);
-		spin_unlock_irq(&tp->lock);
+		spin_unlock_irqrestore(&tp->lock, flags);
+		spin_unlock_bh(&dev->xmit_lock);
 
 		netif_device_attach(dev);
+		netif_wake_queue(dev);
 	}
 
 	return err;
@@ -7034,6 +6985,7 @@
 	struct net_device *dev = pci_get_drvdata(pdev);
 	struct tg3 *tp = dev->priv;
 	int err;
+	unsigned long flags;
 
 	if (!netif_running(dev))
 		return 0;
@@ -7042,17 +6994,18 @@
 	if (err)
 		return err;
 
-	netif_device_attach(dev);
-
-	spin_lock_irq(&tp->lock);
-	spin_lock(&tp->tx_lock);
+	spin_lock_bh(&tp->dev->xmit_lock);
+	spin_lock_irqsave(&tp->lock, flags);
 
 	tg3_init_rings(tp);
 	tg3_init_hw(tp);
 	tg3_enable_ints(tp);
 
-	spin_unlock(&tp->tx_lock);
-	spin_unlock_irq(&tp->lock);
+	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_bh(&tp->dev->xmit_lock);
+
+	netif_device_attach(dev);
+	netif_wake_queue(dev);
 
 	return 0;
 }
diff -Nru a/drivers/net/tg3.h b/drivers/net/tg3.h
--- a/drivers/net/tg3.h	Tue Nov 26 18:05:17 2002
+++ b/drivers/net/tg3.h	Tue Nov 26 18:05:17 2002
@@ -1733,15 +1733,17 @@
 	 * lock: Held during all operations except TX packet
 	 *       processing.
 	 *
-	 * tx_lock: Held during tg3_start_xmit{,_4gbug} and tg3_tx
+	 * dev->xmit_lock: Held in tg3_tx, and by upper layer during
+	 * tg3_start_xmit{,_4gbug}, plus assorted driver areas
+	 * that happen to relate to the TX engine.
 	 *
 	 * If you want to shut up all asynchronous processing you must
-	 * acquire both locks, 'lock' taken before 'tx_lock'.  IRQs must
-	 * be disabled to take 'lock' but only softirq disabling is
-	 * necessary for acquisition of 'tx_lock'.
+	 * acquire both locks, 'xmit_lock' taken before 'lock'.  xmit_lock
+	 * is often acquired by the upper layers, so pay attention.  IRQs must
+	 * be disabled to take 'lock' but only softirq/BH disabling is
+	 * necessary for acquisition of 'xmit_lock'.
 	 */
 	spinlock_t			lock;
-	spinlock_t			tx_lock;
 
 	u32				tx_prod;
 	u32				tx_cons;

Re: [PATCH] tg3 locking update (with NAPI overtones)

[David S. Miller]

Looks good to me on first glance, testing will confirm
further :-)

Probably, to kill the long delays with locks held, we just
need to add a PHY config semaphore.  Interrupts that want to
try and program the PHY just do a down_trylock() on that semaphore
and defer their work if it cannot be acquired.

Re: [PATCH] tg3 locking update (with NAPI overtones)

[Jeff Garzik]

David S. Miller wrote:

> Looks good to me on first glance, testing will confirm
> further :-)

yeppers :)

> Probably, to kill the long delays with locks held, we just
> need to add a PHY config semaphore.  Interrupts that want to
> try and program the PHY just do a down_trylock() on that semaphore
> and defer their work if it cannot be acquired.


I was thinking along similar lines, and you just gave me an idea as well 
:)  Hopefully I can present a patch later on tonight showing these ideas.

For now, just responding to the above, I think there are further needs: 
  even if you acquire that semaphore, you can still wind up a full 
tg3_halt + tg3_init_hw in interrupt context, which means you could be 
there a while.  MAC/firmware init can be expensive, even ignoring the 
phy init.  Also consider the general argument that once you are 
resetting the MAC or phy, you aren't really doing useful RX/TX work; you 
can concentrate on making the slow path simple and safe while knowing 
that the fast path is idle.

So...   I prefer to simply always defer MAC/phy reset if in interrupt 
context.  This allows us to sleep as long as we want during phy and MAC 
init.  If the manual says "poll each block max of 2ms", we can do it 
easily with yield() in a loop or schedule_timeout(), the way God 
intended drivers to sleep :)

	Jeff

Re: [PATCH] tg3 locking update (with NAPI overtones)

[Ben Greear]

Just out of curiosity, have any of you been able to reproduce
any IRQ warnings under heavy loads with the tg3?  If I start
up all ports of a 4-port tulip nic and also start the tg3
(all running high speed), then I see a steady stream of
complaints from the tg3....

At least one other person has seen the same and has emailed
me about it...

-- 
Ben Greear <greearb@candelatech.com>       <Ben_Greear AT excite.com>
President of Candela Technologies Inc      http://www.candelatech.com
ScryMUD:  http://scry.wanfear.com     http://scry.wanfear.com/~greear

Re: [PATCH] tg3 locking update (with NAPI overtones)

[Jeff Garzik]

Ben Greear wrote:

> Just out of curiosity, have any of you been able to reproduce
> any IRQ warnings under heavy loads with the tg3?  If I start
> up all ports of a 4-port tulip nic and also start the tg3
> (all running high speed), then I see a steady stream of
> complaints from the tg3....



Can you be more specific?  :)  Is it "poll already scheduled"?

Also:  is the tg3 sharing interrupts with any other device?

[PATCH] tg3 locking update (with NAPI overtones)

[Robert Olsson]

Jeff Garzik writes:

 > * When net drivers move TX completion from interrupt to dev->poll(), 
 > this allows the rethinking of some traditional locking, namely 
 > eliminating a lock in dev->start_xmit() that most drivers implement 
 > these days.

 Yes. 

 > 
 > tg3_tx_timeout: net stack already holds dev->xmit_lock.  tx_lock GC'd.
 
 For tx_timout and other timer/async work there may exist a scheduled poll 
 to which we have to sync. Something like this could be done. Ideas comes
 from dev->close.

 /*
  *  Synchronize and disable poll
  */

  while (test_and_set_bit(__LINK_STATE_RX_SCHED, &dev->state)) {

      	  current->state = TASK_INTERRUPTIBLE;
	  schedule_timeout(1);
  }

  .
  .

 /* Enable */
  clear_bit(__LINK_STATE_RX_SCHED, &dev->state);


 Cheers.
						--ro