Re: LLTX and netif_stop_queue

[Eric Lemoine <eric.lemoine@gmail.com>]

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

On 28 Dec 2004 08:31:57 -0500, jamal <hadi@cyberus.ca> wrote:
> On Fri, 2004-12-24 at 11:10, Eric Lemoine wrote:
> 
> > Yes but requiring drivers to release a lock that they should not even
> > be aware of doesn't sound good. Another way would be to keep
> > dev->queue_lock grabbed when entering start_xmit() and let the driver
> > drop it (and re-acquire it before it returns) only if it wishes so.
> > Although I don't like this too much either, that's the best way I can
> > think of up to now...
> 
> I am not a big fan of that patch either, but i cant think of a cleaner
> way to do it.
> The violation already happens with the LLTX flag. So maybe a big warning
> that says "Do this only if you driver is LLTX enabled". The other way to
> do it is put a check to see if LLTX is enabled before releasing that
> lock - but why the extra cycles? Driver writer should know.

Two (untested) patches implementing what I described above.

The first pach (sch_generic.patch) keeps queue_lock held in
qdisc_restart() when calling hard_start_xmit() of a LLTX driver. The
second (sungem.patch) makes sungem release queue_lock after grabbing
its private tx lock.

Note that the modifications made to qdisc_restart() are not compatible
with the current LLTX drivers. All LLTX drivers must be modified along
sungem.patch's lines. Take sungem.patch as an example of how things
must be done.

Patches apply to current davem bk snapshot.

-- 
Eric

[-- Attachment #2: sch_generic.patch --]
[-- Type: application/octet-stream, Size: 1965 bytes --]

===== net/sched/sch_generic.c 1.32 vs edited =====
--- 1.32/net/sched/sch_generic.c	2004-12-28 05:51:20 +01:00
+++ edited/net/sched/sch_generic.c	2005-01-02 23:55:18 +01:00
@@ -104,9 +104,17 @@
 		 * locking again. These checks are worth it because
 		 * even uncongested locks can be quite expensive.
 		 * The driver can do trylock like here too, in case
-		 * of lock congestion it should return -1 and the packet
-		 * will be requeued.
+		 * of lock congestion it should return NETDEV_TX_LOCKED
+		 * and the packet will be requeued.
+		 * 
+		 * Note: when the driver has LLTX set queue_lock cannot
+		 * be dropped in here if we want to avoid having packets
+		 * passing eachover during transmit. queue_lock can only
+		 * be dropped in hard_start_xmit() after private tx lock 
+		 * has been grabbed. If hard_start_xmit() does release 
+		 * queue_lock it must grab it again before it returns.
 		 */
+
 		if (!nolock) {
 			if (!spin_trylock(&dev->xmit_lock)) {
 			collision:
@@ -128,12 +136,12 @@
 			}
 			/* Remember that the driver is grabbed by us. */
 			dev->xmit_lock_owner = smp_processor_id();
+
+			/* And release queue */
+			spin_unlock(&dev->queue_lock);
 		}
 		
 		{
-			/* And release queue */
-			spin_unlock(&dev->queue_lock);
-
 			if (!netif_queue_stopped(dev)) {
 				int ret;
 				if (netdev_nit)
@@ -144,14 +152,12 @@
 					if (!nolock) {
 						dev->xmit_lock_owner = -1;
 						spin_unlock(&dev->xmit_lock);
+						spin_lock(&dev->queue_lock);
 					}
-					spin_lock(&dev->queue_lock);
 					return -1;
 				}
-				if (ret == NETDEV_TX_LOCKED && nolock) {
-					spin_lock(&dev->queue_lock);
+				if (ret == NETDEV_TX_LOCKED && nolock)
 					goto collision; 
-				}
 			}
 
 			/* NETDEV_TX_BUSY - we need to requeue */
@@ -159,8 +165,8 @@
 			if (!nolock) { 
 				dev->xmit_lock_owner = -1;
 				spin_unlock(&dev->xmit_lock);
+				spin_lock(&dev->queue_lock);
 			} 
-			spin_lock(&dev->queue_lock);
 			q = dev->qdisc;
 		}
 

[-- Attachment #3: sungem.patch --]
[-- Type: application/octet-stream, Size: 1182 bytes --]

===== drivers/net/sungem.c 1.71 vs edited =====
--- 1.71/drivers/net/sungem.c	2004-11-14 10:45:36 +01:00
+++ edited/drivers/net/sungem.c	2005-01-03 00:00:22 +01:00
@@ -950,6 +950,7 @@
 	return 0;
 }
 
+/* Called with dev->queue_lock held */
 static int gem_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct gem *gp = dev->priv;
@@ -976,8 +977,16 @@
 		return NETDEV_TX_LOCKED;
 	}
 
+	/* We can now drop queue_lock since tx_lock protects us. But remember
+	 * that queue_lock must be reacquired before we return. Moreover we
+	 * must reacquire it before releasing tx_lock to avoid being called
+	 * with the queue stopped.
+	 */
+	spin_unlock(&dev->queue_lock);
+
 	/* This is a hard error, log it. */
 	if (TX_BUFFS_AVAIL(gp) <= (skb_shinfo(skb)->nr_frags + 1)) {
+		spin_lock(&dev->queue_lock);
 		netif_stop_queue(dev);
 		spin_unlock_irqrestore(&gp->tx_lock, flags);
 		printk(KERN_ERR PFX "%s: BUG! Tx Ring full when queue awake!\n",
@@ -1066,6 +1075,7 @@
 		       dev->name, entry, skb->len);
 	mb();
 	writel(gp->tx_new, gp->regs + TXDMA_KICK);
+	spin_lock(&dev->queue_lock);
 	spin_unlock_irqrestore(&gp->tx_lock, flags);
 
 	dev->trans_start = jiffies;

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]