[RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[jamal]

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


In light of Commit: 39e00c5634cd15e14f3e8d2717c4861178dba275
(from Herbert), heres an update of the qdisc_restart cleanup.
Anyone using the batch patch, this is the patch needed on top of the
latest net-2.6, before you apply the batch patch. The old patch wont
apply (its a one line change but patch must be confused)

cheers,
jamal



[-- Attachment #2: rqs-1 --]
[-- Type: text/plain, Size: 7016 bytes --]

[NET_SCHED]: Cleanup readability of qdisc restart

Over the years this code has gotten hairier. Resulting in many long
discussions over long summer days and patches that get it wrong.
This patch helps tame that code so normal people will understand it.

Thanks to Thomas Graf, Peter J. waskiewicz Jr, and Patrick McHardy
for their valuable reviews.

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

---
commit cd2efacc2169986b2e14213718892a54b9e6456a
tree 155bceaaf14d19f3de2f47c1a21b87f4dbb11ac1
parent 8c6cef2fd6f7cb6bb6d8c8c51512d370c636bd47
author Jamal Hadi Salim <hadi@cyberus.ca> Wed, 23 May 2007 22:03:16 -0400
committer Jamal Hadi Salim <hadi@cyberus.ca> Wed, 23 May 2007 22:03:16 -0400

 net/sched/sch_generic.c |  205 ++++++++++++++++++++++++++---------------------
 1 files changed, 112 insertions(+), 93 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index cbefe22..d827486 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -34,6 +34,9 @@
 #include <net/sock.h>
 #include <net/pkt_sched.h>
 
+#define SCHED_TX_DROP -2
+#define SCHED_TX_QUEUE -3
+
 /* Main transmission queue. */
 
 /* Modifications to data participating in scheduling must be protected with
@@ -59,7 +62,74 @@ void qdisc_unlock_tree(struct net_device *dev)
 	spin_unlock_bh(&dev->queue_lock);
 }
 
+static inline int qdisc_qlen(struct Qdisc *q)
+{
+	BUG_ON((int) q->q.qlen < 0);
+	return q->q.qlen;
+}
+
+static inline int handle_dev_cpu_collision(struct net_device *dev)
+{
+	if (unlikely(dev->xmit_lock_owner == smp_processor_id())) {
+		if (net_ratelimit())
+			printk(KERN_WARNING
+			       "Dead loop on netdevice %s, fix it urgently!\n",
+			       dev->name);
+		return SCHED_TX_DROP;
+	}
+	__get_cpu_var(netdev_rx_stat).cpu_collision++;
+	return SCHED_TX_QUEUE;
+}
+
+static inline int
+do_dev_requeue(struct sk_buff *skb, struct net_device *dev, struct Qdisc *q)
+{
+
+	if (unlikely(skb->next))
+		dev->gso_skb = skb;
+	else
+		q->ops->requeue(skb, q);
+	/* XXX: Could netif_schedule fail? Or is that fact we are
+	 * requeueing imply the hardware path is closed
+	 * and even if we fail, some interupt will wake us
+	*/
+	netif_schedule(dev);
+	return 0;
+}
+
+static inline struct sk_buff *
+try_get_tx_pkt(struct net_device *dev, struct Qdisc *q)
+{
+	struct sk_buff *skb = dev->gso_skb;
+
+	if (skb)
+		dev->gso_skb = NULL;
+	else
+		skb = q->dequeue(q);
+
+	return skb;
+}
+
+static inline int
+tx_islocked(struct sk_buff *skb, struct net_device *dev, struct Qdisc *q)
+{
+	int ret = handle_dev_cpu_collision(dev);
+
+	if (ret == SCHED_TX_DROP) {
+		kfree_skb(skb);
+		return qdisc_qlen(q);
+	}
+
+	return do_dev_requeue(skb, dev, q);
+}
+
+
 /*
+   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.
 
@@ -67,116 +137,65 @@ void qdisc_unlock_tree(struct net_device *dev)
 
    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.
 
-/* Kick device.
+   Note, that this procedure can be called by a watchdog timer
 
+   Returns to the caller:
    Returns:  0  - queue is empty or throttled.
 	    >0  - queue is not empty.
 
-   NOTE: Called under dev->queue_lock with locally disabled BH.
 */
 
 static inline int qdisc_restart(struct net_device *dev)
 {
 	struct Qdisc *q = dev->qdisc;
+	unsigned lockless = (dev->features & NETIF_F_LLTX);
 	struct sk_buff *skb;
+	int ret;
 
-	/* Dequeue packet */
-	if (((skb = dev->gso_skb)) || ((skb = q->dequeue(q)))) {
-		unsigned nolock = (dev->features & NETIF_F_LLTX);
-
-		dev->gso_skb = NULL;
-
-		/*
-		 * When the driver has LLTX set it does its own locking
-		 * in start_xmit. No need to add additional overhead by
-		 * 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.
-		 */
-		if (!nolock) {
-			if (!netif_tx_trylock(dev)) {
-			collision:
-				/* So, someone grabbed the driver. */
-
-				/* It may be transient configuration error,
-				   when hard_start_xmit() recurses. We detect
-				   it by checking xmit owner and drop the
-				   packet when deadloop is detected.
-				*/
-				if (dev->xmit_lock_owner == smp_processor_id()) {
-					kfree_skb(skb);
-					if (net_ratelimit())
-						printk(KERN_DEBUG "Dead loop on netdevice %s, fix it urgently!\n", dev->name);
-					goto out;
-				}
-				__get_cpu_var(netdev_rx_stat).cpu_collision++;
-				goto requeue;
-			}
-		}
-
-		{
-			/* And release queue */
-			spin_unlock(&dev->queue_lock);
-
-			if (!netif_queue_stopped(dev)) {
-				int ret;
-
-				ret = dev_hard_start_xmit(skb, dev);
-				if (ret == NETDEV_TX_OK) {
-					if (!nolock) {
-						netif_tx_unlock(dev);
-					}
-					spin_lock(&dev->queue_lock);
-					q = dev->qdisc;
-					goto out;
-				}
-				if (ret == NETDEV_TX_LOCKED && nolock) {
-					spin_lock(&dev->queue_lock);
-					q = dev->qdisc;
-					goto collision;
-				}
-			}
-
-			/* NETDEV_TX_BUSY - we need to requeue */
-			/* Release the driver */
-			if (!nolock) {
-				netif_tx_unlock(dev);
-			}
-			spin_lock(&dev->queue_lock);
-			q = dev->qdisc;
-		}
+	skb = try_get_tx_pkt(dev, q);
+	if (skb == NULL)
+		return 0;
 
-		/* Device kicked us out :(
-		   This is possible in three cases:
-
-		   0. driver is locked
-		   1. fastroute is enabled
-		   2. device cannot determine busy state
-		      before start of transmission (f.e. dialout)
-		   3. device is buggy (ppp)
-		 */
-
-requeue:
-		if (unlikely(q == &noop_qdisc))
-			kfree_skb(skb);
-		else if (skb->next)
-			dev->gso_skb = skb;
-		else
-			q->ops->requeue(skb, q);
-		netif_schedule(dev);
+	/* we have a packet to send */
+	if (!lockless) {
+		if (!netif_tx_trylock(dev))
+			return tx_islocked(skb, dev, q);
 	}
-	return 0;
-
-out:
-	BUG_ON((int) q->q.qlen < 0);
-	return q->q.qlen;
+	/* all clear .. */
+	spin_unlock(&dev->queue_lock);
+
+	ret = NETDEV_TX_BUSY;
+	if (!netif_queue_stopped(dev))
+		/* churn baby churn .. */
+		ret = dev_hard_start_xmit(skb, dev);
+
+	if (!lockless)
+		netif_tx_unlock(dev);
+
+	spin_lock(&dev->queue_lock);
+
+	/* we need to refresh q because it may be invalid since
+	 * we dropped  dev->queue_lock earlier ...
+	 * So dont try to be clever grasshopper
+	*/
+	q = dev->qdisc;
+	/* most likely result, packet went ok */
+	if (ret == NETDEV_TX_OK)
+		return qdisc_qlen(q);
+	/* only for lockless drivers .. */
+	if (ret == NETDEV_TX_LOCKED && lockless)
+		return tx_islocked(skb, dev, q);
+
+	if (unlikely (ret != NETDEV_TX_BUSY && net_ratelimit()))
+		printk(KERN_DEBUG " BUG %s code %d qlen %d\n",dev->name, ret, q->q.qlen);
+
+	return do_dev_requeue(skb, dev, q);
 }
 
+
 void __qdisc_run(struct net_device *dev)
 {
 	do {

Re: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[Patrick McHardy]

jamal wrote:
> +	if (unlikely (ret != NETDEV_TX_BUSY && net_ratelimit()))
> +		printk(KERN_DEBUG " BUG %s code %d qlen %d\n",dev->name, ret, q->q.qlen);


This seems to be based on an old version, your latest patch
changed this to KERN_WARNING.

Re: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[jamal]

On Thu, 2007-24-05 at 05:41 +0200, Patrick McHardy wrote:
> jamal wrote:
> > +	if (unlikely (ret != NETDEV_TX_BUSY && net_ratelimit()))
> > +		printk(KERN_DEBUG " BUG %s code %d qlen %d\n",dev->name, ret, q->q.qlen);
> 
> 
> This seems to be based on an old version, your latest patch
> changed this to KERN_WARNING.

sigh. 
I wont be able to do this change until tonight or tommorow morning. I
think the process i am using to re-generate patches maybe too consuming?
Maybe you could offer some advice. Heres what i do:
1. clone Daves latest tree localy
2. clone another tree from that
3. create patch on second clone
4. compile; test;compile;test until looks reasonable
5. commit with the comments
6. submit
7. build other patches on top (example batching in this case)
8. feedback on patch comes back
9. Re-do steps #2-#6

My steps #2-#6 are expensive; is there a way to optimize that process?
 
cheers,
jamal

RE: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[Waskiewicz Jr, Peter P]

> sigh. 
> I wont be able to do this change until tonight or tommorow 
> morning. I think the process i am using to re-generate 
> patches maybe too consuming?
> Maybe you could offer some advice. Heres what i do:
> 1. clone Daves latest tree localy
> 2. clone another tree from that
> 3. create patch on second clone
> 4. compile; test;compile;test until looks reasonable 5. 
> commit with the comments 6. submit 7. build other patches on 
> top (example batching in this case) 8. feedback on patch 
> comes back 9. Re-do steps #2-#6
> 
> My steps #2-#6 are expensive; is there a way to optimize that process?

1. clone Dave's tree
2. cd into said tree
3. git branch qdiscpatch
4. create patches
5. compile; test; compile until looks reasonable
6. git commit -a
7. git diff (generate patches)
8. git branch batching
9. rinse/repeat 4-7

One tree, multiple branches, very quick to move between them, low
overhead.  If this works for you, also look at stacked GIT (stg) for
pushing and popping patches from your tree - very handy.

Thanks,

-PJ

Re: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[Patrick McHardy]

Waskiewicz Jr, Peter P wrote:
>>sigh. 
>>I wont be able to do this change until tonight or tommorow 
>>morning. I think the process i am using to re-generate 
>>patches maybe too consuming?
>>Maybe you could offer some advice. Heres what i do:
>>1. clone Daves latest tree localy
>>2. clone another tree from that
>>3. create patch on second clone
>>4. compile; test;compile;test until looks reasonable 5. 
>>commit with the comments 6. submit 7. build other patches on 
>>top (example batching in this case) 8. feedback on patch 
>>comes back 9. Re-do steps #2-#6
>>
>>My steps #2-#6 are expensive; is there a way to optimize that process?
> 
> 
> [..]
> 
> One tree, multiple branches, very quick to move between them, low
> overhead.  If this works for you, also look at stacked GIT (stg) for
> pushing and popping patches from your tree - very handy.


I have also found that stgit is a huge time saver, especially for
making changes to patches or when dealing with many patches with
interdependencies.

Re: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[jamal]

On Thu, 2007-24-05 at 18:13 +0200, Patrick McHardy wrote:
> Waskiewicz Jr, Peter P wrote:

> > 
> > One tree, multiple branches, very quick to move between them, low
> > overhead.  If this works for you, also look at stacked GIT (stg) for
> > pushing and popping patches from your tree - very handy.
> 
> 
> I have also found that stgit is a huge time saver, especially for
> making changes to patches or when dealing with many patches with
> interdependencies.

I have had bad experiences with stgit about a year ago - used to mix up
lines between different patches; it may be better now. I think i will
switch to multibranches for now with cogito; wont solve my problem
totaly - but is certainly an improvement over my current process.
I will try stgit at some later point.
Thanks everyone.

cheers,
jamal

[RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[jamal]

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

Dave please apply this patch; against latest net-2.6 

cheers,
jamal


[-- Attachment #2: rqs-2 --]
[-- Type: text/plain, Size: 7018 bytes --]

[NET_SCHED]: Cleanup readability of qdisc restart

Over the years this code has gotten hairier. Resulting in many long
discussions over long summer days and patches that get it wrong.
This patch helps tame that code so normal people will understand it.

Thanks to Thomas Graf, Peter J. waskiewicz Jr, and Patrick McHardy
for their valuable reviews.

Signed-off-by: Jamal Hadi Salim <hadi@cyberus.ca>

---
commit 689af2a1508887470b61cd4f5f24629e796d8436
tree 0bbab1b24b9bd0522f3310b6b99f42c7f02c4d43
parent 04efb8787e4d8a7b21a61aeb723de33154311256
author Jamal Hadi Salim <hadi@cyberus.ca> Thu, 24 May 2007 21:42:50 -0400
committer Jamal Hadi Salim <hadi@cyberus.ca> Thu, 24 May 2007 21:42:50 -0400

 net/sched/sch_generic.c |  205 ++++++++++++++++++++++++++---------------------
 1 files changed, 112 insertions(+), 93 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index cbefe22..5159e0d 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -34,6 +34,9 @@
 #include <net/sock.h>
 #include <net/pkt_sched.h>
 
+#define SCHED_TX_DROP -2
+#define SCHED_TX_QUEUE -3
+
 /* Main transmission queue. */
 
 /* Modifications to data participating in scheduling must be protected with
@@ -59,7 +62,74 @@ void qdisc_unlock_tree(struct net_device *dev)
 	spin_unlock_bh(&dev->queue_lock);
 }
 
+static inline int qdisc_qlen(struct Qdisc *q)
+{
+	BUG_ON((int) q->q.qlen < 0);
+	return q->q.qlen;
+}
+
+static inline int handle_dev_cpu_collision(struct net_device *dev)
+{
+	if (unlikely(dev->xmit_lock_owner == smp_processor_id())) {
+		if (net_ratelimit())
+			printk(KERN_WARNING
+			       "Dead loop on netdevice %s, fix it urgently!\n",
+			       dev->name);
+		return SCHED_TX_DROP;
+	}
+	__get_cpu_var(netdev_rx_stat).cpu_collision++;
+	return SCHED_TX_QUEUE;
+}
+
+static inline int
+do_dev_requeue(struct sk_buff *skb, struct net_device *dev, struct Qdisc *q)
+{
+
+	if (unlikely(skb->next))
+		dev->gso_skb = skb;
+	else
+		q->ops->requeue(skb, q);
+	/* XXX: Could netif_schedule fail? Or is that fact we are
+	 * requeueing imply the hardware path is closed
+	 * and even if we fail, some interupt will wake us
+	*/
+	netif_schedule(dev);
+	return 0;
+}
+
+static inline struct sk_buff *
+try_get_tx_pkt(struct net_device *dev, struct Qdisc *q)
+{
+	struct sk_buff *skb = dev->gso_skb;
+
+	if (skb)
+		dev->gso_skb = NULL;
+	else
+		skb = q->dequeue(q);
+
+	return skb;
+}
+
+static inline int
+tx_islocked(struct sk_buff *skb, struct net_device *dev, struct Qdisc *q)
+{
+	int ret = handle_dev_cpu_collision(dev);
+
+	if (ret == SCHED_TX_DROP) {
+		kfree_skb(skb);
+		return qdisc_qlen(q);
+	}
+
+	return do_dev_requeue(skb, dev, q);
+}
+
+
 /*
+   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.
 
@@ -67,116 +137,65 @@ void qdisc_unlock_tree(struct net_device *dev)
 
    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.
 
-/* Kick device.
+   Note, that this procedure can be called by a watchdog timer
 
+   Returns to the caller:
    Returns:  0  - queue is empty or throttled.
 	    >0  - queue is not empty.
 
-   NOTE: Called under dev->queue_lock with locally disabled BH.
 */
 
 static inline int qdisc_restart(struct net_device *dev)
 {
 	struct Qdisc *q = dev->qdisc;
+	unsigned lockless = (dev->features & NETIF_F_LLTX);
 	struct sk_buff *skb;
+	int ret;
 
-	/* Dequeue packet */
-	if (((skb = dev->gso_skb)) || ((skb = q->dequeue(q)))) {
-		unsigned nolock = (dev->features & NETIF_F_LLTX);
-
-		dev->gso_skb = NULL;
-
-		/*
-		 * When the driver has LLTX set it does its own locking
-		 * in start_xmit. No need to add additional overhead by
-		 * 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.
-		 */
-		if (!nolock) {
-			if (!netif_tx_trylock(dev)) {
-			collision:
-				/* So, someone grabbed the driver. */
-
-				/* It may be transient configuration error,
-				   when hard_start_xmit() recurses. We detect
-				   it by checking xmit owner and drop the
-				   packet when deadloop is detected.
-				*/
-				if (dev->xmit_lock_owner == smp_processor_id()) {
-					kfree_skb(skb);
-					if (net_ratelimit())
-						printk(KERN_DEBUG "Dead loop on netdevice %s, fix it urgently!\n", dev->name);
-					goto out;
-				}
-				__get_cpu_var(netdev_rx_stat).cpu_collision++;
-				goto requeue;
-			}
-		}
-
-		{
-			/* And release queue */
-			spin_unlock(&dev->queue_lock);
-
-			if (!netif_queue_stopped(dev)) {
-				int ret;
-
-				ret = dev_hard_start_xmit(skb, dev);
-				if (ret == NETDEV_TX_OK) {
-					if (!nolock) {
-						netif_tx_unlock(dev);
-					}
-					spin_lock(&dev->queue_lock);
-					q = dev->qdisc;
-					goto out;
-				}
-				if (ret == NETDEV_TX_LOCKED && nolock) {
-					spin_lock(&dev->queue_lock);
-					q = dev->qdisc;
-					goto collision;
-				}
-			}
-
-			/* NETDEV_TX_BUSY - we need to requeue */
-			/* Release the driver */
-			if (!nolock) {
-				netif_tx_unlock(dev);
-			}
-			spin_lock(&dev->queue_lock);
-			q = dev->qdisc;
-		}
+	skb = try_get_tx_pkt(dev, q);
+	if (skb == NULL)
+		return 0;
 
-		/* Device kicked us out :(
-		   This is possible in three cases:
-
-		   0. driver is locked
-		   1. fastroute is enabled
-		   2. device cannot determine busy state
-		      before start of transmission (f.e. dialout)
-		   3. device is buggy (ppp)
-		 */
-
-requeue:
-		if (unlikely(q == &noop_qdisc))
-			kfree_skb(skb);
-		else if (skb->next)
-			dev->gso_skb = skb;
-		else
-			q->ops->requeue(skb, q);
-		netif_schedule(dev);
+	/* we have a packet to send */
+	if (!lockless) {
+		if (!netif_tx_trylock(dev))
+			return tx_islocked(skb, dev, q);
 	}
-	return 0;
-
-out:
-	BUG_ON((int) q->q.qlen < 0);
-	return q->q.qlen;
+	/* all clear .. */
+	spin_unlock(&dev->queue_lock);
+
+	ret = NETDEV_TX_BUSY;
+	if (!netif_queue_stopped(dev))
+		/* churn baby churn .. */
+		ret = dev_hard_start_xmit(skb, dev);
+
+	if (!lockless)
+		netif_tx_unlock(dev);
+
+	spin_lock(&dev->queue_lock);
+
+	/* we need to refresh q because it may be invalid since
+	 * we dropped  dev->queue_lock earlier ...
+	 * So dont try to be clever grasshopper
+	*/
+	q = dev->qdisc;
+	/* most likely result, packet went ok */
+	if (ret == NETDEV_TX_OK)
+		return qdisc_qlen(q);
+	/* only for lockless drivers .. */
+	if (ret == NETDEV_TX_LOCKED && lockless)
+		return tx_islocked(skb, dev, q);
+
+	if (unlikely (ret != NETDEV_TX_BUSY && net_ratelimit()))
+		printk(KERN_WARNING " BUG %s code %d qlen %d\n",dev->name, ret, q->q.qlen);
+
+	return do_dev_requeue(skb, dev, q);
 }
 
+
 void __qdisc_run(struct net_device *dev)
 {
 	do {

Re: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Thu, 24 May 2007 21:58:00 -0400

> Dave please apply this patch; against latest net-2.6 

Jamal, as much as I love your patch, it's a cleanup and doesn't
fix any bugs and we're outside of the merge window for 2.6.22

I'll queue this up for sure when I open up the 2.6.23 tree.

Thanks.

Re: [RESEND][PATCH] [NET_SCHED]make qdisc_restart more readable

[jamal]

On Thu, 2007-24-05 at 19:03 -0700, David Miller wrote:

> I'll queue this up for sure when I open up the 2.6.23 tree.

Thats good enough - thanks. 

cheers,
jamal