Re: [PATCH] (3/3) netem: adjust parent qlen when duplicating

[Patrick McHardy <kaber@trash.net>]

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

David S. Miller wrote:
> On Wed, 04 May 2005 01:48:02 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
> 
>>That's what I already suggested, it should be pretty simple to do
>>so. I'll send a patch once your tree appears on kernel.org.

This one should work. It keeps a pointer to the parent qdisc in struct
Qdisc and adjusts q.qlen of all parent qdiscs in netem. The __parent
pointer also used by CBQ is renamed to parentq and is used for this. To
avoid confusion, the parent classid is also renamed to parentcl. It
should work with all currently included classful qdiscs except HFSC.
Statistics are not correctly updated (and can't be without support from
the qdisc since classes are internal to it), we need to keep this in
mind in case a qdisc like RED which uses qstats.backlog for calculations
is converted to a classful one. Fixing HFSC is very low priority, it
could only use netem in work-conserving mode anyway.

My favourite solution would be to avoid this hack and let tc actions
handle duplication, as Jamal suggested. My previous point against this
of not necessarily having an identical classification result for the
duplicated packet as the original one is actually a plus since it
provides more flexibility. Steven, what do you think about it?

Signed-off-by: Patrick McHardy <kaber@trash.net>

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

Index: include/net/sch_generic.h
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/include/net/sch_generic.h  (mode:100644 sha1:c57504b3b51819522dc9f868aa9a208d61dd3892)
+++ uncommitted/include/net/sch_generic.h  (mode:100644)
@@ -35,7 +35,7 @@
 	int			padded;
 	struct Qdisc_ops	*ops;
 	u32			handle;
-	u32			parent;
+	u32			parentcl;
 	atomic_t		refcnt;
 	struct sk_buff_head	q;
 	struct net_device	*dev;
@@ -49,10 +49,11 @@
 	int			(*reshape_fail)(struct sk_buff *skb,
 					struct Qdisc *q);
 
-	/* This field is deprecated, but it is still used by CBQ
+	/* This field is deprecated, but it is still used by CBQ and netem
 	 * and it will live until better solution will be invented.
+	 * Valid only while qdisc is grafted to its parent.
 	 */
-	struct Qdisc		*__parent;
+	struct Qdisc		*parentq;
 };
 
 struct Qdisc_class_ops
Index: net/sched/sch_api.c
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/net/sched/sch_api.c  (mode:100644 sha1:07977f8f2679b30cf93808f93fbbcce3c3dbc776)
+++ uncommitted/net/sched/sch_api.c  (mode:100644)
@@ -378,9 +378,11 @@
 		if (cops) {
 			unsigned long cl = cops->get(parent, classid);
 			if (cl) {
+				if (new) {
+					new->parentcl = classid;
+					new->parentq = parent;
+				}
 				err = cops->graft(parent, cl, new, old);
-				if (new)
-					new->parent = classid;
 				cops->put(parent, cl);
 			}
 		}
@@ -855,7 +857,7 @@
 				q_idx++;
 				continue;
 			}
-			if (tc_fill_qdisc(skb, q, q->parent, NETLINK_CB(cb->skb).pid,
+			if (tc_fill_qdisc(skb, q, q->parentcl, NETLINK_CB(cb->skb).pid,
 					  cb->nlh->nlmsg_seq, NLM_F_MULTI, RTM_NEWQDISC) <= 0) {
 				read_unlock_bh(&qdisc_tree_lock);
 				goto done;
@@ -1289,7 +1291,6 @@
 
 subsys_initcall(pktsched_init);
 
-EXPORT_SYMBOL(qdisc_lookup);
 EXPORT_SYMBOL(qdisc_get_rtab);
 EXPORT_SYMBOL(qdisc_put_rtab);
 EXPORT_SYMBOL(register_qdisc);
Index: net/sched/sch_cbq.c
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/net/sched/sch_cbq.c  (mode:100644 sha1:d43e3b8cbf6af27a25ab7b9d2aee82a32f8010eb)
+++ uncommitted/net/sched/sch_cbq.c  (mode:100644)
@@ -419,9 +419,6 @@
 		return ret;
 	}
 
-#ifdef CONFIG_NET_CLS_POLICE
-	cl->q->__parent = sch;
-#endif
 	if ((ret = cl->q->enqueue(skb, cl->q)) == NET_XMIT_SUCCESS) {
 		sch->q.qlen++;
 		sch->bstats.packets++;
@@ -456,7 +453,6 @@
 
 #ifdef CONFIG_NET_CLS_POLICE
 	q->rx_class = cl;
-	cl->q->__parent = sch;
 #endif
 	if ((ret = cl->q->ops->requeue(skb, cl->q)) == 0) {
 		sch->q.qlen++;
@@ -690,7 +686,7 @@
 static int cbq_reshape_fail(struct sk_buff *skb, struct Qdisc *child)
 {
 	int len = skb->len;
-	struct Qdisc *sch = child->__parent;
+	struct Qdisc *sch = child->parentq;
 	struct cbq_sched_data *q = qdisc_priv(sch);
 	struct cbq_class *cl = q->rx_class;
 
@@ -701,7 +697,6 @@
 		cbq_mark_toplevel(q, cl);
 
 		q->rx_class = cl;
-		cl->q->__parent = sch;
 
 		if (cl->q->enqueue(skb, cl->q) == 0) {
 			sch->q.qlen++;
Index: net/sched/sch_generic.c
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/net/sched/sch_generic.c  (mode:100644 sha1:87e48a4e105133ca3d0407b5c2d84f1b0e3a72c4)
+++ uncommitted/net/sched/sch_generic.c  (mode:100644)
@@ -501,7 +501,7 @@
 	/* unlink inner qdiscs from dev->qdisc_list immediately */
 	list_for_each_entry(cq, &cql, list)
 		list_for_each_entry_safe(q, n, &qdisc->dev->qdisc_list, list)
-			if (TC_H_MAJ(q->parent) == TC_H_MAJ(cq->handle)) {
+			if (TC_H_MAJ(q->parentcl) == TC_H_MAJ(cq->handle)) {
 				if (q->ops->cl_ops == NULL)
 					list_del_init(&q->list);
 				else
Index: net/sched/sch_netem.c
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/net/sched/sch_netem.c  (mode:100644 sha1:e0c9fbe73b15cee32b44f4469e1ac5eeb9849267)
+++ uncommitted/net/sched/sch_netem.c  (mode:100644)
@@ -230,8 +230,7 @@
 			 * queue, the parent's qlen accounting gets confused,
 			 * so fix it.
 			 */
-			qp = qdisc_lookup(sch->dev, TC_H_MAJ(sch->parent));
-			if (qp)
+			for (qp = sch->parentq; qp; qp = qp->parentq)
 				qp->q.qlen++;
 
 			sch->q.qlen++;