[PATCH] netem: restart device after inserting packets

[PATCH] netem: restart device after inserting packets

[Stephen Hemminger]

The version of netem in 2.6.10 moves packets from the delayed queue
to the qdisc in a timer interrupt. But it forgot to force the device to
pick them up.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/net/sched/sch_netem.c b/net/sched/sch_netem.c
--- a/net/sched/sch_netem.c	2004-12-08 12:29:12 -08:00
+++ b/net/sched/sch_netem.c	2004-12-08 12:29:12 -08:00
@@ -258,12 +258,13 @@
 {
 	struct Qdisc *sch = (struct Qdisc *)arg;
 	struct netem_sched_data *q = qdisc_priv(sch);
+	struct net_device *dev = sch->dev;
 	struct sk_buff *skb;
 	psched_time_t now;
 
 	pr_debug("netem_watchdog: fired @%lu\n", jiffies);
 
-	spin_lock_bh(&sch->dev->queue_lock);
+	spin_lock_bh(&dev->queue_lock);
 	PSCHED_GET_TIME(now);
 
 	while ((skb = skb_peek(&q->delayed)) != NULL) {
@@ -286,7 +287,8 @@
 		else
 			sch->q.qlen++;
 	}
-	spin_unlock_bh(&sch->dev->queue_lock);
+	qdisc_restart(dev);
+	spin_unlock_bh(&dev->queue_lock);
 }
 
 static void netem_reset(struct Qdisc *sch)

Re: [PATCH] netem: restart device after inserting packets

[David S. Miller]

On Wed, 8 Dec 2004 12:31:03 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> The version of netem in 2.6.10 moves packets from the delayed queue
> to the qdisc in a timer interrupt. But it forgot to force the device to
> pick them up.
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Good spotting.  Applied, thanks Stephen.

Re: [PATCH] netem: restart device after inserting packets

[Patrick McHardy]

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

David S. Miller wrote:

>On Wed, 8 Dec 2004 12:31:03 -0800
>Stephen Hemminger <shemminger@osdl.org> wrote:
>  
>
>>The version of netem in 2.6.10 moves packets from the delayed queue
>>to the qdisc in a timer interrupt. But it forgot to force the device to
>>pick them up.
>>
>>Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
>>
>
>Good spotting.  Applied, thanks Stephen.
>
The patch is incomplete, netem may dequeue multiple packets from
the delayed queue at once and feed them to the inner queue, but
qdisc_restart will only dequeue one packet from the inner queue.
This patch moves qdisc_run back to include/net/pkt_sched.h and
replaces qdisc_restart by qdisc_run in netem_watchdog.

Regards
Patrick


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2062 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/12/10 04:24:22+01:00 kaber@coreworks.de 
#   [PKT_SCHED]: Keep netem queue running until inner qdisc is empty
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/sched/sch_netem.c
#   2004/12/10 04:24:13+01:00 kaber@coreworks.de +1 -1
#   [PKT_SCHED]: Keep netem queue running until inner qdisc is empty
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/core/dev.c
#   2004/12/10 04:24:13+01:00 kaber@coreworks.de +0 -7
#   [PKT_SCHED]: Keep netem queue running until inner qdisc is empty
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# include/net/pkt_sched.h
#   2004/12/10 04:24:13+01:00 kaber@coreworks.de +6 -0
#   [PKT_SCHED]: Keep netem queue running until inner qdisc is empty
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/include/net/pkt_sched.h b/include/net/pkt_sched.h
--- a/include/net/pkt_sched.h	2004-12-10 04:24:54 +01:00
+++ b/include/net/pkt_sched.h	2004-12-10 04:24:54 +01:00
@@ -228,6 +228,12 @@
 
 extern int qdisc_restart(struct net_device *dev);
 
+static inline void qdisc_run(struct net_device *dev)
+{
+	while (!netif_queue_stopped(dev) && qdisc_restart(dev) < 0)
+		/* NOTHING */;
+}
+
 extern int tc_classify(struct sk_buff *skb, struct tcf_proto *tp,
 	struct tcf_result *res);
 
diff -Nru a/net/core/dev.c b/net/core/dev.c
--- a/net/core/dev.c	2004-12-10 04:24:54 +01:00
+++ b/net/core/dev.c	2004-12-10 04:24:54 +01:00
@@ -1202,13 +1202,6 @@
 	}						\
 }
 
-static inline void qdisc_run(struct net_device *dev)
-{
-	while (!netif_queue_stopped(dev) &&
-	       qdisc_restart(dev)<0)
-		/* NOTHING */;
-}
-
 /**
  *	dev_queue_xmit - transmit a buffer
  *	@skb: buffer to transmit
diff -Nru a/net/sched/sch_netem.c b/net/sched/sch_netem.c
--- a/net/sched/sch_netem.c	2004-12-10 04:24:54 +01:00
+++ b/net/sched/sch_netem.c	2004-12-10 04:24:54 +01:00
@@ -287,7 +287,7 @@
 		else
 			sch->q.qlen++;
 	}
-	qdisc_restart(dev);
+	qdisc_run(dev);
 	spin_unlock_bh(&dev->queue_lock);
 }
 

Re: [PATCH] netem: restart device after inserting packets

[Stephen Hemminger]

On Fri, 10 Dec 2004 04:33:21 +0100
Patrick McHardy <kaber@trash.net> wrote:

> David S. Miller wrote:
> 
> >On Wed, 8 Dec 2004 12:31:03 -0800
> >Stephen Hemminger <shemminger@osdl.org> wrote:
> >  
> >
> >>The version of netem in 2.6.10 moves packets from the delayed queue
> >>to the qdisc in a timer interrupt. But it forgot to force the device to
> >>pick them up.
> >>
> >>Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> >>
> >
> >Good spotting.  Applied, thanks Stephen.
> >
> The patch is incomplete, netem may dequeue multiple packets from
> the delayed queue at once and feed them to the inner queue, but
> qdisc_restart will only dequeue one packet from the inner queue.
> This patch moves qdisc_run back to include/net/pkt_sched.h and
> replaces qdisc_restart by qdisc_run in netem_watchdog.

Yes, I wasn't running big enough delays to notice.

Re: [PATCH] netem: restart device after inserting packets

[David S. Miller]

On Fri, 10 Dec 2004 04:33:21 +0100
Patrick McHardy <kaber@trash.net> wrote:

> The patch is incomplete, netem may dequeue multiple packets from
> the delayed queue at once and feed them to the inner queue, but
> qdisc_restart will only dequeue one packet from the inner queue.
> This patch moves qdisc_run back to include/net/pkt_sched.h and
> replaces qdisc_restart by qdisc_run in netem_watchdog.

Applied, thanks Patrick.

Don't we need 2.4.x versions of these two fixes?

Re: [PATCH] netem: restart device after inserting packets

[Stephen Hemminger]

2.4 version of the netem wakeup patch. Also fixes the qlen
in a couple of places. This makes code basically same as 2.6

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

diff -Nru a/net/sched/sch_netem.c b/net/sched/sch_netem.c
--- a/net/sched/sch_netem.c	2004-12-14 13:10:18 -08:00
+++ b/net/sched/sch_netem.c	2004-12-14 13:10:18 -08:00
@@ -259,12 +259,13 @@
 {
 	struct Qdisc *sch = (struct Qdisc *)arg;
 	struct netem_sched_data *q = qdisc_priv(sch);
+	struct net_device *dev = sch->dev;
 	struct sk_buff *skb;
 	psched_time_t now;
 
 	pr_debug("netem_watchdog: fired @%lu\n", jiffies);
 
-	spin_lock_bh(&sch->dev->queue_lock);
+	spin_lock_bh(&dev->queue_lock);
 	PSCHED_GET_TIME(now);
 
 	while ((skb = skb_peek(&q->delayed)) != NULL) {
@@ -284,8 +285,11 @@
 
 		if (q->qdisc->enqueue(skb, q->qdisc))
 			sch->stats.drops++;
+		else
+			sch->q.qlen++;
 	}
-	spin_unlock_bh(&sch->dev->queue_lock);
+	qdisc_run(dev);
+	spin_unlock_bh(&dev->queue_lock);
 }
 
 static void netem_reset(struct Qdisc *sch)
@@ -505,7 +509,7 @@
 	sch_tree_lock(sch);
 	*old = xchg(&q->qdisc, new);
 	qdisc_reset(*old);
-	sch->q.qlen = q->delayed.qlen;
+	sch->q.qlen = 0;
 	sch_tree_unlock(sch);
 
 	return 0;

Re: [PATCH] netem: restart device after inserting packets

[David S. Miller]

On Tue, 14 Dec 2004 13:11:13 -0800
Stephen Hemminger <shemminger@osdl.org> wrote:

> 2.4 version of the netem wakeup patch. Also fixes the qlen
> in a couple of places. This makes code basically same as 2.6
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Applied, thanks a lot Stephen.