netdev
[Top] [All Lists]

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

To: "David S. Miller" <davem@xxxxxxxxxxxxx>
Subject: Re: [PATCH] (3/3) netem: adjust parent qlen when duplicating
From: Patrick McHardy <kaber@xxxxxxxxx>
Date: Wed, 04 May 2005 03:54:33 +0200
Cc: shemminger@xxxxxxxx, netdev@xxxxxxxxxxx, netem@xxxxxxxx
In-reply-to: <20050503165937.0c6cac6d.davem@xxxxxxxxxxxxx>
References: <20050503162550.30acf31a@xxxxxxxxxxxxxxxxx> <42780AC1.8040409@xxxxxxxxx> <20050503163025.38bb9682.davem@xxxxxxxxxxxxx> <42780DB2.2090201@xxxxxxxxx> <20050503165937.0c6cac6d.davem@xxxxxxxxxxxxx>
Sender: netdev-bounce@xxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.7.7) Gecko/20050420 Debian/1.7.7-2
David S. Miller wrote:
> On Wed, 04 May 2005 01:48:02 +0200
> Patrick McHardy <kaber@xxxxxxxxx> 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@xxxxxxxxx>
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++;
<Prev in Thread] Current Thread [Next in Thread>