net_sched: Add qdisc __NET_XMIT_STOLEN flag
Subsystems (4)
PATCH 1/1 net_sched: Add qdisc __NET_XMIT_STOLEN flag
Hi,
Here is a complete, I hope, version of the patch to not linger this all to much, but of course any redoing is possible.
Thanks,
Jarek P.
----------------->
Subject: net_sched: Add qdisc __NET_XMIT_STOLEN flag
Patrick McHardy <[email protected]> noticed:
"The other problem that affects all qdiscs supporting actions is TC_ACT_QUEUED/TC_ACT_STOLEN getting mapped to NET_XMIT_SUCCESS even though the packet is not queued, corrupting upper qdiscs' qlen counters."
and later explained:
"The reason why it translates it at all seems to be to not increase the drops counter. Within a single qdisc this could be avoided by other means easily, upper qdiscs would still increase the counter when we return anything besides NET_XMIT_SUCCESS though.
This means we need a new NET_XMIT return value to indicate this to the upper qdiscs. So I'd suggest to introduce NET_XMIT_STOLEN, return that to upper qdiscs and translate it to NET_XMIT_SUCCESS in dev_queue_xmit, similar to NET_XMIT_BYPASS."
David Miller <[email protected]> noticed:
"Maybe these NET_XMIT_* values being passed around should be a set of bits. They could be composed of base meanings, combined with specific attributes.
So you could say "NET_XMIT_DROP | __NET_XMIT_NO_DROP_COUNT"
The attributes get masked out by the top-level ->enqueue() caller, such that the base meanings are the only thing that make their way up into the stack. If it's only about communication within the qdisc tree, let's simply code it that way."
This patch is trying to realize these ideas.
---
include/linux/netdevice.h | 1 + include/net/sch_generic.h | 14 +++++++++++++- net/sched/sch_atm.c | 12 +++++++----- net/sched/sch_cbq.c | 23 +++++++++++++++-------- net/sched/sch_dsmark.c | 8 +++++--- net/sched/sch_hfsc.c | 8 +++++--- net/sched/sch_htb.c | 18 +++++++++++------- net/sched/sch_netem.c | 3 ++- net/sched/sch_prio.c | 8 +++++--- net/sched/sch_red.c | 2 +- net/sched/sch_sfq.c | 2 +- net/sched/sch_tbf.c | 3 ++- 12 files changed, 68 insertions(+), 34 deletions(-)
include/linux/netdevice.h +1 -0
@@ -64,6 +64,7 @@ struct wireless_dev;#define NET_XMIT_BYPASS 4/* packet does not leave via dequeue; (TC use only - dev_queue_xmit returns this as NET_XMIT_SUCCESS) */ +#define NET_XMIT_MASK 0xFFFF/* qdisc flags in net/sch_generic.h */ /* Backlog congestion levels */ #define NET_RX_SUCCESS 0/* keep 'em coming, baby */
include/net/sch_generic.h +13 -1
@@ -331,6 +331,18 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } +#ifdef CONFIG_NET_CLS_ACT +/* additional qdisc xmit flags */ +enum net_xmit_qdisc_t { + __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , +}; + +#define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) + +#else +#define net_xmit_drop_count (e) (1) +#endif +static inline int qdisc_enqueue (struct sk_buff *skb,struct Qdisc *sch) {#ifdef CONFIG_NET_SCHED
@@ -343,7 +355,7 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) {qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch); +return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ; } static inline int __qdisc_enqueue_tail(struct sk_buff *skb, struct Qdisc *sch,
net/sched/sch_atm.c +7 -5
@@ -415,7 +415,7 @@ static int atm_tc_enqueue(struct sk_buff *skb, struct Qdisc *sch)case TC_ACT_QUEUED :case TC_ACT_STOLEN :kfree_skb (skb); -return NET_XMIT_SUCCESS ; +return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :kfree_skb (skb); goto drop;
@@ -432,9 +432,11 @@ static int atm_tc_enqueue(struct sk_buff *skb, struct Qdisc *sch) ret =qdisc_enqueue (skb, flow->q);if (ret !=0 ) { drop:__maybe_unused - sch->qstats.drops++; -if (flow) - flow->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; +if (flow) + flow->qstats.drops++; + }return ret; } sch->bstats.bytes +=qdisc_pkt_len (skb);
@@ -530,7 +532,7 @@ static int atm_tc_requeue(struct sk_buff *skb, struct Qdisc *sch)if (!ret) { sch->q.qlen++; sch->qstats.requeues++; - }else { + }else if (net_xmit_drop_count (ret)) { sch->qstats.drops++; p->link.qstats.drops++; }
net/sched/sch_cbq.c +15 -8
@@ -256,7 +256,7 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ;case TC_ACT_RECLASSIFY :
@@ -397,9 +397,11 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch)return ret; } - sch->qstats.drops++; -cbq_mark_toplevel (q, cl); - cl->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; +cbq_mark_toplevel (q, cl); + cl->qstats.drops++; + }return ret; }
@@ -430,8 +432,10 @@ cbq_requeue(struct sk_buff *skb, struct Qdisc *sch)cbq_activate_class (cl);return 0 ; } - sch->qstats.drops++; - cl->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; + cl->qstats.drops++; + }return ret; }
@@ -664,13 +668,15 @@ static int cbq_reshape_fail(struct sk_buff *skb, struct Qdisc *child) q->rx_class =NULL ;if (cl && (cl =cbq_reclassify (skb, cl)) !=NULL ) { +int ret;cbq_mark_toplevel (q, cl); q->rx_class = cl; cl->q->__parent = sch; -if (qdisc_enqueue (skb, cl->q) ==0 ) { + ret =qdisc_enqueue (skb, cl->q); +if (ret ==NET_XMIT_SUCCESS ) { sch->q.qlen++; sch->bstats.packets++; sch->bstats.bytes +=qdisc_pkt_len (skb);
@@ -678,7 +684,8 @@ static int cbq_reshape_fail(struct sk_buff *skb, struct Qdisc *child)cbq_activate_class (cl);return 0 ; } - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return 0 ; }
net/sched/sch_dsmark.c +5 -3
@@ -236,7 +236,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch)case TC_ACT_QUEUED :case TC_ACT_STOLEN :kfree_skb (skb); -return NET_XMIT_SUCCESS ; +return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT : goto drop;
@@ -254,7 +254,8 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch) err =qdisc_enqueue (skb, p->q);if (err !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; +if (net_xmit_drop_count (err)) + sch->qstats.drops++;return err; }
@@ -321,7 +322,8 @@ static int dsmark_requeue(struct sk_buff *skb, struct Qdisc *sch) err = p->q->ops->requeue (skb, p->q);if (err !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; +if (net_xmit_drop_count (err)) + sch->qstats.drops++;return err; }
net/sched/sch_hfsc.c +5 -3
@@ -1166,7 +1166,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ; }
@@ -1586,8 +1586,10 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch) err =qdisc_enqueue (skb, cl->qdisc);if (unlikely (err !=NET_XMIT_SUCCESS )) { - cl->qstats.drops++; - sch->qstats.drops++; +if (net_xmit_drop_count (err)) { + cl->qstats.drops++; + sch->qstats.drops++; + }return err; }
net/sched/sch_htb.c +11 -7
@@ -221,7 +221,7 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ; }
@@ -572,9 +572,11 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch)kfree_skb (skb);return ret;#endif - }else if (qdisc_enqueue (skb, cl->un.leaf.q) !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; - cl->qstats.drops++; + }else if ((ret =qdisc_enqueue (skb, cl->un.leaf.q)) !=NET_XMIT_SUCCESS ) { +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; + cl->qstats.drops++; + }return NET_XMIT_DROP ; }else { cl->bstats.packets +=
@@ -615,10 +617,12 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch)kfree_skb (skb);return ret;#endif - }else if (cl->un.leaf.q->ops->requeue (skb, cl->un.leaf.q) != + }else if ((ret = cl->un.leaf.q->ops->requeue (skb, cl->un.leaf.q)) !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; - cl->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; + cl->qstats.drops++; + }return NET_XMIT_DROP ; }else htb_activate (q, cl);
net/sched/sch_netem.c +2 -1
@@ -240,8 +240,9 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch) sch->q.qlen++; sch->bstats.bytes +=qdisc_pkt_len (skb); sch->bstats.packets++; - }else + }else if (net_xmit_drop_count (ret)) { sch->qstats.drops++; + }pr_debug ("netem: enqueue ret %d\n" , ret);return ret;
net/sched/sch_prio.c +5 -3
@@ -45,7 +45,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)switch (err) {case TC_ACT_STOLEN :case TC_ACT_QUEUED : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ; }
@@ -88,7 +88,8 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch) sch->q.qlen++;return NET_XMIT_SUCCESS ; } - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return ret; }
@@ -114,7 +115,8 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch) sch->qstats.requeues++;return 0 ; } - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return NET_XMIT_DROP ; }
net/sched/sch_red.c +1 -1
@@ -97,7 +97,7 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc* sch) sch->bstats.bytes +=qdisc_pkt_len (skb); sch->bstats.packets++; sch->q.qlen++; - }else { + }else if (net_xmit_drop_count (ret)) { q->stats.pdrop++; sch->qstats.drops++; }
net/sched/sch_sfq.c +1 -1
@@ -178,7 +178,7 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,switch (result) {case TC_ACT_STOLEN :case TC_ACT_QUEUED : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return 0 ; }
net/sched/sch_tbf.c +2 -1
@@ -135,7 +135,8 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc* sch) ret =qdisc_enqueue (skb, q->qdisc);if (ret !=0 ) { - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return ret; }
Quoted 4 lines · Jarek Poplawski
Hi,
Here is a complete, I hope, version of the patch to not linger this all to much, but of course any redoing is possible.
This looks good to me. Just one suggestion:
Quoted 16 lines
include/net/sch_generic.h +12
-0+#ifdef CONFIG_NET_CLS_ACT +/* additional qdisc xmit flags */ +enum net_xmit_qdisc_t { + __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , +}; + +#define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) + +#else +#define net_xmit_drop_count (e) (1) +#endif +static inline int qdisc_enqueue (struct sk_buff *skb,struct Qdisc *sch) {#ifdef CONFIG_NET_SCHED
include/net/sch_generic.h static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch) · line 358
Quoted 5 lines
include/net/sch_generic.h +1
-1@@ -343,7 +355,7 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) {qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch); +return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ;
It would be nice to handle NET_XMIT_BYPASS similar, currently its mapped to NET_XMIT_SUCCESS by dev_queue_xmit(). Simply adding it to net_xmit_qdisc_t as
__NET_XMIT_BYPASS = NET_XMIT_MASK + 0x2
and removing the mapping from dev_queue_xmit() should work I think.
Quoted 34 lines · Patrick McHardy
Quoted 4 lines · Jarek Poplawski
Hi,
Here is a complete, I hope, version of the patch to not linger this all to much, but of course any redoing is possible.
This looks good to me. Just one suggestion:
Quoted 16 lines
include/net/sch_generic.h +12
-0+#ifdef CONFIG_NET_CLS_ACT +/* additional qdisc xmit flags */ +enum net_xmit_qdisc_t { + __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , +}; + +#define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) + +#else +#define net_xmit_drop_count (e) (1) +#endif +static inline int qdisc_enqueue (struct sk_buff *skb,struct Qdisc *sch) {#ifdef CONFIG_NET_SCHED include/net/sch_generic.h static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch) · line 358
Quoted 5 lines
include/net/sch_generic.h +1
-1@@ -343,7 +355,7 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) {qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch); +return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ;It would be nice to handle
NET_XMIT_BYPASSsimilar, currently its mapped toNET_XMIT_SUCCESSbydev_queue_xmit(). Simply adding it tonet_xmit_qdisc_tas
Quoted 3 lines
__NET_XMIT_BYPASS=NET_XMIT_MASK+0x2and removing the mapping from
dev_queue_xmit()should work I think.
OK, I think, I'll prepare a separate patch for this.
Thanks,
Jarek P.
PATCH 1/1 net_sched: Add qdisc __NET_XMIT_BYPASS flag
In reply to Patrick McHardyHere is this additional patch. I hope I got it mostly right...
BTW, after this patch the only user of NET_XMIT_BYPASS seems to be dst_input(), but I can't see where it gets this (even before this patch).
Jarek P.
------------------>
Subject: net_sched: Add qdisc __NET_XMIT_BYPASS flag
Patrick McHardy <[email protected]> noticed that it would be nice to handle NET_XMIT_BYPASS by NET_XMIT_SUCCESS with an internal qdisc flag __NET_XMIT_BYPASS and to remove the mapping from dev_queue_xmit().
---
(apply on top of [PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag)
include/net/sch_generic.h | 6 +++--- net/core/dev.c | 1 - net/sched/sch_atm.c | 2 +- net/sched/sch_cbq.c | 4 ++-- net/sched/sch_dsmark.c | 2 +- net/sched/sch_hfsc.c | 4 ++-- net/sched/sch_htb.c | 6 +++--- net/sched/sch_netem.c | 2 +- net/sched/sch_prio.c | 6 +++--- net/sched/sch_sfq.c | 6 +++--- 10 files changed, 19 insertions(+), 20 deletions(-)
include/net/sch_generic.h +3 -3
@@ -331,14 +331,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } -#ifdef CONFIG_NET_CLS_ACT /* additional qdisc xmit flags */ enum net_xmit_qdisc_t { - __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , + __NET_XMIT_STOLEN =NET_XMIT_MASK +0x1 , + __NET_XMIT_BYPASS =NET_XMIT_MASK +0x2 , }; +#ifdef CONFIG_NET_CLS_ACT #define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) -#else #define net_xmit_drop_count (e) (1)#endif
net/core/dev.c +0 -1
@@ -1805,7 +1805,6 @@ gso:spin_unlock (root_lock); - rc = rc ==NET_XMIT_BYPASS ?NET_XMIT_SUCCESS : rc; goto out; }
net/sched/sch_atm.c +1 -1
@@ -457,7 +457,7 @@ drop: __maybe_unusedreturn 0 ; }tasklet_schedule (&p->task); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }/*
net/sched/sch_cbq.c +2 -2
@@ -230,7 +230,7 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) (cl = cbq_class_lookup(q, prio)) != NULL)return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;for (;;) {int result =0 ; defmap = head->defaults;
@@ -377,7 +377,7 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch) q->rx_class = cl;#endif if (cl ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_dsmark.c +1 -1
@@ -268,7 +268,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch) drop:kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }static struct sk_buff *dsmark_dequeue (struct Qdisc *sch)
net/sched/sch_hfsc.c +2 -2
@@ -1159,7 +1159,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)if (cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->root.filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -1578,7 +1578,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch) cl =hfsc_classify (skb, sch, &err);if (cl ==NULL ) { -if (err ==NET_XMIT_BYPASS ) +if (err & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return err;
net/sched/sch_htb.c +3 -3
@@ -214,7 +214,7 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,if ((cl =htb_find (skb->priority, sch)) !=NULL && cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -567,7 +567,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -612,7 +612,7 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_netem.c +1 -1
@@ -176,7 +176,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (count ==0 ) { sch->qstats.drops++;kfree_skb (skb); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }skb_orphan (skb);
net/sched/sch_prio.c +3 -3
@@ -38,7 +38,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)struct tcf_result res;int err; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;if (TC_H_MAJ (skb->priority) != sch->handle) { err =tc_classify (skb, q->filter_list, &res);#ifdef CONFIG_NET_CLS_ACT
@@ -74,7 +74,7 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch)#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -103,7 +103,7 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch) qdisc =prio_classify (skb, sch, &ret);#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_sfq.c +3 -3
@@ -171,7 +171,7 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,if (!q->filter_list)return sfq_hash (q, skb) +1 ; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; result =tc_classify (skb, q->filter_list, &res);if (result >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -285,7 +285,7 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -339,7 +339,7 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
David Miller
maintainerRe: [PATCH] net_sched: Add qdisc __NET_XMIT_BYPASS flag
From: Jarek Poplawski <[email protected]>
Date: Fri, 1 Aug 2008 10:19:29 +0000
include/net/sch_generic.h static inline unsigned int qdisc_pkt_len(struct sk_buff *skb) · line 339
Quoted 12 lines
include/net/sch_generic.h +2
-2@@ -331,14 +331,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } -#ifdef CONFIG_NET_CLS_ACT /* additional qdisc xmit flags */ enum net_xmit_qdisc_t { - __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , + __NET_XMIT_STOLEN =NET_XMIT_MASK +0x1 , + __NET_XMIT_BYPASS =NET_XMIT_MASK +0x2 , };
NET_XMIT_MASK + 0x2 is (0xffff + 2) which is 0x10001 and this is very much not what you intended.
Please just explicitly spell out the bit mask values instead of trying to create some false relationship with the mask and these values.
Next, these patches doesn't fix the while problem, in that htb and friends are still corrupting the enqueue return value so that TCP is going to do the wrong thing still.
PATCH 1/1 net_sched: Add qdisc __NET_XMIT_BYPASS flag
Quoted 7 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Fri, 1 Aug 2008 10:19:29 +0000Quoted 4 lines
(quoted diff) +0
-0@@ -331,14 +331,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb) return qdisc_skb_cb(skb)->pkt_len; }
Quoted 11 lines
Review static inline unsigned int qdisc_pkt_len(struct sk_buff *skb) · line 339
Quoted 8 lines
(quoted diff) +2
-2-#ifdef CONFIG_NET_CLS_ACT /* additional qdisc xmit flags */ enum net_xmit_qdisc_t { - __NET_XMIT_STOLEN = NET_XMIT_MASK + 1, + __NET_XMIT_STOLEN = NET_XMIT_MASK + 0x1, + __NET_XMIT_BYPASS = NET_XMIT_MASK + 0x2, };
NET_XMIT_MASK+0x2is (0xffff+ 2) which is0x10001and this is very much not what you intended.
Thanks for spotting this!
Quoted 7 lines
Please just explicitly spell out the bit mask values instead of trying to create some false relationship with the mask and these values.
Next, these patches doesn't fix the while problem, in that htb and friends are still corrupting the enqueue return value so that TCP is going to do the wrong thing still.
These 2 patches aren't supposed to fix these problems:
- the __NET_XMIT_STOLEN patch fixes a problem when a qdiscs passed upwards NET_XMIT_SUCCESS when eg. an skb was stolen by an action, to prevent treating this as dropped; then the upper qdiscs couldn't tell it's not the full NET_XMIT_SUCCESS and counted it as queued, which was wrong. (BTW, I especially tried to do it with minimal changes in htb_enqueue() to let you apply your htb patch without much editing.)
- the __NET_XMIT_BYPASS actually doesn't fix any serious problem, but lets to remove the mapping from dev_queue_xmit(), and BTW it passes better information to upper qdiscs too.
Thanks,
Jarek P.
------------------> (take 2)
Subject: net_sched: Add qdisc __NET_XMIT_BYPASS flag
Patrick McHardy <[email protected]> noticed that it would be nice to handle NET_XMIT_BYPASS by NET_XMIT_SUCCESS with an internal qdisc flag __NET_XMIT_BYPASS and to remove the mapping from dev_queue_xmit().
David Miller <[email protected]> spotted a serious bug in the first version of this patch.
---
(apply on top of [PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag)
include/net/sch_generic.h | 8 ++++---- net/core/dev.c | 1 - net/sched/sch_atm.c | 2 +- net/sched/sch_cbq.c | 4 ++-- net/sched/sch_dsmark.c | 2 +- net/sched/sch_hfsc.c | 4 ++-- net/sched/sch_htb.c | 6 +++--- net/sched/sch_netem.c | 2 +- net/sched/sch_prio.c | 6 +++--- net/sched/sch_sfq.c | 6 +++--- 10 files changed, 20 insertions(+), 21 deletions(-)
include/net/sch_generic.h +4 -4
@@ -331,14 +331,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } -#ifdef CONFIG_NET_CLS_ACT -/* additional qdisc xmit flags */ +/* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */ enum net_xmit_qdisc_t { - __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , + __NET_XMIT_STOLEN =0x0001FFFF , + __NET_XMIT_BYPASS =0x0002FFFF , }; +#ifdef CONFIG_NET_CLS_ACT #define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) -#else #define net_xmit_drop_count (e) (1)#endif
net/core/dev.c +0 -1
@@ -1805,7 +1805,6 @@ gso:spin_unlock (root_lock); - rc = rc ==NET_XMIT_BYPASS ?NET_XMIT_SUCCESS : rc; goto out; }
net/sched/sch_atm.c +1 -1
@@ -457,7 +457,7 @@ drop: __maybe_unusedreturn 0 ; }tasklet_schedule (&p->task); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }/*
net/sched/sch_cbq.c +2 -2
@@ -230,7 +230,7 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) (cl = cbq_class_lookup(q, prio)) != NULL)return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;for (;;) {int result =0 ; defmap = head->defaults;
@@ -377,7 +377,7 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch) q->rx_class = cl;#endif if (cl ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_dsmark.c +1 -1
@@ -268,7 +268,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch) drop:kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }static struct sk_buff *dsmark_dequeue (struct Qdisc *sch)
net/sched/sch_hfsc.c +2 -2
@@ -1159,7 +1159,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)if (cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->root.filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -1578,7 +1578,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch) cl =hfsc_classify (skb, sch, &err);if (cl ==NULL ) { -if (err ==NET_XMIT_BYPASS ) +if (err & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return err;
net/sched/sch_htb.c +3 -3
@@ -214,7 +214,7 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,if ((cl =htb_find (skb->priority, sch)) !=NULL && cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -567,7 +567,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -612,7 +612,7 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_netem.c +1 -1
@@ -176,7 +176,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (count ==0 ) { sch->qstats.drops++;kfree_skb (skb); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }skb_orphan (skb);
net/sched/sch_prio.c +3 -3
@@ -38,7 +38,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)struct tcf_result res;int err; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;if (TC_H_MAJ (skb->priority) != sch->handle) { err =tc_classify (skb, q->filter_list, &res);#ifdef CONFIG_NET_CLS_ACT
@@ -74,7 +74,7 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch)#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -103,7 +103,7 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch) qdisc =prio_classify (skb, sch, &ret);#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_sfq.c +3 -3
@@ -171,7 +171,7 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,if (!q->filter_list)return sfq_hash (q, skb) +1 ; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; result =tc_classify (skb, q->filter_list, &res);if (result >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -285,7 +285,7 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -339,7 +339,7 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
David Miller
maintainerRe: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
From: Jarek Poplawski <[email protected]>
Date: Mon, 4 Aug 2008 06:28:13 +0000
Quoted 10 lines
These 2 patches aren't supposed to fix these problems:
- the
__NET_XMIT_STOLENpatch fixes a problem when a qdiscs passed upwardsNET_XMIT_SUCCESSwhen eg. an skb was stolen by an action, to prevent treating this as dropped; then the upper qdiscs couldn't tell it's not the fullNET_XMIT_SUCCESSand counted it as queued, which was wrong. (BTW, I especially tried to do it with minimal changes inhtb_enqueue()to let you apply your htb patch without much editing.)
Quoted 3 lines
- the
__NET_XMIT_BYPASSactually doesn't fix any serious problem, but lets to remove the mapping fromdev_queue_xmit(), and BTW it passes better information to upper qdiscs too.
I understand.
But be aware that all of these things are related :-)
Quoted 11 lines
Subject:
net_sched: Add qdisc__NET_XMIT_BYPASSflagPatrick McHardy <[email protected]> noticed that it would be nice to handle
NET_XMIT_BYPASSbyNET_XMIT_SUCCESSwith an internal qdisc flag__NET_XMIT_BYPASSand to remove the mapping fromdev_queue_xmit().David Miller <[email protected]> spotted a serious bug in the first version of this patch.
Signed-off-by Jarek Poplawski <[email protected]>
...
include/net/sch_generic.h static inline unsigned int qdisc_pkt_len(struct sk_buff *skb) · line 339
Quoted 13 lines
include/net/sch_generic.h +3
-3@@ -331,14 +331,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } -#ifdef CONFIG_NET_CLS_ACT -/* additional qdisc xmit flags */ +/* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */ enum net_xmit_qdisc_t { - __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , + __NET_XMIT_STOLEN =0x0001FFFF , + __NET_XMIT_BYPASS =0x0002FFFF , };
Please, respin both patches, so that the "NET_XMIT_MASK + 1" construct doesn't ever exist in the tree.
Thanks again.
Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
On Sun, Aug 03, 2008 at 11:42:00PM -0700, David Miller wrote:
...
Quoted 4 lines
Quoted 4 lines
include/net/sch_generic.h +1
-2-#ifdef CONFIG_NET_CLS_ACT -/* additional qdisc xmit flags */ +/* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */ enum net_xmit_qdisc_t {
Quoted 8 lines
include/net/sch_generic.h static inline unsigned int qdisc_pkt_len(struct sk_buff *skb) · line 338
Quoted 5 lines
include/net/sch_generic.h +2
-1- __NET_XMIT_STOLEN = NET_XMIT_MASK + 1, + __NET_XMIT_STOLEN =0x0001FFFF , + __NET_XMIT_BYPASS = 0x0002FFFF, };Please, respin both patches, so that the "
NET_XMIT_MASK+ 1" construct doesn't ever exist in the tree.
OK, I'll soon respin this all!
Jarek P.
PATCH 1/2 net_sched: Add qdisc __NET_XMIT_STOLEN flag
In reply to David MillerOn Sun, Aug 03, 2008 at 11:42:00PM -0700, David Miller wrote:
...
Quoted 9 lines
Review
Quoted 6 lines
enum
net_xmit_qdisc_t{(quoted diff) +2
-1- __NET_XMIT_STOLEN = NET_XMIT_MASK + 1, + __NET_XMIT_STOLEN = 0x0001FFFF, + __NET_XMIT_BYPASS = 0x0002FFFF, };Please, respin both patches, so that the "
NET_XMIT_MASK+ 1" construct doesn't ever exist in the tree.
My git went dangling, so it took longer than I expected, sorry.
Jarek P.
----------------->
Subject: net_sched: Add qdisc __NET_XMIT_STOLEN flag
Patrick McHardy <[email protected]> noticed:
"The other problem that affects all qdiscs supporting actions is TC_ACT_QUEUED/TC_ACT_STOLEN getting mapped to NET_XMIT_SUCCESS even though the packet is not queued, corrupting upper qdiscs' qlen counters."
and later explained:
"The reason why it translates it at all seems to be to not increase the drops counter. Within a single qdisc this could be avoided by other means easily, upper qdiscs would still increase the counter when we return anything besides NET_XMIT_SUCCESS though.
This means we need a new NET_XMIT return value to indicate this to the upper qdiscs. So I'd suggest to introduce NET_XMIT_STOLEN, return that to upper qdiscs and translate it to NET_XMIT_SUCCESS in dev_queue_xmit, similar to NET_XMIT_BYPASS."
David Miller <[email protected]> noticed:
"Maybe these NET_XMIT_* values being passed around should be a set of bits. They could be composed of base meanings, combined with specific attributes.
So you could say "NET_XMIT_DROP | __NET_XMIT_NO_DROP_COUNT"
The attributes get masked out by the top-level ->enqueue() caller, such that the base meanings are the only thing that make their way up into the stack. If it's only about communication within the qdisc tree, let's simply code it that way."
This patch is trying to realize these ideas.
---
include/linux/netdevice.h | 1 + include/net/sch_generic.h | 14 +++++++++++++- net/sched/sch_atm.c | 12 +++++++----- net/sched/sch_cbq.c | 23 +++++++++++++++-------- net/sched/sch_dsmark.c | 8 +++++--- net/sched/sch_hfsc.c | 8 +++++--- net/sched/sch_htb.c | 18 +++++++++++------- net/sched/sch_netem.c | 3 ++- net/sched/sch_prio.c | 8 +++++--- net/sched/sch_red.c | 2 +- net/sched/sch_sfq.c | 2 +- net/sched/sch_tbf.c | 3 ++- 12 files changed, 68 insertions(+), 34 deletions(-)
include/linux/netdevice.h +1 -0
@@ -64,6 +64,7 @@ struct wireless_dev;#define NET_XMIT_BYPASS 4/* packet does not leave via dequeue; (TC use only - dev_queue_xmit returns this as NET_XMIT_SUCCESS) */ +#define NET_XMIT_MASK 0xFFFF/* qdisc flags in net/sch_generic.h */ /* Backlog congestion levels */ #define NET_RX_SUCCESS 0/* keep 'em coming, baby */
include/net/sch_generic.h +13 -1
@@ -343,6 +343,18 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } +#ifdef CONFIG_NET_CLS_ACT +/* additional qdisc xmit flags */ +enum net_xmit_qdisc_t { + __NET_XMIT_STOLEN =0x00010000 , +}; + +#define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) + +#else +#define net_xmit_drop_count (e) (1) +#endif +static inline int qdisc_enqueue (struct sk_buff *skb,struct Qdisc *sch) {#ifdef CONFIG_NET_SCHED
@@ -355,7 +367,7 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) {qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch); +return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ; } static inline int __qdisc_enqueue_tail(struct sk_buff *skb, struct Qdisc *sch,
net/sched/sch_atm.c +7 -5
@@ -415,7 +415,7 @@ static int atm_tc_enqueue(struct sk_buff *skb, struct Qdisc *sch)case TC_ACT_QUEUED :case TC_ACT_STOLEN :kfree_skb (skb); -return NET_XMIT_SUCCESS ; +return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :kfree_skb (skb); goto drop;
@@ -432,9 +432,11 @@ static int atm_tc_enqueue(struct sk_buff *skb, struct Qdisc *sch) ret =qdisc_enqueue (skb, flow->q);if (ret !=0 ) { drop:__maybe_unused - sch->qstats.drops++; -if (flow) - flow->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; +if (flow) + flow->qstats.drops++; + }return ret; } sch->bstats.bytes +=qdisc_pkt_len (skb);
@@ -530,7 +532,7 @@ static int atm_tc_requeue(struct sk_buff *skb, struct Qdisc *sch)if (!ret) { sch->q.qlen++; sch->qstats.requeues++; - }else { + }else if (net_xmit_drop_count (ret)) { sch->qstats.drops++; p->link.qstats.drops++; }
net/sched/sch_cbq.c +15 -8
@@ -256,7 +256,7 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ;case TC_ACT_RECLASSIFY :
@@ -397,9 +397,11 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch)return ret; } - sch->qstats.drops++; -cbq_mark_toplevel (q, cl); - cl->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; +cbq_mark_toplevel (q, cl); + cl->qstats.drops++; + }return ret; }
@@ -430,8 +432,10 @@ cbq_requeue(struct sk_buff *skb, struct Qdisc *sch)cbq_activate_class (cl);return 0 ; } - sch->qstats.drops++; - cl->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; + cl->qstats.drops++; + }return ret; }
@@ -664,13 +668,15 @@ static int cbq_reshape_fail(struct sk_buff *skb, struct Qdisc *child) q->rx_class =NULL ;if (cl && (cl =cbq_reclassify (skb, cl)) !=NULL ) { +int ret;cbq_mark_toplevel (q, cl); q->rx_class = cl; cl->q->__parent = sch; -if (qdisc_enqueue (skb, cl->q) ==0 ) { + ret =qdisc_enqueue (skb, cl->q); +if (ret ==NET_XMIT_SUCCESS ) { sch->q.qlen++; sch->bstats.packets++; sch->bstats.bytes +=qdisc_pkt_len (skb);
@@ -678,7 +684,8 @@ static int cbq_reshape_fail(struct sk_buff *skb, struct Qdisc *child)cbq_activate_class (cl);return 0 ; } - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return 0 ; }
net/sched/sch_dsmark.c +5 -3
@@ -236,7 +236,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch)case TC_ACT_QUEUED :case TC_ACT_STOLEN :kfree_skb (skb); -return NET_XMIT_SUCCESS ; +return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT : goto drop;
@@ -254,7 +254,8 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch) err =qdisc_enqueue (skb, p->q);if (err !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; +if (net_xmit_drop_count (err)) + sch->qstats.drops++;return err; }
@@ -321,7 +322,8 @@ static int dsmark_requeue(struct sk_buff *skb, struct Qdisc *sch) err = p->q->ops->requeue (skb, p->q);if (err !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; +if (net_xmit_drop_count (err)) + sch->qstats.drops++;return err; }
net/sched/sch_hfsc.c +5 -3
@@ -1166,7 +1166,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ; }
@@ -1586,8 +1586,10 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch) err =qdisc_enqueue (skb, cl->qdisc);if (unlikely (err !=NET_XMIT_SUCCESS )) { - cl->qstats.drops++; - sch->qstats.drops++; +if (net_xmit_drop_count (err)) { + cl->qstats.drops++; + sch->qstats.drops++; + }return err; }
net/sched/sch_htb.c +11 -7
@@ -221,7 +221,7 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ; }
@@ -572,9 +572,11 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch)kfree_skb (skb);return ret;#endif - }else if (qdisc_enqueue (skb, cl->un.leaf.q) !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; - cl->qstats.drops++; + }else if ((ret =qdisc_enqueue (skb, cl->un.leaf.q)) !=NET_XMIT_SUCCESS ) { +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; + cl->qstats.drops++; + }return NET_XMIT_DROP ; }else { cl->bstats.packets +=
@@ -615,10 +617,12 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch)kfree_skb (skb);return ret;#endif - }else if (cl->un.leaf.q->ops->requeue (skb, cl->un.leaf.q) != + }else if ((ret = cl->un.leaf.q->ops->requeue (skb, cl->un.leaf.q)) !=NET_XMIT_SUCCESS ) { - sch->qstats.drops++; - cl->qstats.drops++; +if (net_xmit_drop_count (ret)) { + sch->qstats.drops++; + cl->qstats.drops++; + }return NET_XMIT_DROP ; }else htb_activate (q, cl);
net/sched/sch_netem.c +2 -1
@@ -240,8 +240,9 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch) sch->q.qlen++; sch->bstats.bytes +=qdisc_pkt_len (skb); sch->bstats.packets++; - }else + }else if (net_xmit_drop_count (ret)) { sch->qstats.drops++; + }pr_debug ("netem: enqueue ret %d\n" , ret);return ret;
net/sched/sch_prio.c +5 -3
@@ -45,7 +45,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)switch (err) {case TC_ACT_STOLEN :case TC_ACT_QUEUED : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return NULL ; }
@@ -88,7 +88,8 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch) sch->q.qlen++;return NET_XMIT_SUCCESS ; } - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return ret; }
@@ -114,7 +115,8 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch) sch->qstats.requeues++;return 0 ; } - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return NET_XMIT_DROP ; }
net/sched/sch_red.c +1 -1
@@ -97,7 +97,7 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc* sch) sch->bstats.bytes +=qdisc_pkt_len (skb); sch->bstats.packets++; sch->q.qlen++; - }else { + }else if (net_xmit_drop_count (ret)) { q->stats.pdrop++; sch->qstats.drops++; }
net/sched/sch_sfq.c +1 -1
@@ -178,7 +178,7 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,switch (result) {case TC_ACT_STOLEN :case TC_ACT_QUEUED : - *qerr =NET_XMIT_SUCCESS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;case TC_ACT_SHOT :return 0 ; }
net/sched/sch_tbf.c +2 -1
@@ -135,7 +135,8 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc* sch) ret =qdisc_enqueue (skb, q->qdisc);if (ret !=0 ) { - sch->qstats.drops++; +if (net_xmit_drop_count (ret)) + sch->qstats.drops++;return ret; }
From: Jarek Poplawski <[email protected]>
Date: Mon, 4 Aug 2008 08:55:09 +0000
Quoted 1 line
net_sched: Add qdisc__NET_XMIT_STOLENflag
Applied, thanks Jarek.
PATCH 2/2 net_sched: Add qdisc __NET_XMIT_BYPASS flag
In reply to David MillerSubject: net_sched: Add qdisc __NET_XMIT_BYPASS flag
Patrick McHardy <[email protected]> noticed that it would be nice to handle NET_XMIT_BYPASS by NET_XMIT_SUCCESS with an internal qdisc flag __NET_XMIT_BYPASS and to remove the mapping from dev_queue_xmit().
David Miller <[email protected]> spotted a serious bug in the first version of this patch.
---
include/net/sch_generic.h | 6 +++--- net/core/dev.c | 1 - net/sched/sch_atm.c | 2 +- net/sched/sch_cbq.c | 4 ++-- net/sched/sch_dsmark.c | 2 +- net/sched/sch_hfsc.c | 4 ++-- net/sched/sch_htb.c | 6 +++--- net/sched/sch_netem.c | 2 +- net/sched/sch_prio.c | 6 +++--- net/sched/sch_sfq.c | 6 +++--- 10 files changed, 19 insertions(+), 20 deletions(-)
include/net/sch_generic.h +3 -3
@@ -343,14 +343,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } -#ifdef CONFIG_NET_CLS_ACT -/* additional qdisc xmit flags */ +/* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */ enum net_xmit_qdisc_t { __NET_XMIT_STOLEN =0x00010000 , + __NET_XMIT_BYPASS =0x00020000 , }; +#ifdef CONFIG_NET_CLS_ACT #define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) -#else #define net_xmit_drop_count (e) (1)#endif
net/core/dev.c +0 -1
@@ -1805,7 +1805,6 @@ gso:spin_unlock (root_lock); - rc = rc ==NET_XMIT_BYPASS ?NET_XMIT_SUCCESS : rc; goto out; }
net/sched/sch_atm.c +1 -1
@@ -457,7 +457,7 @@ drop: __maybe_unusedreturn 0 ; }tasklet_schedule (&p->task); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }/*
net/sched/sch_cbq.c +2 -2
@@ -230,7 +230,7 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) (cl = cbq_class_lookup(q, prio)) != NULL)return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;for (;;) {int result =0 ; defmap = head->defaults;
@@ -377,7 +377,7 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch) q->rx_class = cl;#endif if (cl ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_dsmark.c +1 -1
@@ -268,7 +268,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch) drop:kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }static struct sk_buff *dsmark_dequeue (struct Qdisc *sch)
net/sched/sch_hfsc.c +2 -2
@@ -1159,7 +1159,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)if (cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->root.filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -1578,7 +1578,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch) cl =hfsc_classify (skb, sch, &err);if (cl ==NULL ) { -if (err ==NET_XMIT_BYPASS ) +if (err & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return err;
net/sched/sch_htb.c +3 -3
@@ -214,7 +214,7 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,if ((cl =htb_find (skb->priority, sch)) !=NULL && cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -567,7 +567,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -612,7 +612,7 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_netem.c +1 -1
@@ -176,7 +176,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (count ==0 ) { sch->qstats.drops++;kfree_skb (skb); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }skb_orphan (skb);
net/sched/sch_prio.c +3 -3
@@ -38,7 +38,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)struct tcf_result res;int err; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;if (TC_H_MAJ (skb->priority) != sch->handle) { err =tc_classify (skb, q->filter_list, &res);#ifdef CONFIG_NET_CLS_ACT
@@ -74,7 +74,7 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch)#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -103,7 +103,7 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch) qdisc =prio_classify (skb, sch, &ret);#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_sfq.c +3 -3
@@ -171,7 +171,7 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,if (!q->filter_list)return sfq_hash (q, skb) +1 ; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; result =tc_classify (skb, q->filter_list, &res);if (result >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -285,7 +285,7 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -339,7 +339,7 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
Re: [PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag
From: Jarek Poplawski <[email protected]>
Date: Mon, 4 Aug 2008 08:57:34 +0000
Quoted 10 lines
net_sched: Add qdisc__NET_XMIT_BYPASSflagPatrick McHardy <[email protected]> noticed that it would be nice to handle
NET_XMIT_BYPASSbyNET_XMIT_SUCCESSwith an internal qdisc flag__NET_XMIT_BYPASSand to remove the mapping fromdev_queue_xmit().David Miller <[email protected]> spotted a serious bug in the first version of this patch.
Signed-off-by Jarek Poplawski <[email protected]>
Applied, thanks Jarek.
After applying this I noticed that dst_input()'s usage of plain NET_XMIT_BYPASS was bogus, and after killing that there are no more references.
Thus I added the following commit.
commit cc6533e98a7f3cb7fce9d740da49195c7aa523a4
Author: David S. Miller <[email protected]> Date: Mon Aug 4 23:04:08 2008 -0700
net: Kill plain NET_XMIT_BYPASS.
dst_input() was doing something completely absurd, looping on skb->dst->input() if NET_XMIT_BYPASS was seen, but these functions never return such an error.
And as a result plain ole' NET_XMIT_BYPASS has no more references and can be completely killed off.
include/linux/netdevice.h +0 -3
@@ -61,9 +61,6 @@ struct wireless_dev;#define NET_XMIT_DROP 1/* skb dropped */ #define NET_XMIT_CN 2/* congestion notification */ #define NET_XMIT_POLICED 3/* skb is shot by police */ -#define NET_XMIT_BYPASS 4/* packet does not leave via dequeue; -(TC use only - dev_queue_xmit -returns this as NET_XMIT_SUCCESS) */ #define NET_XMIT_MASK 0xFFFF/* qdisc flags in net/sch_generic.h */ /* Backlog congestion levels */
include/net/dst.h +1 -11
@@ -252,17 +252,7 @@ static inline int dst_output(struct sk_buff *skb)/* Input packet from network to transport. */ static inline int dst_input (struct sk_buff *skb) { -int err; - -for (;;) { - err = skb->dst->input (skb); - -if (likely (err ==0 )) -return err; -/* Oh, Jamal... Seems, I will not forgive you this mess. :-) */ -if (unlikely (err !=NET_XMIT_BYPASS )) -return err; - } +return skb->dst->input (skb); }static inline struct dst_entry *dst_check (struct dst_entry *dst,u32 cookie)
Re: [PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag
On Mon, Aug 04, 2008 at 11:14:49PM -0700, David Miller wrote:
...
Quoted 46 lines
After applying this I noticed that dst_input()'s usage of plain
NET_XMIT_BYPASSwas bogus, and after killing that there are no more references.Thus I added the following commit.
commit
cc6533e98a7f3cb7fce9d740da49195c7aa523a4Author: David S. Miller <[email protected]> Date: Mon Aug 4 23:04:08 2008 -0700net: Kill plain
NET_XMIT_BYPASS.
dst_input()was doing something completely absurd, looping onskb->dst->input()ifNET_XMIT_BYPASSwas seen, but these functions never return such an error.And as a result plain ole'
NET_XMIT_BYPASShas no more references and can be completely killed off.Signed-off-by David S. Miller <[email protected]>include/linux/netdevice.h +0
-3@@ -61,9 +61,6 @@ struct wireless_dev;#define NET_XMIT_DROP 1/* skb dropped */ #define NET_XMIT_CN 2/* congestion notification */ #define NET_XMIT_POLICED 3/* skb is shot by police */ -#define NET_XMIT_BYPASS 4/* packet does not leave via dequeue; -(TC use only - dev_queue_xmit -returns this as NET_XMIT_SUCCESS) */ #define NET_XMIT_MASK 0xFFFF/* qdisc flags in net/sch_generic.h */ /* Backlog congestion levels */ include/net/dst.h +0
-3@@ -252,17 +252,7 @@ static inline int dst_output(struct sk_buff *skb)/* Input packet from network to transport. */ static inline int dst_input (struct sk_buff *skb) { -int err; - -for (;;) {
include/net/dst.h static inline int dst_output(struct sk_buff *skb) · line 256
Quoted 10 lines
include/net/dst.h +1
-8- err = skb->dst->input (skb); - -if (likely (err ==0 )) -return err; -/* Oh, Jamal... Seems, I will not forgive you this mess. :-) */ -if (unlikely (err !=NET_XMIT_BYPASS )) -return err; - } +return skb->dst->input (skb); }
NAK! You're not allowed to remove such a great comment!
BTW, I hope you've checked this enough, becouse I've some doubts
here: this NET_XMIT_BYPASS looks wrong here but maybe it was meant to be some NET_RX_ code, like NET_RX_CN_HIGH?
Jarek P.
David Miller
maintainerRe: [PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag
From: Jarek Poplawski <[email protected]>
Date: Tue, 5 Aug 2008 06:34:23 +0000
Quoted 5 lines
NAK! You're not allowed to remove such a great comment!
BTW, I hope you've checked this enough, becouse I've some doubts
here: thisNET_XMIT_BYPASSlooks wrong here but maybe it was meant to be someNET_RX_code, likeNET_RX_CN_HIGH?
I think it was trying to handle the cases where the input packet called dev_queue_xmit() and that return value funnelled back into dst_input().
But that couldn't happen, and even if it could dev_queue_xmit() always "fixed up" NET_XMIT_BYPASS so that callers never saw it.
Really, this code was totally useless as far as I can tell.
Re: [PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag
Quoted 9 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Tue, 5 Aug 2008 06:34:23 +0000Quoted 5 lines
NAK! You're not allowed to remove such a great comment!
BTW, I hope you've checked this enough, becouse I've some doubts
here: thisNET_XMIT_BYPASSlooks wrong here but maybe it was meant to be someNET_RX_code, likeNET_RX_CN_HIGH?
Quoted 8 lines
I think it was trying to handle the cases where the input packet called
dev_queue_xmit()and that return value funnelled back intodst_input().But that couldn't happen, and even if it could
dev_queue_xmit()always "fixed up"NET_XMIT_BYPASSso that callers never saw it.Really, this code was totally useless as far as I can tell.
This looks reasonable, the only doubt is if it could get anywhere this NET_RX_CN_HIGH (against original intention), because then there would be some change. Anyway, the comment could stay...
Jarek P.
Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
In reply to Jarek PoplawskiQuoting "Jarek Poplawski" <[email protected]>:
Quoted 11 lines
These 2 patches aren't supposed to fix these problems:
- the
__NET_XMIT_STOLENpatch fixes a problem when a qdiscs passed upwardsNET_XMIT_SUCCESSwhen eg. an skb was stolen by an action, to prevent treating this as dropped; then the upper qdiscs couldn't tell it's not the fullNET_XMIT_SUCCESSand counted it as queued, which was wrong. (BTW, I especially tried to do it with minimal changes inhtb_enqueue()to let you apply your htb patch without much editing.)
Quoted 4 lines
- the
__NET_XMIT_BYPASSactually doesn't fix any serious problem, but lets to remove the mapping fromdev_queue_xmit(), and BTW it passes better information to upper qdiscs too.
This seems to also make sure that when returning 'full' NET_XMIT_SUCCESS, skb pointer is safe(r?) to use now.
Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
Quoted 13 lines · Jussi Kivilinna
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 11 lines
These 2 patches aren't supposed to fix these problems:
- the
__NET_XMIT_STOLENpatch fixes a problem when a qdiscs passed upwardsNET_XMIT_SUCCESSwhen eg. an skb was stolen by an action, to prevent treating this as dropped; then the upper qdiscs couldn't tell it's not the fullNET_XMIT_SUCCESSand counted it as queued, which was wrong. (BTW, I especially tried to do it with minimal changes inhtb_enqueue()to let you apply your htb patch without much editing.)
Quoted 7 lines
Quoted 4 lines
- the
__NET_XMIT_BYPASSactually doesn't fix any serious problem, but lets to remove the mapping fromdev_queue_xmit(), and BTW it passes better information to upper qdiscs too.This seems to also make sure that when returning 'full'
NET_XMIT_SUCCESS, skb pointer is safe(r?) to use now.
I guess so... At least theoretically - I was mainly afraid of redirecting, which isn't fully implemented yet. On the other hand, outwards of qdiscs there is still no difference.
Jarek P.
Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 10 lines · Jussi Kivilinna
Quoted 4 lines
Quoting "Jarek Poplawski" <[email protected]>:
This seems to also make sure that when returning 'full'
NET_XMIT_SUCCESS, skb pointer is safe(r?) to use now.I guess so... At least theoretically - I was mainly afraid of redirecting, which isn't fully implemented yet. On the other hand, outwards of qdiscs there is still no difference.
How about making skb shared before passing into qdisc tree? That would make skb usage safe after qdisc enqueues.
---
include/net/sch_generic.h +0 -0
@@ -352,10 +352,16 @@ static inline int qdisc_enqueue(struct sk_buff
*skb, struct Qdisc *sch)
return sch->enqueue(skb, sch);
}
-static inline int qdisc_enqueue_root(struct sk_buff *skb, struct Qdisc *sch) +static inline int qdisc_enqueue_root(struct sk_buff *__skb, struct Qdisc *sch)
{
+ struct sk_buff *skb = skb_get(__skb);
+ int ret;
+
qdisc_skb_cb(skb)->pkt_len = skb->len; - return qdisc_enqueue(skb, sch); + ret = qdisc_enqueue(skb, sch); + kfree_skb(skb);
+
+ return ret;
}
static inline int __qdisc_enqueue_tail(struct sk_buff *skb, struct
Qdisc *sch,
Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
Quoted 8 lines · Jussi Kivilinna
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 6 lines · Jussi Kivilinna
Quoted 4 lines
Quoting "Jarek Poplawski" <[email protected]>:
This seems to also make sure that when returning 'full'
NET_XMIT_SUCCESS, skb pointer is safe(r?) to use now.
Quoted 7 lines
Quoted 4 lines
I guess so... At least theoretically - I was mainly afraid of redirecting, which isn't fully implemented yet. On the other hand, outwards of qdiscs there is still no difference.
How about making skb shared before passing into qdisc tree? That would make skb usage safe after qdisc enqueues.
It's a bit costly (atomics), so there should be a good reason for this. It should be first checked if there is real danger. And if it's only for more exact stats, I'm not sure it's worth of it.
Jarek P.
Quoted 30 lines
---
include/net/sch_generic.h +8
-2@@ -352,10 +352,16 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)return sch->enqueue (skb, sch); } -static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) +static inline int qdisc_enqueue_root (struct sk_buff *__skb,struct Qdisc *sch) { +struct sk_buff *skb =skb_get (__skb); +int ret; +qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch); + ret =qdisc_enqueue (skb, sch); +kfree_skb (skb); + +return ret; }static inline int __qdisc_enqueue_tail(struct
sk_buff*skb, struct
Qdisc *sch,
qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb (Was: Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag)
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 8 lines
Quoted 3 lines
How about making skb shared before passing into qdisc tree? That would make skb usage safe after qdisc enqueues.
It's a bit costly (atomics), so there should be a good reason for this. It should be first checked if there is real danger. And if it's only for more exact stats, I'm not sure it's worth of it.
Ok, I went throught all enqueue (and requeue) functions for any case of freeing skb and returning full NET_XMIT_SUCCESS without new flags and found only in sch_blackhole (qdisc_drop + return NET_XMIT_SUCCESS). This could be fixed by delaying kfree_skb to exit on qdisc_enqueue_root, here's (completely untested) patch:
---
include/net/sch_generic.h +1 -0
@@ -175,6 +175,7 @@ struct tcf_protostruct qdisc_skb_cb {unsigned int pkt_len; +__u8 delayed_enqueue_free:1 ;char data[]; };
@@ -364,10 +365,23 @@ static inline int qdisc_enqueue(struct sk_buff
*skb, struct Qdisc *sch)
return sch->enqueue(skb, sch);
}
+static inline void qdisc_delayed_kfree_skb(struct sk_buff *skb) +{
+ qdisc_skb_cb(skb)->delayed_enqueue_free = 1;
+}
+
static inline int qdisc_enqueue_root(struct sk_buff *skb, struct Qdisc *sch) {
+ int ret;
+
+ qdisc_skb_cb(skb)->delayed_enqueue_free = 0;
qdisc_skb_cb(skb)->pkt_len = skb->len;- return qdisc_enqueue(skb, sch) & NET_XMIT_MASK;
+ ret = qdisc_enqueue(skb, sch);
+
+ if (ret == NET_XMIT_SUCCESS &&
qdisc_skb_cb(skb)->delayed_enqueue_free)
+ kfree_skb(skb);
+
+ return ret & NET_XMIT_MASK;
}
static inline int __qdisc_enqueue_tail(struct sk_buff *skb, struct
Qdisc *sch,
net/sched/sch_blackhole.c +2 -2
@@ -19,7 +19,8 @@static int blackhole_enqueue (struct sk_buff *skb,struct Qdisc *sch) { -qdisc_drop (skb, sch); +qdisc_delayed_kfree_skb (skb); + sch->qstats.drops++;return NET_XMIT_SUCCESS ; } ---
If this isn't good way to solve this, qdisc_pkt_len use for stats could be fixed with either passing packet length pointer throught qdisc tree or adding new qdisc_pkt_len_diff and adding difference in at dequeue as you said (but here
inner dequeue could return NULL and difference wouldn't be added after all but well it is just stats).
As I went throught code I found two cases where skb pointer is used after inner
enqueue with full NET_XMIT_SUCCESS (other than qdisc_pkt_len for stats): HTB uses skb_is_gso(), HFSC uses packet length for set_active(). HTB is trivial (for me) to fix while HFSC isn't. Because HFSC part it would be easier for me to declare full NET_XMIT_SUCCESS as safe zone for skb pointer.
- Jussi
PS. I noticed something fishy in HTB; HTB always returns NET_XMIT_DROP if qdisc_enqueue doesn't return full NET_XMIT_SUCCESS, shouldn't it return return value from qdisc_enqueue. Same in HTB requeue. That can't be right, right?
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb (Was: Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag)
On Wed, Aug 06, 2008 at 10:42:48PM +0300, Jussi Kivilinna wrote:
...
Quoted 3 lines
Ok, I went throught all enqueue (and requeue) functions for any case of freeing skb and returning full
NET_XMIT_SUCCESSwithout new flags and found only insch_blackhole(qdisc_drop+ returnNET_XMIT_SUCCESS).
Very interesting observation. Probably mostly theoretical (I wonder how many people use this). There is a question if this code can be returned in such a case? noop returns NET_XMIT_CN, which looks safer, but maybe this is an exception? I don't know. Anyway, if it happens e.g. with forwarded skb it looks like reading after kfree.
Quoted 58 lines
This could be fixed by delaying
kfree_skbto exit onqdisc_enqueue_root, here's (completely untested) patch:
---include/net/sch_generic.h +15
-1@@ -175,6 +175,7 @@ struct tcf_protostruct qdisc_skb_cb {unsigned int pkt_len; +__u8 delayed_enqueue_free:1 ;char data[]; };@@ -364,10 +365,23 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)return sch->enqueue (skb, sch); } +static inline void qdisc_delayed_kfree_skb (struct sk_buff *skb) +{ +qdisc_skb_cb (skb)->delayed_enqueue_free =1 ; +} +static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) { +int ret; + +qdisc_skb_cb (skb)->delayed_enqueue_free =0 ;qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ; + ret =qdisc_enqueue (skb, sch); + +if (ret ==NET_XMIT_SUCCESS &&qdisc_skb_cb (skb)->delayed_enqueue_free) +kfree_skb (skb); + +return ret &NET_XMIT_MASK ; }static inline int __qdisc_enqueue_tail(struct
sk_buff*skb, struct
Qdisc *sch,net/sched/sch_blackhole.c +2
-2@@ -19,7 +19,8 @@static int blackhole_enqueue (struct sk_buff *skb,struct Qdisc *sch) { -qdisc_drop (skb, sch); +qdisc_delayed_kfree_skb (skb); + sch->qstats.drops++;return NET_XMIT_SUCCESS ; } ---
Quoted 6 lines
If this isn't good way to solve this,
qdisc_pkt_lenuse for stats could be fixed with either passing packet length pointer throught qdisc tree or adding newqdisc_pkt_len_diffand adding difference in at dequeue as you said (but here
inner dequeue could return NULL and difference wouldn't be added after all but well it is just stats).
I doubt that such a rare case should change the way all packets are treated, but if so, there probably could be used one of these new __NET_XMIT flags for this.
Quoted 6 lines
As I went throught code I found two cases where skb pointer is used after inner
enqueue with fullNET_XMIT_SUCCESS(other thanqdisc_pkt_lenfor stats): HTB usesskb_is_gso(), HFSC uses packet length forset_active(). HTB is trivial (for me) to fix while HFSC isn't. Because HFSC part it would be easier for me to declare fullNET_XMIT_SUCCESSas safe zone for skb pointer.
I guess some wiser guys should decide how serious problem it is.
Quoted 7 lines
- Jussi
PS. I noticed something fishy in HTB; HTB always returns
NET_XMIT_DROPifqdisc_enqueuedoesn't return fullNET_XMIT_SUCCESS, shouldn't it return return value fromqdisc_enqueue. Same in HTB requeue. That can't be right, right?
Yes, very good point, and quite hard to diagnose bug - happily solved already (but not fixed yet) by David Miller himself.
Jarek P.
PS: it seems your mailer wrapped some lines of above patch.
David Miller
maintainerRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
I still haven't seen anyone suggest creating a __NET_XMIT_KFREE_SKB to fix this most rediculious problem.
I'm still waiting...
David Miller
maintainerRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
From: David Miller <[email protected]>
Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)
Quoted 5 lines
I still haven't seen anyone suggest creating a
__NET_XMIT_KFREE_SKBto fix this most rediculious problem.I'm still waiting...
Here is what it might look like. I haven't tried to boot this, but the only non-trivial case was SFQ's congestion drop code.
include/net/sch_generic.h +11 -5
@@ -347,6 +347,7 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)enum net_xmit_qdisc_t { __NET_XMIT_STOLEN =0x00010000 , __NET_XMIT_BYPASS =0x00020000 , + __NET_XMIT_KFREE =0x00040000 , };#ifdef CONFIG_NET_CLS_ACT
@@ -366,8 +367,15 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) { +int ret; +qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ; + ret =qdisc_enqueue (skb, sch); + +if (unlikely (ret & __NET_XMIT_KFREE)) +kfree_skb (skb); + +return ret &NET_XMIT_MASK ; } static inline int __qdisc_enqueue_tail(struct sk_buff *skb, struct Qdisc *sch,
@@ -470,10 +478,9 @@ static inline unsigned int qdisc_queue_drop(struct Qdisc *sch)static inline int qdisc_drop (struct sk_buff *skb,struct Qdisc *sch) { -kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_DROP ; +return NET_XMIT_DROP | __NET_XMIT_KFREE; }static inline int qdisc_reshape_fail (struct sk_buff *skb,struct Qdisc *sch)
@@ -488,8 +495,7 @@ static inline int qdisc_reshape_fail(struct sk_buff *skb, struct Qdisc *sch) drop:#endif -kfree_skb (skb); -return NET_XMIT_DROP ; +return NET_XMIT_DROP | __NET_XMIT_KFREE; }/* Length to Time (L2T) lookup in a qdisc_rate_table, to determine how
net/sched/sch_atm.c +4 -3
@@ -414,10 +414,11 @@ static int atm_tc_enqueue(struct sk_buff *skb, struct Qdisc *sch)switch (result) {case TC_ACT_QUEUED :case TC_ACT_STOLEN : -kfree_skb (skb); -return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN; +return (NET_XMIT_SUCCESS | + __NET_XMIT_STOLEN | + __NET_XMIT_KFREE);case TC_ACT_SHOT : -kfree_skb (skb); + ret |= __NET_XMIT_KFREE; goto drop;case TC_POLICE_RECLASSIFY :if (flow->excess)
net/sched/sch_blackhole.c +1 -1
@@ -20,7 +20,7 @@static int blackhole_enqueue (struct sk_buff *skb,struct Qdisc *sch) {qdisc_drop (skb, sch); -return NET_XMIT_SUCCESS ; +return NET_XMIT_SUCCESS | __NET_XMIT_KFREE; }static struct sk_buff *blackhole_dequeue (struct Qdisc *sch)
net/sched/sch_cbq.c +1 -2
@@ -379,8 +379,7 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (cl ==NULL ) {if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++; -kfree_skb (skb); -return ret; +return ret | __NET_XMIT_KFREE; }#ifdef CONFIG_NET_CLS_ACT
net/sched/sch_dsmark.c +4 -4
@@ -235,8 +235,9 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch)#ifdef CONFIG_NET_CLS_ACT case TC_ACT_QUEUED :case TC_ACT_STOLEN : -kfree_skb (skb); -return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN; +return (NET_XMIT_SUCCESS | + __NET_XMIT_STOLEN | + __NET_XMIT_KFREE);case TC_ACT_SHOT : goto drop;
@@ -266,9 +267,8 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch)return NET_XMIT_SUCCESS ; drop: -kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS | __NET_XMIT_KFREE; }static struct sk_buff *dsmark_dequeue (struct Qdisc *sch)
net/sched/sch_generic.c +1 -2
@@ -291,8 +291,7 @@ EXPORT_SYMBOL(netif_carrier_off);static int noop_enqueue (struct sk_buff *skb,struct Qdisc * qdisc) { -kfree_skb (skb); -return NET_XMIT_CN ; +return NET_XMIT_CN | __NET_XMIT_KFREE; }static struct sk_buff *noop_dequeue (struct Qdisc * qdisc)
net/sched/sch_gred.c +1 -1
@@ -237,7 +237,7 @@ drop: congestion_drop:qdisc_drop (skb, sch); -return NET_XMIT_CN ; +return NET_XMIT_CN | __NET_XMIT_KFREE; }static int gred_requeue (struct sk_buff *skb,struct Qdisc * sch)
net/sched/sch_hfsc.c +1 -2
@@ -1580,8 +1580,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (cl ==NULL ) {if (err & __NET_XMIT_BYPASS) sch->qstats.drops++; -kfree_skb (skb); -return err; +return err | __NET_XMIT_KFREE; } err =qdisc_enqueue (skb, cl->qdisc);
net/sched/sch_htb.c +2 -4
@@ -561,16 +561,14 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch)__skb_queue_tail (&q->direct_queue, skb); q->direct_pkts++; }else { -kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_DROP ; +return NET_XMIT_DROP | __NET_XMIT_KFREE; } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) {if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++; -kfree_skb (skb); -return ret; +return ret | __NET_XMIT_KFREE;#endif }else if ((ret =qdisc_enqueue (skb, cl->un.leaf.q)) !=NET_XMIT_SUCCESS ) {if (net_xmit_drop_count (ret)) {
net/sched/sch_netem.c +1 -2
@@ -175,8 +175,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (count ==0 ) { sch->qstats.drops++; -kfree_skb (skb); -return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS | __NET_XMIT_KFREE; }skb_orphan (skb);
net/sched/sch_prio.c +1 -2
@@ -76,8 +76,7 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++; -kfree_skb (skb); - return ret; + return ret | __NET_XMIT_KFREE; }#endif
net/sched/sch_red.c +1 -1
@@ -105,7 +105,7 @@ static int red_enqueue(struct sk_buff *skb, struct Qdisc* sch) congestion_drop:qdisc_drop (skb, sch); -return NET_XMIT_CN ; +return NET_XMIT_CN | __NET_XMIT_KFREE; }static int red_requeue (struct sk_buff *skb,struct Qdisc * sch)
net/sched/sch_sfq.c +30 -10
@@ -232,7 +232,7 @@ static inline void sfq_inc(struct sfq_sched_data *q, sfq_index x)sfq_link (q, x); } -static unsigned int sfq_drop (struct Qdisc *sch) +static struct sk_buff *__sfq_drop (struct Qdisc *sch,int *len_p) {struct sfq_sched_data *q =qdisc_priv (sch);sfq_index d = q->max_depth;
@@ -247,12 +247,13 @@ static unsigned int sfq_drop(struct Qdisc *sch) skb = q->qs[x].prev; len =qdisc_pkt_len (skb);__skb_unlink (skb, &q->qs[x]); -kfree_skb (skb);sfq_dec (q, x); sch->q.qlen--; sch->qstats.drops++; sch->qstats.backlog -= len; -return len; +if (len_p) + *len_p = len; +return skb; }if (d ==1 ) {
@@ -263,22 +264,37 @@ static unsigned int sfq_drop(struct Qdisc *sch) skb = q->qs[d].prev; len =qdisc_pkt_len (skb);__skb_unlink (skb, &q->qs[d]); -kfree_skb (skb);sfq_dec (q, d); sch->q.qlen--; q->ht[q->hash[d]] =SFQ_DEPTH ; sch->qstats.drops++; sch->qstats.backlog -= len; -return len; +if (len_p) + *len_p = len; +return skb; } -return 0 ; +if (len_p) + *len_p =0 ; +return NULL ; +} + +static unsigned int sfq_drop (struct Qdisc *sch) +{ +struct sk_buff *skb; +int len; + + skb =__sfq_drop (sch, &len); +if (skb) +kfree_skb (skb); +return len; }static int sfq_enqueue (struct sk_buff *skb,struct Qdisc *sch) {struct sfq_sched_data *q =qdisc_priv (sch); +struct sk_buff *drop_skb;unsigned int hash;sfq_index x;int ret;
@@ -287,8 +303,7 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (hash ==0 ) {if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++; -kfree_skb (skb); -return ret; +return ret | __NET_XMIT_KFREE; } hash--;
@@ -325,8 +340,13 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch)return 0 ; } -sfq_drop (sch); -return NET_XMIT_CN ; + drop_skb =__sfq_drop (sch,NULL ); +if (drop_skb == skb) { +return NET_XMIT_CN | __NET_XMIT_KFREE; + }else { +kfree_skb (drop_skb); +return NET_XMIT_CN ; + } } static int
net/sched/sch_tbf.c +3 -2
@@ -125,12 +125,13 @@ static int tbf_enqueue(struct sk_buff *skb, struct Qdisc* sch)if (qdisc_pkt_len (skb) > q->max_size) { sch->qstats.drops++; + ret =NET_XMIT_DROP ;#ifdef CONFIG_NET_CLS_ACT if (sch->reshape_fail == NULL || sch->reshape_fail(skb, sch))#endif -kfree_skb (skb); + ret |= __NET_XMIT_KFREE; -return NET_XMIT_DROP ; +return ret; } ret =qdisc_enqueue (skb, q->qdisc);
net/sched/sch_teql.c +1 -2
@@ -88,9 +88,8 @@ teql_enqueue(struct sk_buff *skb, struct Qdisc* sch)return 0 ; } -kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_DROP ; +return NET_XMIT_DROP | __NET_XMIT_KFREE; } static int
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 26 lines · David Miller
From: David Miller <[email protected]>
Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)Quoted 5 lines
I still haven't seen anyone suggest creating a
__NET_XMIT_KFREE_SKBto fix this most rediculious problem.I'm still waiting...
Here is what it might look like. I haven't tried to boot this, but the only non-trivial case was SFQ's congestion drop code.
include/net/sch_generic.h +1
-0@@ -347,6 +347,7 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)enum net_xmit_qdisc_t { __NET_XMIT_STOLEN =0x00010000 , __NET_XMIT_BYPASS =0x00020000 , + __NET_XMIT_KFREE =0x00040000 , };#ifdef CONFIG_NET_CLS_ACT @@ -366,8 +367,15 @@ static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
include/net/sch_generic.h static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch) · line 376
Quoted 10 lines
include/net/sch_generic.h +6
-1static inline int qdisc_enqueue_root (struct sk_buff *skb,struct Qdisc *sch) { +int ret; +qdisc_skb_cb (skb)->pkt_len = skb->len; -return qdisc_enqueue (skb, sch) &NET_XMIT_MASK ; + ret =qdisc_enqueue (skb, sch); + +if (unlikely (ret & __NET_XMIT_KFREE)) +kfree_skb (skb);
Looks very interesting, but isn't there possible a little optimization? If (almost?) everything not NET_XMIT_SUCCESS does this maybe?:
if (unlikely(ret != NET_XMIT_SUCCESS))
kfree_skb(skb);
or:
if (unlikely(ret != NET_XMIT_SUCCESS &&
!(ret & __NET_XMIT_SKIP_KFREE)))
kfree_skb(skb);
Anyway, it's quite a bit of change, so I definitely need more time to check this (after awakening...).
Jarek P.
Quoted 3 lines
include/net/sch_generic.h +2
-0+ +return ret &NET_XMIT_MASK ; }
...
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
In reply to David MillerQuoted 11 lines · David Miller
From: David Miller <[email protected]>
Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)Quoted 5 lines
I still haven't seen anyone suggest creating a
__NET_XMIT_KFREE_SKBto fix this most rediculious problem.I'm still waiting...
Here is what it might look like. I haven't tried to boot this, but the only non-trivial case was SFQ's congestion drop code.
After some checking it looks mostly OK to me, but one thing: in sch_gred gred_drop() calls qdisc_drop(), so now it needs kfree_skb(). BTW, maybe it would be nicer to add __qdisc_drop() for these new things?
There is also some doubt about differences between ->enqueue() and ->requeue() wrt. kfree_skb() and returning codes: maybe it would be better (for uniformity) to add similar changes to requeues (and dev_requeue_skb()) as well?
I don't know if it's worth to mention, but now, if somebody really uses this sch_blackhole under some upper qdiscs the stats will change.
Jarek P.
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 5 lines
There is also some doubt about differences between ->enqueue() and ->requeue() wrt.
kfree_skb()and returning codes: maybe it would be better (for uniformity) to add similar changes to requeues (anddev_requeue_skb()) as well?
I think requeue should be changed to return same as enqueue, netem even uses requeue as enqueue replacement for packet reordering. Maybe add new function qdisc_requeue_tree to handle freeing and masking flags and change outside uses of requeue to use it (qdisc_peek_len in hfsc, sch_atm_dequeue, dev_requeue_skb).
- Jussi
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 8 lines · Jussi Kivilinna
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 5 lines
There is also some doubt about differences between ->enqueue() and ->requeue() wrt.
kfree_skb()and returning codes: maybe it would be better (for uniformity) to add similar changes to requeues (anddev_requeue_skb()) as well?
Quoted 5 lines
I think requeue should be changed to return same as enqueue, netem even uses requeue as enqueue replacement for packet reordering. Maybe add new function
qdisc_requeue_treeto handle freeing and masking flags and change outside uses of requeue to use it (qdisc_peek_lenin hfsc,sch_atm_dequeue,dev_requeue_skb).
Yes, good point again: if ->enqueue() could be mixed with ->requeue() there is more than uniformity argument to do the change everywhere.
Jarek P.
David Miller
maintainerRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
In reply to Jussi KivilinnaFrom: "Jussi Kivilinna" <[email protected]>
Date: Thu, 07 Aug 2008 14:36:54 +0300
Quoted 8 lines
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 5 lines
There is also some doubt about differences between ->enqueue() and ->requeue() wrt.
kfree_skb()and returning codes: maybe it would be better (for uniformity) to add similar changes to requeues (anddev_requeue_skb()) as well?
Quoted 5 lines
I think requeue should be changed to return same as enqueue, netem even uses requeue as enqueue replacement for packet reordering. Maybe add new function
qdisc_requeue_treeto handle freeing and masking flags and change outside uses of requeue to use it (qdisc_peek_lenin hfsc,sch_atm_dequeue,dev_requeue_skb).
Yes, I in fact stumbled through this stuff while working on the HTB/TBF ->enqueue() fixes.
It's not going to be easy to cure this completely and we'll have to work in stages.
What I'll do tonight is first make a simple set of fixes, like fixing these things to use the proper NET_XMIT_* values instead of constant "0" and stuff like that.
Next I'll do the real ->requeue() return value audit.
As an aside I still think we really don't need ->requeue(), once we pull a packet out of ->enqueue() it's out of the packet scheduler's realm and we can definitely hold onto it as the next packet to give to the device. This netem code is really an abuse of this ->requeue() callback. It was never meant to be used like that.
In fact, as I look at how TBF and NETEM want to use ->requeue() they could just the same implement it using my suggested alternative for ->requeue() which is just an SKB list ala gso_list hung off of the qdisc.
They all just want to "insert to front" when they call ->requeue(), nothing more.
In the TBF case, it wants to defer the send to some time in the future. But that would work in my scheme as well as it would now. TBF's ->enqueue() would return NULL yet stick the packet into the qdisc->skb_list, the watchdog is schedules will run the queue when it fires.
So, in short, the more I think about it the more I think we can certainly kill off ->requeue() and all of it's resulting complexity.
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 5 lines · David Miller
In fact, as I look at how TBF and NETEM want to use ->requeue() they could just the same implement it using my suggested alternative for ->requeue() which is just an SKB list ala
gso_listhung off of the qdisc.
For TBF at least its requeue is actually a bit more sophisticated. For example, if you hang a prio qdisc off a TBF, then when the packet is requeued into the prio, the next time TBF dequeues it, it may get a different packet (of a higher priority).
If you stick it to a list without requeueing, then packets of a higher priority won't be able to jump the queue anymore.
It might be possible to rewrite TBF to not use requeueing, after all HTB does it all without requeueing. However as it stands, TBF does need it.
I haven't followed this thread closely so what is the main problem with requeueing (apart from being abused by things like LLTX drivers and/or NETEM, nothing other than qdiscs like TBF should use it)?
Another possibility would to replace requeue by a peek+force_dequeue interface, where you can peek at the next packet, and you could then dequeue that particular packet if you're satisfied.
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 18 lines · Herbert Xu
Quoted 4 lines · David Miller
In fact, as I look at how TBF and NETEM want to use ->requeue() they could just the same implement it using my suggested alternative for ->requeue() which is just an SKB list ala
gso_listhung off of the qdisc.For TBF at least its requeue is actually a bit more sophisticated. For example, if you hang a prio qdisc off a TBF, then when the packet is requeued into the prio, the next time TBF dequeues it, it may get a different packet (of a higher priority).
If you stick it to a list without requeueing, then packets of a higher priority won't be able to jump the queue anymore.
It might be possible to rewrite TBF to not use requeueing, after all HTB does it all without requeueing. However as it stands, TBF does need it.
Quoted 7 lines
I haven't followed this thread closely so what is the main problem with requeueing (apart from being abused by things like LLTX drivers and/or NETEM, nothing other than qdiscs like TBF should use it)?
Another possibility would to replace requeue by a peek+force_dequeue interface, where you can peek at the next packet, and you could then dequeue that particular packet if you're satisfied.
That would be fine for TBF since it only needs to check whether the current packet exceeds the limit and reschedule otherwise. HFSC OTOH really needs to know the length of the next packet for calculating the deadline.
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 9 lines · Patrick McHardy
Quoted 4 lines
Another possibility would to replace requeue by a peek+force_dequeue interface, where you can peek at the next packet, and you could then dequeue that particular packet if you're satisfied.
That would be fine for TBF since it only needs to check whether the current packet exceeds the limit and reschedule otherwise. HFSC OTOH really needs to know the length of the next packet for calculating the deadline.
You mean a peek interface is insufficient for HFSC? Could you elaborate?
Thanks,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 11 lines · Herbert Xu
Quoted 7 lines · Patrick McHardy
Quoted 3 lines
Another possibility would to replace requeue by a peek+force_dequeue interface, where you can peek at the next packet, and you could then dequeue that particular packet if you're satisfied.
That would be fine for TBF since it only needs to check whether the current packet exceeds the limit and reschedule otherwise. HFSC OTOH really needs to know the length of the next packet for calculating the deadline.
You mean a peek interface is insufficient for HFSC? Could you elaborate?
I might have misunderstood you, but the way I imagine force_dequeue is that it would give you the packet peeked at, even if a higher priority packet is available.
But actually I don't understand the use for force_dequeue at all, assuming ->peek behaves correctly ->dequeue should already hand out the correct packet.
(Note: Its OK to hand out a different packet if we had a ->enqueue operation after ->peek since the upper qdisc can just re-peek and recalculate based on the new highest priority packet).
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 8 lines · Patrick McHardy
I might have misunderstood you, but the way I imagine
force_dequeueis that it would give you the packet peeked at, even if a higher priority packet is available.But actually I don't understand the use for
force_dequeueat all, assuming ->peek behaves correctly ->dequeue should already hand out the correct packet.
You're quite right, we don't need a forced dequeue at all because all dequeueing and enqueueing occur under the root qdisc lock, so the peek result has to be the same as that of the next dequeue.
Problem solved :)
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Just want to report, latest net-2.6 works fine for me (intensive classes creation/deletion, tbf, flow classifier, htb, pfifo) , no crashes, no warnings.
I will test also after few days on backbone router and probably router with hfsc.
Thanks for great code and support :-)
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 8 lines · Denys Fedoryshchenko
Just want to report, latest net-2.6 works fine for me (intensive classes creation/deletion, tbf, flow classifier, htb, pfifo) , no crashes, no warnings.
I will test also after few days on backbone router and probably router with hfsc.
Thanks for great code and support :-)
Thanks for great reporting and testing Denys,
Jarek P.
David Miller
maintainerRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
In reply to Denys FedoryshchenkoFrom: Denys Fedoryshchenko <[email protected]>
Date: Tue, 19 Aug 2008 23:10:52 +0300
Quoted 1 line
Thanks for great code and support :-)
Thanks for all of your excellent testing :-)
David Miller
maintainerRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
In reply to Jarek PoplawskiFrom: Jarek Poplawski <[email protected]>
Date: Thu, 7 Aug 2008 10:09:10 +0000
Quoted 10 lines · David Miller
Quoted 9 lines
From: David Miller <[email protected]>
Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)Quoted 5 lines
I still haven't seen anyone suggest creating a
__NET_XMIT_KFREE_SKBto fix this most rediculious problem.I'm still waiting...
Quoted 7 lines
Quoted 2 lines
Here is what it might look like. I haven't tried to boot this, but the only non-trivial case was SFQ's congestion drop code.
After some checking it looks mostly OK to me, but one thing: in
sch_gredgred_drop()callsqdisc_drop(), so now it needskfree_skb(). BTW, maybe it would be nicer to add__qdisc_drop()for these new things?
qdisc_drop() sets the new __NET_XMIT_KFREE bit, but sch_gred wants to return NET_XMIT_CN, so I OR'd in the __NET_XMIT_KFREE bit there.
Quoted 4 lines
There is also some doubt about differences between ->enqueue() and ->requeue() wrt.
kfree_skb()and returning codes: maybe it would be better (for uniformity) to add similar changes to requeues (anddev_requeue_skb()) as well?
I did not annotate ->requeue() nor remove kfree_skb() calls there and this was intentional.
The lifetime issue only exists in the ->enqueue() path.
In the long term we could add a wrapper around root level ->requeue() and do the bit handling just like ->enqueue(), sure.
Quoted 2 lines
I don't know if it's worth to mention, but now, if somebody really uses this
sch_blackholeunder some upper qdiscs the stats will change.
That might even be useful.
As it stands it was borderline buggy.
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
Quoted 2 lines · David Miller
From: Jarek Poplawski <[email protected]>
Date: Thu, 7 Aug 2008 10:09:10 +0000
...
Quoted 7 lines
Quoted 4 lines
After some checking it looks mostly OK to me, but one thing: in
sch_gredgred_drop()callsqdisc_drop(), so now it needskfree_skb(). BTW, maybe it would be nicer to add__qdisc_drop()for these new things?
qdisc_drop()sets the new__NET_XMIT_KFREEbit, butsch_gredwants to returnNET_XMIT_CN, so I OR'd in the__NET_XMIT_KFREEbit there.
Hmm... I'm not sure we're thinking about the same function?
Jarek P.
David Miller
maintainerRe: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
From: Jarek Poplawski <[email protected]>
Date: Thu, 7 Aug 2008 10:31:30 +0000
Quoted 9 lines · David Miller
Quoted 2 lines
From: Jarek Poplawski <[email protected]>
Date: Thu, 7 Aug 2008 10:09:10 +0000...
Quoted 5 lines
Quoted 4 lines
After some checking it looks mostly OK to me, but one thing: in
sch_gredgred_drop()callsqdisc_drop(), so now it needskfree_skb(). BTW, maybe it would be nicer to add__qdisc_drop()for these new things?
Quoted 4 lines
Quoted 2 lines
qdisc_drop()sets the new__NET_XMIT_KFREEbit, butsch_gredwants to returnNET_XMIT_CN, so I OR'd in the__NET_XMIT_KFREEbit there.Hmm... I'm not sure we're thinking about the same function?
Indeed, you're right.
I've added the kfree_skb() call to gred_drop() in my local tree.
Thanks!
BTW, speaking of ->requeue(), I think we could easily eliminate that thing. It only exists because we allow drivers to kick things back to us via ->hard_start_xmit() return values.
There are two cases:
1) NETDEV_TX_LOCKED... thanks to the bogon named LLTX
2) NETDEV_TX_BUSY, which is pretty much a bug
%99.999 of the cases that return NETDEV_TX_BUSY are error conditions in the driver which log a message and we could just as validly drop the packet for this case
But anyways, LLTX isn't dying tomorrow as much as we'd like it go away.
But we could cache the SKB instead of requeueing it, just like how we handle the ->gso_skb right now.
In fact it seems we can just reuse the ->gso_skb Qdisc member for this purpose.
Then all of the ->requeue() code and resulting complexity can just go away.
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb
On Thu, Aug 07, 2008 at 03:45:45AM -0700, David Miller wrote:
...
Quoted 18 lines
BTW, speaking of ->requeue(), I think we could easily eliminate that thing. It only exists because we allow drivers to kick things back to us via ->hard_start_xmit() return values.
There are two cases:
1)
NETDEV_TX_LOCKED... thanks to the bogon named LLTX2)
NETDEV_TX_BUSY, which is pretty much a bug%99.999 of the cases that return
NETDEV_TX_BUSYare error conditions in the driver which log a message and we could just as validly drop the packet for this caseBut anyways, LLTX isn't dying tomorrow as much as we'd like it go away.
Quoted 8 lines
But we could cache the SKB instead of requeueing it, just like how we handle the ->gso_skb right now.
In fact it seems we can just reuse the ->gso_skb Qdisc member for this purpose.
Then all of the ->requeue() code and resulting complexity can just go away.
In most cases it looks like the right thing, but it seems there are cases (RT or OOB) which could need more control on this, so they would sometimes prefer to change the order of next xmitted packet if previous had failed, I guess.
Jarek P.
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb (Was: Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag)
In reply to Jarek PoplawskiQuoting "Jarek Poplawski" <[email protected]>:
Quoted 7 lines
Quoted 4 lines
PS. I noticed something fishy in HTB; HTB always returns
NET_XMIT_DROPifqdisc_enqueuedoesn't return fullNET_XMIT_SUCCESS, shouldn't it return return value fromqdisc_enqueue. Same in HTB requeue. That can't be right, right?Yes, very good point, and quite hard to diagnose bug - happily solved already (but not fixed yet) by David Miller himself.
Ok, I assume that same case in sch_prio:requeue has been already spotted too.
Quoted 3 lines
PS: it seems your mailer wrapped some lines of above patch.
Doh, time to find alternative for horde.
- Jussi
Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb (Was: Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag)
Quoted 8 lines · Jussi Kivilinna
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 6 lines
Quoted 5 lines
PS. I noticed something fishy in HTB; HTB always returns
NET_XMIT_DROPifqdisc_enqueuedoesn't return fullNET_XMIT_SUCCESS, shouldn't it return return
value fromqdisc_enqueue. Same in HTB requeue. That can't be right, right?
Quoted 5 lines
Quoted 2 lines
Yes, very good point, and quite hard to diagnose bug - happily solved already (but not fixed yet) by David Miller himself.
Ok, I assume that same case in sch_prio:requeue has been already spotted too.
Actually, I didn't hear about this!? After the HTB case David waits for Patrick's audit, and this seems to get some time.
So, looks like another good point, and IMHO, you should send a patch for for this.
Jarek P.
David Miller
maintainerRe: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag
In reply to Jussi KivilinnaFrom: "Jussi Kivilinna" <[email protected]>
Date: Tue, 05 Aug 2008 15:43:50 +0300
Quoted 8 lines
Quoting "Jarek Poplawski" <[email protected]>:
Quoted 6 lines · Jussi Kivilinna
Quoted 4 lines
Quoting "Jarek Poplawski" <[email protected]>:
This seems to also make sure that when returning 'full'
NET_XMIT_SUCCESS, skb pointer is safe(r?) to use now.
Quoted 7 lines
Quoted 4 lines
I guess so... At least theoretically - I was mainly afraid of redirecting, which isn't fully implemented yet. On the other hand, outwards of qdiscs there is still no difference.
How about making skb shared before passing into qdisc tree? That would make skb usage safe after qdisc enqueues.
That's two extra atomic operations on every transmitted packet, no thanks :-)
PATCH 1/1 net_sched: Add qdisc __NET_XMIT_BYPASS flag
In reply to David MillerOOPS!!! Forget take!
Sorry,
Jarek P.
------------------> (take 3)
Subject: net_sched: Add qdisc __NET_XMIT_BYPASS flag
Patrick McHardy <[email protected]> noticed that it would be nice to handle NET_XMIT_BYPASS by NET_XMIT_SUCCESS with an internal qdisc flag __NET_XMIT_BYPASS and to remove the mapping from dev_queue_xmit().
David Miller <[email protected]> spotted a serious bug in the first version of this patch.
---
(apply on top of [PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag)
include/net/sch_generic.h | 8 ++++---- net/core/dev.c | 1 - net/sched/sch_atm.c | 2 +- net/sched/sch_cbq.c | 4 ++-- net/sched/sch_dsmark.c | 2 +- net/sched/sch_hfsc.c | 4 ++-- net/sched/sch_htb.c | 6 +++--- net/sched/sch_netem.c | 2 +- net/sched/sch_prio.c | 6 +++--- net/sched/sch_sfq.c | 6 +++--- 10 files changed, 20 insertions(+), 21 deletions(-)
include/net/sch_generic.h +4 -4
@@ -331,14 +331,14 @@ static inline unsigned int qdisc_pkt_len(struct sk_buff *skb)return qdisc_skb_cb (skb)->pkt_len; } -#ifdef CONFIG_NET_CLS_ACT -/* additional qdisc xmit flags */ +/* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */ enum net_xmit_qdisc_t { - __NET_XMIT_STOLEN =NET_XMIT_MASK +1 , + __NET_XMIT_STOLEN =0x00010000 , + __NET_XMIT_BYPASS =0x00020000 , }; +#ifdef CONFIG_NET_CLS_ACT #define net_xmit_drop_count (e) ((e) & __NET_XMIT_STOLEN ? 0 : 1) -#else #define net_xmit_drop_count (e) (1)#endif
net/core/dev.c +0 -1
@@ -1805,7 +1805,6 @@ gso:spin_unlock (root_lock); - rc = rc ==NET_XMIT_BYPASS ?NET_XMIT_SUCCESS : rc; goto out; }
net/sched/sch_atm.c +1 -1
@@ -457,7 +457,7 @@ drop: __maybe_unusedreturn 0 ; }tasklet_schedule (&p->task); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }/*
net/sched/sch_cbq.c +2 -2
@@ -230,7 +230,7 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) (cl = cbq_class_lookup(q, prio)) != NULL)return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;for (;;) {int result =0 ; defmap = head->defaults;
@@ -377,7 +377,7 @@ cbq_enqueue(struct sk_buff *skb, struct Qdisc *sch) q->rx_class = cl;#endif if (cl ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_dsmark.c +1 -1
@@ -268,7 +268,7 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch) drop:kfree_skb (skb); sch->qstats.drops++; -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }static struct sk_buff *dsmark_dequeue (struct Qdisc *sch)
net/sched/sch_hfsc.c +2 -2
@@ -1159,7 +1159,7 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)if (cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->root.filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -1578,7 +1578,7 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch) cl =hfsc_classify (skb, sch, &err);if (cl ==NULL ) { -if (err ==NET_XMIT_BYPASS ) +if (err & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return err;
net/sched/sch_htb.c +3 -3
@@ -214,7 +214,7 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,if ((cl =htb_find (skb->priority, sch)) !=NULL && cl->level ==0 )return cl; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; tcf = q->filter_list;while (tcf && (result =tc_classify (skb, tcf, &res)) >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -567,7 +567,7 @@ static int htb_enqueue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -612,7 +612,7 @@ static int htb_requeue(struct sk_buff *skb, struct Qdisc *sch) } #ifdef CONFIG_NET_CLS_ACT }else if (!cl) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_netem.c +1 -1
@@ -176,7 +176,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)if (count ==0 ) { sch->qstats.drops++;kfree_skb (skb); -return NET_XMIT_BYPASS ; +return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; }skb_orphan (skb);
net/sched/sch_prio.c +3 -3
@@ -38,7 +38,7 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)struct tcf_result res;int err; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;if (TC_H_MAJ (skb->priority) != sch->handle) { err =tc_classify (skb, q->filter_list, &res);#ifdef CONFIG_NET_CLS_ACT
@@ -74,7 +74,7 @@ prio_enqueue(struct sk_buff *skb, struct Qdisc *sch)#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -103,7 +103,7 @@ prio_requeue(struct sk_buff *skb, struct Qdisc* sch) qdisc =prio_classify (skb, sch, &ret);#ifdef CONFIG_NET_CLS_ACT if (qdisc ==NULL ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
net/sched/sch_sfq.c +3 -3
@@ -171,7 +171,7 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,if (!q->filter_list)return sfq_hash (q, skb) +1 ; - *qerr =NET_XMIT_BYPASS ; + *qerr =NET_XMIT_SUCCESS | __NET_XMIT_BYPASS; result =tc_classify (skb, q->filter_list, &res);if (result >=0 ) {#ifdef CONFIG_NET_CLS_ACT
@@ -285,7 +285,7 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;
@@ -339,7 +339,7 @@ sfq_requeue(struct sk_buff *skb, struct Qdisc *sch) hash =sfq_classify (skb, sch, &ret);if (hash ==0 ) { -if (ret ==NET_XMIT_BYPASS ) +if (ret & __NET_XMIT_BYPASS) sch->qstats.drops++;kfree_skb (skb);return ret;