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:57:02 +0200
Cc: shemminger@xxxxxxxx, netdev@xxxxxxxxxxx, netem@xxxxxxxx
In-reply-to: <42782B59.3000500@trash.net>
References: <20050503162550.30acf31a@dxpl.pdx.osdl.net> <42780AC1.8040409@trash.net> <20050503163025.38bb9682.davem@davemloft.net> <42780DB2.2090201@trash.net> <20050503165937.0c6cac6d.davem@davemloft.net> <42782B59.3000500@trash.net>
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
Patrick McHardy wrote:
> 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>
> 

Oops, I've attached an old patch with a bug. This one is the
correct one.

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,11 +230,9 @@
                         * 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; qp; qp = qp->parentq)
                                qp->q.qlen++;
 
-                       sch->q.qlen++;
                        sch->bstats.bytes += skb2->len;
                        sch->bstats.packets++;
                } else
<Prev in Thread] Current Thread [Next in Thread>