: cleanup qdisc_restart

Folks,

I am not a big readability officianado, but this piece of code has become a victim of hairy activities over the last few years. So while i was furiously chasing Herbert's qdisc_is_running changes[1] i made a small cleanup just so that i could absorb what was going on.

The patch included is a small side effect of that effort (theres a lot of WIP as well which is not part of this patch that may never see the light of day).

Actually the patch itself may be slightly unreadable, so i have attached how qdisc_restart looks after the patch.

I am not yet asking for inclusion, but if people are fine with it i will run some performance tests to make sure we at least get the same numbers as before and submit for 2.6.19. I have tested the code with a lot of other stuff but not its the version i am attaching. It does compile however ;->

Also if i am missing some piece of the translation please comment. Alexey, I know you havent looked at this creation of yours in years and a few changes have happened since, so please if you have time stare and comment.

cheers,
jamal

[1] I wanted to check the qdisc is running change before 2.6.18 came out. I have to say I have failed to find any issues with it. There are some theoretical issues but i cant practically create them. Maybe we can chat over a drink.

index 75f02d8..49278c8 100644
@@ -75,8 +75,11 @@ #define MAX_ADDR_LEN	32		/* Largest hard
 
 /* Driver transmit return codes */
 #define NETDEV_TX_OK 0		/* driver took care of packet */
-#define NETDEV_TX_BUSY 1	/* driver tx path was busy*/
-#define NETDEV_TX_LOCKED -1	/* driver tx lock was already taken */
+#define NETDEV_TX_BUSY -1	/* driver tx path was busy*/
+#define NETDEV_TX_LOCKED -2	/* driver tx lock was already taken */
+#define NETDEV_TX_DROP -3      /* request caller to drop packet */
+#define NETDEV_TX_QUEUE -4     /* request caller to requeue packet */
+
 
 /*
  *	Compute the worst case header length according to the protocols

/*
NOTE: Called under dev->queue_lock with locally disabled BH.

__LINK_STATE_QDISC_RUNNING guarantees only one CPU can enter this region at a time.

dev->queue_lock serializes queue accesses for this device AND dev->qdisc pointer itself.

netif_tx_lock serializes accesses to device driver.

dev->queue_lock and netif_tx_lock are mutually exclusive, if one is grabbed, another must be free.

Multiple CPUs may contend for the two locks.

Note, that this procedure can be called by a watchdog timer, so that we do not check dev->tbusy flag here.

Returns to the caller:
0 - queue is empty.
>0 - queue is not empty: tx lock access or queue throttle <0 - Theres room to receive more if !netif_queue_stopped.
(It is upon the caller to check for netif_queue_stopped
before invoking this routine)
*/

int
qdisc_restart(struct net_device *dev)

{
	struct Qdisc *q = dev->qdisc;
	unsigned lockless = (dev->features & NETIF_F_LLTX);
	struct sk_buff *skb = NULL;
	int ret;

	ret = try_get_tx_pkt(&skb, dev, q);
	if (ret >= 0)
		return ret;

	/* we have a packet to send */
	if (!lockless) {
		if (!netif_tx_trylock(dev))
			return handle_tx_locked(skb,dev,q);
	}
		
	/* all clear .. */
	spin_unlock(&dev->queue_lock);

	/* churn baby churn .. */
	ret = dev_hard_start_xmit(skb, dev);

	if (!lockless)  
		netif_tx_unlock(dev);

	spin_lock(&dev->queue_lock);

	/* most likely result, packet went ok */
	if (ret == NETDEV_TX_OK)
		return -1;
	/* only for lockless drivers .. */
	if (ret == NETDEV_TX_LOCKED && lockless) {
		return handle_tx_locked(skb,dev,q);
	}

	if (unlikely (ret != NETDEV_TX_BUSY)) { 
		/* XXX: Do we need a ratelimit? */
		printk("BUG %s code %d qlen %d\n",dev->name,ret,q->q.qlen);
	}

	return handle_dev_requeue(skb,dev,q);
}

From: Stephen Hemminger <[email protected]>
Date: Tue, 15 Aug 2006 07:42:39 -0700

Unfortunately, this is probably very true. So we indeed cannot change the NETDEV_TX_BUSY value.

That slipped from the experiments i was trying out. I can use valid return codes.
The question to ask though is: shouldnt we be having drivers use the mnemonics instead of values like 1?

cheers,
jamal

From: jamal <[email protected]>
Date: Mon, 14 Aug 2006 18:57:27 -0400

Absolutely, but while such conversions still remain undone we shouldn't knowingly break stuff :)