From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick McHardy Subject: Re: [PATCH] (3/3) netem: adjust parent qlen when duplicating Date: Wed, 04 May 2005 03:57:02 +0200 Message-ID: <42782BEE.2050206@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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------070808090604040309020309" Cc: shemminger@osdl.org, netdev@oss.sgi.com, netem@osdl.org Return-path: To: "David S. Miller" In-Reply-To: <42782B59.3000500@trash.net> Sender: netdev-bounce@oss.sgi.com Errors-to: netdev-bounce@oss.sgi.com List-Id: netdev.vger.kernel.org This is a multi-part message in MIME format. --------------070808090604040309020309 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Patrick McHardy wrote: > David S. Miller wrote: > >>On Wed, 04 May 2005 01:48:02 +0200 >>Patrick McHardy 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 > Oops, I've attached an old patch with a bug. This one is the correct one. --------------070808090604040309020309 Content-Type: text/plain; name="x" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="x" 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 --------------070808090604040309020309--