[RFC 0/3] tc cleanup?

[RFC 0/3] tc cleanup?

[Alexei Starovoitov]

Hi,

I've started cleaning TC a bit.
Before I go too far, need your feedback on this RFC:

patch 1 - stop abuse of return values in ingress qdisc
patch 2 - deprecate TC_ACT_QUEUED
patch 3 - reduce copy-paste around tc_classify()

Lightly tested so far. Waiting for John's and other tc test scripts.

Alexei Starovoitov (3):
  tc: fix return values of ingress qdisc
  tc: deprecate TC_ACT_QUEUED
  tc: cleanup tc_classify

 include/net/pkt_sched.h      |    2 ++
 include/net/sch_generic.h    |    7 +++++++
 include/uapi/linux/pkt_cls.h |    2 +-
 net/core/dev.c               |    8 ++------
 net/sched/sch_api.c          |   22 ++++++++++++++++++++++
 net/sched/sch_atm.c          |    1 -
 net/sched/sch_cbq.c          |    1 -
 net/sched/sch_choke.c        |   17 +++--------------
 net/sched/sch_drr.c          |   18 +++---------------
 net/sched/sch_dsmark.c       |    1 -
 net/sched/sch_fq_codel.c     |   25 ++++++-------------------
 net/sched/sch_hfsc.c         |    1 -
 net/sched/sch_htb.c          |    1 -
 net/sched/sch_ingress.c      |   10 ++++------
 net/sched/sch_multiq.c       |    1 -
 net/sched/sch_prio.c         |    1 -
 net/sched/sch_qfq.c          |   16 ++--------------
 net/sched/sch_sfb.c          |   17 +++--------------
 net/sched/sch_sfq.c          |   26 ++++++--------------------
 19 files changed, 61 insertions(+), 116 deletions(-)

-- 
1.7.9.5

[RFC 1/3] tc: fix return values of ingress qdisc

[Alexei Starovoitov]

ingress qdisc should return NET_XMIT_* values just like all other qdiscs.

Since it's invoked via qdisc_enqueue_root() (which suppose to return
only NET_XMIT_* values as well), it was working by accident,
since TC_ACT_* values fit within NET_XMIT_MASK.

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 net/core/dev.c          |    8 ++------
 net/sched/sch_ingress.c |    9 ++++-----
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 1796cef55ab5..ac6233f6f353 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3533,7 +3533,7 @@ static int ing_filter(struct sk_buff *skb, struct netdev_queue *rxq)
 {
 	struct net_device *dev = skb->dev;
 	u32 ttl = G_TC_RTTL(skb->tc_verd);
-	int result = TC_ACT_OK;
+	int result = NET_XMIT_SUCCESS;
 	struct Qdisc *q;
 
 	if (unlikely(MAX_RED_LOOP < ttl++)) {
@@ -3570,12 +3570,8 @@ static inline struct sk_buff *handle_ing(struct sk_buff *skb,
 		*pt_prev = NULL;
 	}
 
-	switch (ing_filter(skb, rxq)) {
-	case TC_ACT_SHOT:
-	case TC_ACT_STOLEN:
-		kfree_skb(skb);
+	if (ing_filter(skb, rxq) == NET_XMIT_DROP)
 		return NULL;
-	}
 
 	return skb;
 }
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 4cdbfb85686a..e68f4a5dbeba 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -65,21 +65,20 @@ static int ingress_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 
 	result = tc_classify(skb, fl, &res);
 
-	qdisc_bstats_update(sch, skb);
 	switch (result) {
 	case TC_ACT_SHOT:
-		result = TC_ACT_SHOT;
 		qdisc_qstats_drop(sch);
-		break;
 	case TC_ACT_STOLEN:
 	case TC_ACT_QUEUED:
-		result = TC_ACT_STOLEN;
+		result = NET_XMIT_DROP;
+		kfree_skb(skb);
 		break;
 	case TC_ACT_RECLASSIFY:
 	case TC_ACT_OK:
 		skb->tc_index = TC_H_MIN(res.classid);
 	default:
-		result = TC_ACT_OK;
+		qdisc_bstats_update(sch, skb);
+		result = NET_XMIT_SUCCESS;
 		break;
 	}
 
-- 
1.7.9.5

[RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Alexei Starovoitov]

TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
Get rid of redundant checks in all qdiscs.
Instead do it once.

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 include/uapi/linux/pkt_cls.h |    2 +-
 net/sched/sch_api.c          |    2 ++
 net/sched/sch_atm.c          |    1 -
 net/sched/sch_cbq.c          |    1 -
 net/sched/sch_choke.c        |    1 -
 net/sched/sch_drr.c          |    1 -
 net/sched/sch_dsmark.c       |    1 -
 net/sched/sch_fq_codel.c     |    1 -
 net/sched/sch_hfsc.c         |    1 -
 net/sched/sch_htb.c          |    1 -
 net/sched/sch_ingress.c      |    1 -
 net/sched/sch_multiq.c       |    1 -
 net/sched/sch_prio.c         |    1 -
 net/sched/sch_qfq.c          |    1 -
 net/sched/sch_sfb.c          |    1 -
 net/sched/sch_sfq.c          |    1 -
 16 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h
index bf08e76bf505..208e5ed5256c 100644
--- a/include/uapi/linux/pkt_cls.h
+++ b/include/uapi/linux/pkt_cls.h
@@ -102,7 +102,7 @@ enum {
 #define TC_ACT_SHOT		2
 #define TC_ACT_PIPE		3
 #define TC_ACT_STOLEN		4
-#define TC_ACT_QUEUED		5
+#define TC_ACT_QUEUED		5 /* deprecated. same as TC_ACT_STOLEN */
 #define TC_ACT_REPEAT		6
 #define TC_ACT_JUMP		0x10000000
 
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index ad9eed70bc8f..f7950327bb22 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1820,6 +1820,8 @@ int tc_classify_compat(struct sk_buff *skb, const struct tcf_proto *tp,
 #ifdef CONFIG_NET_CLS_ACT
 			if (err != TC_ACT_RECLASSIFY && skb->tc_verd)
 				skb->tc_verd = SET_TC_VERD(skb->tc_verd, 0);
+			if (err == TC_ACT_QUEUED)
+				return TC_ACT_STOLEN;
 #endif
 			return err;
 		}
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index e3e2cc5fd068..7ef0bf4bdce6 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -396,7 +396,6 @@ done:
 		/*@@@ looks good ... but it's not supposed to work :-) */
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			kfree_skb(skb);
 			return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index beeb75f80fdb..eca4725e3273 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -258,7 +258,6 @@ cbq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 			goto fallback;
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index c009eb9045ce..a3bc7cf151d3 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -212,7 +212,6 @@ static bool choke_classify(struct sk_buff *skb,
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
 		case TC_ACT_STOLEN:
-		case TC_ACT_QUEUED:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
 			return false;
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 338706092c27..1051c5d4e85b 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -335,7 +335,6 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
 	if (result >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 66700a6116aa..ce9d4123cbbe 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -236,7 +236,6 @@ static int dsmark_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 
 		switch (result) {
 #ifdef CONFIG_NET_CLS_ACT
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			kfree_skb(skb);
 			return NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 1e52decb7b59..4d00ece3243d 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -104,7 +104,6 @@ static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
 		case TC_ACT_STOLEN:
-		case TC_ACT_QUEUED:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
 			return 0;
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index e6c7416d0332..3438f9b4fd8b 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -1168,7 +1168,6 @@ hfsc_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 	while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index f1acb0f60dc3..32c1f081bed9 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -232,7 +232,6 @@ static struct htb_class *htb_classify(struct sk_buff *skb, struct Qdisc *sch,
 	while (tcf && (result = tc_classify(skb, tcf, &res)) >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index e68f4a5dbeba..63cbd1e066bd 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -69,7 +69,6 @@ static int ingress_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	case TC_ACT_SHOT:
 		qdisc_qstats_drop(sch);
 	case TC_ACT_STOLEN:
-	case TC_ACT_QUEUED:
 		result = NET_XMIT_DROP;
 		kfree_skb(skb);
 		break;
diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c
index 42dd218871e0..dc2a708a772c 100644
--- a/net/sched/sch_multiq.c
+++ b/net/sched/sch_multiq.c
@@ -50,7 +50,6 @@ multiq_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 #ifdef CONFIG_NET_CLS_ACT
 	switch (err) {
 	case TC_ACT_STOLEN:
-	case TC_ACT_QUEUED:
 		*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 	case TC_ACT_SHOT:
 		return NULL;
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index 8e5cd34aaa74..de4825a9d0aa 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -46,7 +46,6 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 #ifdef CONFIG_NET_CLS_ACT
 		switch (err) {
 		case TC_ACT_STOLEN:
-		case TC_ACT_QUEUED:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
 			return NULL;
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 3ec7e88a43ca..9d1e43ea0ccb 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -723,7 +723,6 @@ static struct qfq_class *qfq_classify(struct sk_buff *skb, struct Qdisc *sch,
 	if (result >= 0) {
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
-		case TC_ACT_QUEUED:
 		case TC_ACT_STOLEN:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 5819dd82630d..61e18108d0d3 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -264,7 +264,6 @@ static bool sfb_classify(struct sk_buff *skb, struct tcf_proto *fl,
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
 		case TC_ACT_STOLEN:
-		case TC_ACT_QUEUED:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
 			return false;
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index b877140beda5..b508ff655da3 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -207,7 +207,6 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,
 #ifdef CONFIG_NET_CLS_ACT
 		switch (result) {
 		case TC_ACT_STOLEN:
-		case TC_ACT_QUEUED:
 			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
 		case TC_ACT_SHOT:
 			return 0;
-- 
1.7.9.5

[RFC 3/3] tc: cleanup tc_classify

[Alexei Starovoitov]

introduce tc_classify_act() and qdisc_drop_bypass() helper functions to reduce
copy-paste among different qdiscs

Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 include/net/pkt_sched.h   |    2 ++
 include/net/sch_generic.h |    7 +++++++
 net/sched/sch_api.c       |   20 ++++++++++++++++++++
 net/sched/sch_choke.c     |   16 +++-------------
 net/sched/sch_drr.c       |   17 +++--------------
 net/sched/sch_fq_codel.c  |   24 ++++++------------------
 net/sched/sch_qfq.c       |   15 ++-------------
 net/sched/sch_sfb.c       |   16 +++-------------
 net/sched/sch_sfq.c       |   25 ++++++-------------------
 9 files changed, 52 insertions(+), 90 deletions(-)

diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 2342bf12cb78..7c73cbe95169 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -114,6 +114,8 @@ int tc_classify_compat(struct sk_buff *skb, const struct tcf_proto *tp,
 		       struct tcf_result *res);
 int tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 		struct tcf_result *res);
+int tc_classify_act(struct sk_buff *skb, const struct tcf_proto *tp,
+		    struct tcf_result *res, int *qerr);
 
 static inline __be16 tc_skb_protocol(const struct sk_buff *skb)
 {
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 6d778efcfdfd..9a50bad24b1d 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -715,6 +715,13 @@ static inline int qdisc_drop(struct sk_buff *skb, struct Qdisc *sch)
 	return NET_XMIT_DROP;
 }
 
+static inline void qdisc_drop_bypass(struct sk_buff *skb, struct Qdisc *sch, int err)
+{
+	if (err & __NET_XMIT_BYPASS)
+		qdisc_qstats_drop(sch);
+	kfree_skb(skb);
+}
+
 static inline int qdisc_reshape_fail(struct sk_buff *skb, struct Qdisc *sch)
 {
 	qdisc_qstats_drop(sch);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f7950327bb22..c7c4a672eb35 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1860,6 +1860,26 @@ reclassify:
 }
 EXPORT_SYMBOL(tc_classify);
 
+int tc_classify_act(struct sk_buff *skb, const struct tcf_proto *tp,
+		    struct tcf_result *res, int *qerr)
+{
+	int result;
+
+	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
+	result = tc_classify(skb, tp, res);
+
+#ifdef CONFIG_NET_CLS_ACT
+	switch (result) {
+	case TC_ACT_STOLEN:
+		*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
+	case TC_ACT_SHOT:
+		return -1;
+	}
+#endif
+	return result;
+}
+EXPORT_SYMBOL(tc_classify_act);
+
 bool tcf_destroy(struct tcf_proto *tp, bool force)
 {
 	if (tp->ops->destroy(tp, force)) {
diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index a3bc7cf151d3..8d8ad5303497 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -207,16 +207,8 @@ static bool choke_classify(struct sk_buff *skb,
 	int result;
 
 	fl = rcu_dereference_bh(q->filter_list);
-	result = tc_classify(skb, fl, &res);
+	result = tc_classify_act(skb, fl, &res, qerr);
 	if (result >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-		switch (result) {
-		case TC_ACT_STOLEN:
-			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
-		case TC_ACT_SHOT:
-			return false;
-		}
-#endif
 		choke_set_classid(skb, TC_H_MIN(res.classid));
 		return true;
 	}
@@ -268,9 +260,9 @@ static bool choke_match_random(const struct choke_sched_data *q,
 
 static int choke_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
-	int ret = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	struct choke_sched_data *q = qdisc_priv(sch);
 	const struct red_parms *p = &q->parms;
+	int ret;
 
 	if (rcu_access_pointer(q->filter_list)) {
 		/* If using external classifiers, get result and record it. */
@@ -343,9 +335,7 @@ congestion_drop:
 	return NET_XMIT_CN;
 
 other_drop:
-	if (ret & __NET_XMIT_BYPASS)
-		qdisc_qstats_drop(sch);
-	kfree_skb(skb);
+	qdisc_drop_bypass(skb, sch, ret);
 	return ret;
 }
 
diff --git a/net/sched/sch_drr.c b/net/sched/sch_drr.c
index 1051c5d4e85b..36ab69375c79 100644
--- a/net/sched/sch_drr.c
+++ b/net/sched/sch_drr.c
@@ -329,18 +329,9 @@ static struct drr_class *drr_classify(struct sk_buff *skb, struct Qdisc *sch,
 			return cl;
 	}
 
-	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	fl = rcu_dereference_bh(q->filter_list);
-	result = tc_classify(skb, fl, &res);
+	result = tc_classify_act(skb, fl, &res, qerr);
 	if (result >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-		switch (result) {
-		case TC_ACT_STOLEN:
-			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
-		case TC_ACT_SHOT:
-			return NULL;
-		}
-#endif
 		cl = (struct drr_class *)res.class;
 		if (cl == NULL)
 			cl = drr_find_class(sch, res.classid);
@@ -353,13 +344,11 @@ static int drr_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 {
 	struct drr_sched *q = qdisc_priv(sch);
 	struct drr_class *cl;
-	int err = 0;
+	int err;
 
 	cl = drr_classify(skb, sch, &err);
 	if (cl == NULL) {
-		if (err & __NET_XMIT_BYPASS)
-			qdisc_qstats_drop(sch);
-		kfree_skb(skb);
+		qdisc_drop_bypass(skb, sch, err);
 		return err;
 	}
 
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 4d00ece3243d..84094637807f 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -98,20 +98,10 @@ static unsigned int fq_codel_classify(struct sk_buff *skb, struct Qdisc *sch,
 	if (!filter)
 		return fq_codel_hash(q, skb) + 1;
 
-	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
-	result = tc_classify(skb, filter, &res);
-	if (result >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-		switch (result) {
-		case TC_ACT_STOLEN:
-			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
-		case TC_ACT_SHOT:
-			return 0;
-		}
-#endif
-		if (TC_H_MIN(res.classid) <= q->flows_cnt)
-			return TC_H_MIN(res.classid);
-	}
+	result = tc_classify_act(skb, filter, &res, qerr);
+	if (result >= 0 && TC_H_MIN(res.classid) <= q->flows_cnt)
+		return TC_H_MIN(res.classid);
+
 	return 0;
 }
 
@@ -174,13 +164,11 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	struct fq_codel_sched_data *q = qdisc_priv(sch);
 	unsigned int idx;
 	struct fq_codel_flow *flow;
-	int uninitialized_var(ret);
+	int ret;
 
 	idx = fq_codel_classify(skb, sch, &ret);
 	if (idx == 0) {
-		if (ret & __NET_XMIT_BYPASS)
-			qdisc_qstats_drop(sch);
-		kfree_skb(skb);
+		qdisc_drop_bypass(skb, sch, ret);
 		return ret;
 	}
 	idx--;
diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 9d1e43ea0ccb..7bed6b5bc500 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -717,18 +717,9 @@ static struct qfq_class *qfq_classify(struct sk_buff *skb, struct Qdisc *sch,
 			return cl;
 	}
 
-	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 	fl = rcu_dereference_bh(q->filter_list);
-	result = tc_classify(skb, fl, &res);
+	result = tc_classify_act(skb, fl, &res, qerr);
 	if (result >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-		switch (result) {
-		case TC_ACT_STOLEN:
-			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
-		case TC_ACT_SHOT:
-			return NULL;
-		}
-#endif
 		cl = (struct qfq_class *)res.class;
 		if (cl == NULL)
 			cl = qfq_find_class(sch, res.classid);
@@ -1227,9 +1218,7 @@ static int qfq_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 
 	cl = qfq_classify(skb, sch, &err);
 	if (cl == NULL) {
-		if (err & __NET_XMIT_BYPASS)
-			qdisc_qstats_drop(sch);
-		kfree_skb(skb);
+		qdisc_drop_bypass(skb, sch, err);
 		return err;
 	}
 	pr_debug("qfq_enqueue: cl = %x\n", cl->common.classid);
diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 61e18108d0d3..c36c596ba56b 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -259,16 +259,8 @@ static bool sfb_classify(struct sk_buff *skb, struct tcf_proto *fl,
 	struct tcf_result res;
 	int result;
 
-	result = tc_classify(skb, fl, &res);
+	result = tc_classify_act(skb, fl, &res, qerr);
 	if (result >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-		switch (result) {
-		case TC_ACT_STOLEN:
-			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
-		case TC_ACT_SHOT:
-			return false;
-		}
-#endif
 		*salt = TC_H_MIN(res.classid);
 		return true;
 	}
@@ -285,7 +277,7 @@ static int sfb_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	u32 p_min = ~0;
 	u32 minqlen = ~0;
 	u32 r, slot, salt, sfbhash;
-	int ret = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
+	int ret;
 	struct flow_keys keys;
 
 	if (unlikely(sch->q.qlen >= q->limit)) {
@@ -418,9 +410,7 @@ drop:
 	qdisc_drop(skb, sch);
 	return NET_XMIT_CN;
 other_drop:
-	if (ret & __NET_XMIT_BYPASS)
-		qdisc_qstats_drop(sch);
-	kfree_skb(skb);
+	qdisc_drop_bypass(skb, sch, ret);
 	return ret;
 }
 
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index b508ff655da3..7d9ad84bc8f9 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -201,20 +201,10 @@ static unsigned int sfq_classify(struct sk_buff *skb, struct Qdisc *sch,
 		return sfq_hash(q, skb) + 1;
 	}
 
-	*qerr = NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
-	result = tc_classify(skb, fl, &res);
-	if (result >= 0) {
-#ifdef CONFIG_NET_CLS_ACT
-		switch (result) {
-		case TC_ACT_STOLEN:
-			*qerr = NET_XMIT_SUCCESS | __NET_XMIT_STOLEN;
-		case TC_ACT_SHOT:
-			return 0;
-		}
-#endif
-		if (TC_H_MIN(res.classid) <= q->divisor)
-			return TC_H_MIN(res.classid);
-	}
+	result = tc_classify_act(skb, fl, &res, qerr);
+	if (result >= 0 && TC_H_MIN(res.classid) <= q->divisor)
+		return TC_H_MIN(res.classid);
+
 	return 0;
 }
 
@@ -371,15 +361,12 @@ sfq_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	unsigned int hash;
 	sfq_index x, qlen;
 	struct sfq_slot *slot;
-	int uninitialized_var(ret);
 	struct sk_buff *head;
-	int delta;
+	int delta, ret;
 
 	hash = sfq_classify(skb, sch, &ret);
 	if (hash == 0) {
-		if (ret & __NET_XMIT_BYPASS)
-			qdisc_qstats_drop(sch);
-		kfree_skb(skb);
+		qdisc_drop_bypass(skb, sch, ret);
 		return ret;
 	}
 	hash--;
-- 
1.7.9.5

Re: [RFC 1/3] tc: fix return values of ingress qdisc

[Cong Wang]

On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> ingress qdisc should return NET_XMIT_* values just like all other qdiscs.
>

XMIT already means egress...

> Since it's invoked via qdisc_enqueue_root() (which suppose to return
> only NET_XMIT_* values as well), it was working by accident,
> since TC_ACT_* values fit within NET_XMIT_MASK.
>

Why not just add a BUILD_BUG_ON() to capture this?

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Cong Wang]

On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
> Get rid of redundant checks in all qdiscs.
> Instead do it once.

The current code can be easily extended, while your code not.
I don't see the need of this change.

Re: [RFC 3/3] tc: cleanup tc_classify

[Cong Wang]

On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> introduce tc_classify_act() and qdisc_drop_bypass() helper functions to reduce
> copy-paste among different qdiscs


I don't think qdisc_drop_bypass() is more readable than without it,
maybe you need a better name, or just leave the code as it is.

tc_classify_act() seems ok.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Daniel Borkmann]

On 04/22/2015 07:02 AM, Cong Wang wrote:
> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
>> Get rid of redundant checks in all qdiscs.
>> Instead do it once.
>
> The current code can be easily extended, while your code not.

So, do you plan to extend it ?

The alias has been there since pre-git times, so more than
10 years now, where no-one felt the need to extended the
semantics of it ...

Re: [RFC 1/3] tc: fix return values of ingress qdisc

[Alexei Starovoitov]

On 4/21/15 9:59 PM, Cong Wang wrote:
> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> ingress qdisc should return NET_XMIT_* values just like all other qdiscs.
>>
>
> XMIT already means egress...

may be then it should be renamed as well.
from include/linux/netdevice.h:
/* qdisc ->enqueue() return codes. */
#define NET_XMIT_SUCCESS        0x00
...

the point is that qdisc->enqeue() must return NET_XMIT_* values.
ingress qdisc is violating this and therefore should be fixed.

>> Since it's invoked via qdisc_enqueue_root() (which suppose to return
>> only NET_XMIT_* values as well), it was working by accident,
>> since TC_ACT_* values fit within NET_XMIT_MASK.
>>
>
> Why not just add a BUILD_BUG_ON() to capture this?

ingress qdisc returning TC_ACT_* values is an obvious layering
violation. I'm puzzled why it's been this way for so long.
Adding BUILD_BUG_ON is not an option.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Alexei Starovoitov]

On 4/21/15 10:02 PM, Cong Wang wrote:
> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
>> Get rid of redundant checks in all qdiscs.
>> Instead do it once.
>
> The current code can be easily extended, while your code not.
> I don't see the need of this change.

well, iproute2 doesn't use TC_ACT_QUEUED action at all and
TC_ACT_STOLEN is used by mirred. All in-tree qdiscs alias them.
If you're saying that some future actions together with
some future qdiscs may take advantage of that, then why they didn't
use it over the last 10 years?
Having both that do the same thing is only confusing.
I think having one value to indicate 'stolen' condition makes TC
code easier to understand.
Jamal, what's your take on this?

Re: [RFC 3/3] tc: cleanup tc_classify

[Alexei Starovoitov]

On 4/21/15 10:05 PM, Cong Wang wrote:
> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> introduce tc_classify_act() and qdisc_drop_bypass() helper functions to reduce
>> copy-paste among different qdiscs
>
>
> I don't think qdisc_drop_bypass() is more readable than without it,
> maybe you need a better name, or just leave the code as it is.

what would be a better name? I'm open to suggestions.

We already have qdisc_drop() that does:
         kfree_skb(skb);
         qdisc_qstats_drop(sch);

my proposed qdisc_drop_bypass() does stats math conditionally:
         if (err & __NET_XMIT_BYPASS)
                 qdisc_qstats_drop(sch);
         kfree_skb(skb);

So together I think they fit nicely. With this helper the sch_choke.c
looks like:
congestion_drop:
         qdisc_drop(skb, sch);
         return NET_XMIT_CN;
other_drop:
         qdisc_drop_bypass(skb, sch, ret);
         return ret;

and in the next set of cleanups I'm planning to combine these two
helpers into one, but I need to cleanup __NET_XMIT_BYPASS flag first.

Re: [RFC 1/3] tc: fix return values of ingress qdisc

[Cong Wang]

On Wed, Apr 22, 2015 at 3:04 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On 4/21/15 9:59 PM, Cong Wang wrote:
>>
>> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com>
>> wrote:
>>>
>>> ingress qdisc should return NET_XMIT_* values just like all other qdiscs.
>>>
>>
>> XMIT already means egress...
>
>
> may be then it should be renamed as well.
> from include/linux/netdevice.h:
> /* qdisc ->enqueue() return codes. */
> #define NET_XMIT_SUCCESS        0x00
> ...
>
> the point is that qdisc->enqeue() must return NET_XMIT_* values.
> ingress qdisc is violating this and therefore should be fixed.

XMIT is non-sense for ingress, you really need to pick another
name for it if TC_ACT_OK isn't okay for you (it is okay for me).


>
>>> Since it's invoked via qdisc_enqueue_root() (which suppose to return
>>> only NET_XMIT_* values as well), it was working by accident,
>>> since TC_ACT_* values fit within NET_XMIT_MASK.
>>>
>>
>> Why not just add a BUILD_BUG_ON() to capture this?
>
>
> ingress qdisc returning TC_ACT_* values is an obvious layering
> violation. I'm puzzled why it's been this way for so long.

Why? Everyone knows ingress is the only qdisc on ingress
and it has no queue and can only accept filters (actions are on filters).

> Adding BUILD_BUG_ON is not an option.
>

It at least tells people we know their value are same, IOW, not a bug.

I don't see the need for the change except a BUILD_BUG_ON().

Re: [RFC 3/3] tc: cleanup tc_classify

[Cong Wang]

On Wed, Apr 22, 2015 at 3:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On 4/21/15 10:05 PM, Cong Wang wrote:
>>
>> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com>
>> wrote:
>>>
>>> introduce tc_classify_act() and qdisc_drop_bypass() helper functions to
>>> reduce
>>> copy-paste among different qdiscs
>>
>>
>>
>> I don't think qdisc_drop_bypass() is more readable than without it,
>> maybe you need a better name, or just leave the code as it is.
>
>
> what would be a better name? I'm open to suggestions.

My reading for "qdisc_drop_bypass()" is it bypasses packet
dropping for some case, apparently doesn't match its definition.

I can't think out a better name therefore I don't think it deserves
a function, just leave as it is.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Cong Wang]

On Wed, Apr 22, 2015 at 3:22 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On 4/21/15 10:02 PM, Cong Wang wrote:
>>
>> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com>
>> wrote:
>>>
>>> TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
>>> Get rid of redundant checks in all qdiscs.
>>> Instead do it once.
>>
>>
>> The current code can be easily extended, while your code not.
>> I don't see the need of this change.
>
>
> well, iproute2 doesn't use TC_ACT_QUEUED action at all and
> TC_ACT_STOLEN is used by mirred. All in-tree qdiscs alias them.
> If you're saying that some future actions together with
> some future qdiscs may take advantage of that, then why they didn't
> use it over the last 10 years?
> Having both that do the same thing is only confusing.
> I think having one value to indicate 'stolen' condition makes TC
> code easier to understand.

Then remove it, I am all for this. ;)

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Alexei Starovoitov]

On 4/22/15 4:39 PM, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 3:22 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On 4/21/15 10:02 PM, Cong Wang wrote:
>>>
>>> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com>
>>> wrote:
>>>>
>>>> TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
>>>> Get rid of redundant checks in all qdiscs.
>>>> Instead do it once.
>>>
>>>
>>> The current code can be easily extended, while your code not.
>>> I don't see the need of this change.
>>
>>
>> well, iproute2 doesn't use TC_ACT_QUEUED action at all and
>> TC_ACT_STOLEN is used by mirred. All in-tree qdiscs alias them.
>> If you're saying that some future actions together with
>> some future qdiscs may take advantage of that, then why they didn't
>> use it over the last 10 years?
>> Having both that do the same thing is only confusing.
>> I think having one value to indicate 'stolen' condition makes TC
>> code easier to understand.
>
> Then remove it, I am all for this. ;)

TC_ACT_QUEUED cannot be removed.
Only deprecated with backwards compatibility the way this patch did it.
That should have been obvious.

The other two threads degenerated into non-technical comments.

Anyway, this set was RFC to answer my main question whether I should
continue with tc cleanup or stop right here. I got my answer.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Daniel Borkmann]

On 04/23/2015 04:46 AM, Alexei Starovoitov wrote:
...
> The other two threads degenerated into non-technical comments.

Yep. :-/

> Anyway, this set was RFC to answer my main question whether I should
> continue with tc cleanup or stop right here. I got my answer.

I think it's worth proceeding with this, it's long overdue.

Re: [RFC 1/3] tc: fix return values of ingress qdisc

[Thomas Graf]

On 04/22/15 at 04:29pm, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 3:04 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> > On 4/21/15 9:59 PM, Cong Wang wrote:
> >>
> >> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com>
> >> wrote:
> >>>
> >>> ingress qdisc should return NET_XMIT_* values just like all other qdiscs.
> >>>
> >>
> >> XMIT already means egress...
> >
> >
> > may be then it should be renamed as well.
> > from include/linux/netdevice.h:
> > /* qdisc ->enqueue() return codes. */
> > #define NET_XMIT_SUCCESS        0x00
> > ...
> >
> > the point is that qdisc->enqeue() must return NET_XMIT_* values.
> > ingress qdisc is violating this and therefore should be fixed.
> 
> XMIT is non-sense for ingress, you really need to pick another
> name for it if TC_ACT_OK isn't okay for you (it is okay for me).

You transmit into a qdisc. If that terminology doesn't suit
you then rename it to NET_QUEUE_* but moving away from returning
TC_ACT_* is definitely the right thing to do here.
> 

Re: [RFC 3/3] tc: cleanup tc_classify

[Thomas Graf]

On 04/22/15 at 04:38pm, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 3:27 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> > On 4/21/15 10:05 PM, Cong Wang wrote:
> >>
> >> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov <ast@plumgrid.com>
> >> wrote:
> >>>
> >>> introduce tc_classify_act() and qdisc_drop_bypass() helper functions to
> >>> reduce
> >>> copy-paste among different qdiscs

I like this cleanup. It aligns all skb dropping in qdiscs to a
qdisc_drop*() function.

> >> I don't think qdisc_drop_bypass() is more readable than without it,
> >> maybe you need a better name, or just leave the code as it is.
> >
> >
> > what would be a better name? I'm open to suggestions.
> 
> My reading for "qdisc_drop_bypass()" is it bypasses packet
> dropping for some case, apparently doesn't match its definition.
> 
> I can't think out a better name therefore I don't think it deserves
> a function, just leave as it is.

Interesting logic ;-)

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Cong Wang]

On Wed, Apr 22, 2015 at 7:46 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>
> TC_ACT_QUEUED cannot be removed.
> Only deprecated with backwards compatibility the way this patch did it.
> That should have been obvious.
>

It is at least the third time I have to repeat that: we really don't care
about out-of-tree modules.

Everyone MUST read Documentation/stable_api_nonsense.txt.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Daniel Borkmann]

On 04/23/2015 08:12 PM, Cong Wang wrote:
> On Wed, Apr 22, 2015 at 7:46 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
...
>> TC_ACT_QUEUED cannot be removed.
>> Only deprecated with backwards compatibility the way this patch did it.
>> That should have been obvious.
>
> It is at least the third time I have to repeat that: we really don't care
> about out-of-tree modules.
>
> Everyone MUST read Documentation/stable_api_nonsense.txt.

Seriously, what are you talking about ???

$ git grep -n TC_ACT_QUEUED include/uapi/
include/uapi/linux/pkt_cls.h:105:#define TC_ACT_QUEUED          5

This is *UAPI*, no-one is even remotely talking about out-of-tree
kernel modules.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Cong Wang]

On Thu, Apr 23, 2015 at 11:21 AM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 04/23/2015 08:12 PM, Cong Wang wrote:
>>
>> On Wed, Apr 22, 2015 at 7:46 PM, Alexei Starovoitov <ast@plumgrid.com>
>> wrote:
>
> ...
>>>
>>> TC_ACT_QUEUED cannot be removed.
>>> Only deprecated with backwards compatibility the way this patch did it.
>>> That should have been obvious.
>>
>>
>> It is at least the third time I have to repeat that: we really don't care
>> about out-of-tree modules.
>>
>> Everyone MUST read Documentation/stable_api_nonsense.txt.
>
>
> Seriously, what are you talking about ???
>
> $ git grep -n TC_ACT_QUEUED include/uapi/
> include/uapi/linux/pkt_cls.h:105:#define TC_ACT_QUEUED          5
>
> This is *UAPI*, no-one is even remotely talking about out-of-tree
> kernel modules.

I am not stupid. You should figure out we can just leave its definition
by removing it, leaving the default as stolen.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Jamal Hadi Salim]

On 04/22/15 18:22, Alexei Starovoitov wrote:
> On 4/21/15 10:02 PM, Cong Wang wrote:
>> On Tue, Apr 21, 2015 at 12:27 PM, Alexei Starovoitov
>> <ast@plumgrid.com> wrote:
>>> TC_ACT_QUEUED was always an alias of TC_ACT_STOLEN.
>>> Get rid of redundant checks in all qdiscs.
>>> Instead do it once.
>>
>> The current code can be easily extended, while your code not.
>> I don't see the need of this change.
>
> well, iproute2 doesn't use TC_ACT_QUEUED action at all and
> TC_ACT_STOLEN is used by mirred. All in-tree qdiscs alias them.
> If you're saying that some future actions together with
> some future qdiscs may take advantage of that, then why they didn't
> use it over the last 10 years?
> Having both that do the same thing is only confusing.
> I think having one value to indicate 'stolen' condition makes TC
> code easier to understand.
> Jamal, what's your take on this?


Sorry - I am on travel so not very helpful/responsive.
Several things:

1) the _XMIT semantics are useful on the egress side because in fact
we do have queues and they can be attached to qdiscs etc.
The TC_ACT_XXX codes were _intentional_ since ingress works as a
classifier shell. The fact that qdiscs dealt with these codes directly
allows for specialized handling. Moving them to a generic function
seems to defeat that purpose. So I am siding with Cong on this.
You probably have some other motivation for doing this which is not 
clear yet? Are you planning to queue things on ingress?

2) the ACT_QUEUED vs STOLEN was supposed to have semantics of something
that was stolen (eg redirection should definetely have been returning
STOLEN not QUEUED); something that queues for later re-injection
(with any/all metadata) was intended to use QUEUED. I believe netfilter
may have  followed suit and introduced similar codes (so it would be
interesting to see how they use them). In any case, it is clear it is
not being used - but i want to point the reason it was there
(which may still be useful; problem is TheLinuxWay is cutnpaste where
someone repeats the code that exists).

cheers,
jamal

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Florian Westphal]

Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> 2) the ACT_QUEUED vs STOLEN was supposed to have semantics of something
> that was stolen (eg redirection should definetely have been returning
> STOLEN not QUEUED); something that queues for later re-injection
> (with any/all metadata) was intended to use QUEUED. I believe netfilter
> may have  followed suit and introduced similar codes (so it would be
> interesting to see how they use them).

Hooks (Targets) don't queue themselves, i.e. NF_QUEUE tells the
netfilter core that the skb is to be handed off to nf_queue machinery,
while NF_STOLEN is the more obvious "don't touch this skb ever again".

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Alexei Starovoitov]

On 4/23/15 1:45 PM, Jamal Hadi Salim wrote:
>
> 1) the _XMIT semantics are useful on the egress side because in fact
> we do have queues and they can be attached to qdiscs etc.
> The TC_ACT_XXX codes were _intentional_ since ingress works as a
> classifier shell.

then it is worse mess than I thought :(
Why call it _qdisc_ then? and have special and convoluted handling for
it in qdisc_create, qdisc_graft and other places?

 > Are you planning to queue things on ingress?

I thought that was the whole purpose of ingress qdisc.
why then we have dev->ingress_queue?

If queueing was never a goal, may be we should kill ingress qdisc
and replace it with a simple shim that only does cls/act.
The code overall will get much simpler and faster.

Feels like falling into rabbit hole.

> The fact that qdiscs dealt with these codes directly
> allows for specialized handling. Moving them to a generic function
> seems to defeat that purpose. So I am siding with Cong on this.

that's not what patch 1 is doing. It is still doing specialized
handling... but in light of what you said above, it looks like much
bigger cleanup is needed. We'll continue arguing when I refactor
this set and resubmit when net-next reopens.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Cong Wang]

On Thu, Apr 23, 2015 at 3:13 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On 4/23/15 1:45 PM, Jamal Hadi Salim wrote:
>>
>>
>> 1) the _XMIT semantics are useful on the egress side because in fact
>> we do have queues and they can be attached to qdiscs etc.
>> The TC_ACT_XXX codes were _intentional_ since ingress works as a
>> classifier shell.
>
>
> then it is worse mess than I thought :(
> Why call it _qdisc_ then? and have special and convoluted handling for
> it in qdisc_create, qdisc_graft and other places?

It needs to glue into qdisc layer, unify the abstractions.

>
>> Are you planning to queue things on ingress?
>
> I thought that was the whole purpose of ingress qdisc.

There is no queue for ingress qdisc.

> why then we have dev->ingress_queue?

Glue layer, qdisc ties too much with txq, which is what I want to solve.

We have struct netdev_rx_queue too, but look at it, there is nothing
but some sysfs stuffs needed by RPS.

>
> If queueing was never a goal, may be we should kill ingress qdisc
> and replace it with a simple shim that only does cls/act.
> The code overall will get much simpler and faster.
>

As I said in response to your patch for skb->data, we need to align
ingress with egress, rather than making more differences, this includes
qdisc's too. We do have per-cpu queues for ingress, at least in theory
we have a queue for ingress to use.

Of course, RX is naturally different from TX, which is the root cause
why we have so many differences here.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Jamal Hadi Salim]

On 04/23/15 18:13, Alexei Starovoitov wrote:
> On 4/23/15 1:45 PM, Jamal Hadi Salim wrote:

> then it is worse mess than I thought :(
> Why call it _qdisc_ then? and have special and convoluted handling for
> it in qdisc_create, qdisc_graft and other places?

Convinience for tooling and using existing abstractions is the
historical. You can attach qdiscs to netdevs.
You can then use all sorts of well understood tools.

>  > Are you planning to queue things on ingress?
>
> I thought that was the whole purpose of ingress qdisc.
> why then we have dev->ingress_queue?
>

So you are planning to add queues? If you are that is a different
discussion (and the use case needs some clarity).

> If queueing was never a goal, may be we should kill ingress qdisc
> and replace it with a simple shim that only does cls/act.
> The code overall will get much simpler and faster.
>

Attaching to the device was considered "simpler" based on the tooling
thought process. On whether we should queue on ingress - it was not
considered useful if the packets were going to the host i.e we want to
proceed to completion likely queueing to some socket layer further
upstream.
For packets being forwarded we already had egress qdiscs which had
queues so it didnt seem to make sense to enqueue on ingress for such
use cases.
There was a use case later where multiple ingress ports had to be shared
and ifb was born - which is pseudo temporary enqueueing on ingress.


> Feels like falling into rabbit hole.
>
>> The fact that qdiscs dealt with these codes directly
>> allows for specialized handling. Moving them to a generic function
>> seems to defeat that purpose. So I am siding with Cong on this.
>
> that's not what patch 1 is doing. It is still doing specialized
> handling... but in light of what you said above, it looks like much
> bigger cleanup is needed. We'll continue arguing when I refactor
> this set and resubmit when net-next reopens.
>

Sorry - i was refereing to the patch where you agregated things after
the qdisc invokes a classifier.

cheers,
jamal

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Alexei Starovoitov]

On 4/23/15 3:51 PM, Jamal Hadi Salim wrote:
>
> So you are planning to add queues? If you are that is a different
> discussion (and the use case needs some clarity).

nope. I wasn't planning to do that.

 > For packets being forwarded we already had egress qdiscs which had
 > queues so it didnt seem to make sense to enqueue on ingress for such
 > use cases.
 > There was a use case later where multiple ingress ports had to be
 > shared
 > and ifb was born - which is pseudo temporary enqueueing on ingress.

agree. imo ifb approach is more flexible, since it has full hierarchy
of qdiscs. As you're saying above and from the old ifb logs I thought
that ifb is _temporary_ and long term plan is to use ingress_queue,
but looks like this is not the case. Also not too long ago we decided
that we don't want another ingress qdisc. Therefore I think it makes
sense to simplify the code and boost performance.
I'm not proposing to change tooling, of course.
 From iproute2 point of view we'll still have ingress qdisc.
Right now we're pointlessly allocating memory for it and for
ingress_queue, whereas we only need to call cls/act.
I'm proposing to kill them and used tcf_proto in net_device instead.
Right now to reach cls in critical path on ingress we do:
rxq = skb->dev->ingress_queue
sch = rxq->qdisc
sch->enqueue
sch->filter_list
with a bunch of 'if' conditions and useless memory accesses in-between.
If we add 'ingress_filter_list' directly to net_device,
it will be just:
skb->dev->ingress_filter_list
which is huge performance boost. Code size will shrink as well.
iproute2 and all existing tools will work as-is. Looks as win-win to me.

>>> The fact that qdiscs dealt with these codes directly
>>> allows for specialized handling. Moving them to a generic function
>>> seems to defeat that purpose. So I am siding with Cong on this.
>>
>> that's not what patch 1 is doing. It is still doing specialized
>> handling... but in light of what you said above, it looks like much
>> bigger cleanup is needed. We'll continue arguing when I refactor
>> this set and resubmit when net-next reopens.
>
> Sorry - i was refereing to the patch where you agregated things after
> the qdisc invokes a classifier.

hmm. There I also didn't convert all qdiscs into single helper.
Only those that have exactly the same logic after tc_classify.
All other qdiscs have custom handling.
No worries, it's hard to review things while traveling. Been there :)

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Cong Wang]

On Thu, Apr 23, 2015 at 5:59 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On 4/23/15 3:51 PM, Jamal Hadi Salim wrote:
>>
>>
>> So you are planning to add queues? If you are that is a different
>> discussion (and the use case needs some clarity).
>
>
> nope. I wasn't planning to do that.
>
>> For packets being forwarded we already had egress qdiscs which had
>> queues so it didnt seem to make sense to enqueue on ingress for such
>> use cases.
>> There was a use case later where multiple ingress ports had to be
>> shared
>> and ifb was born - which is pseudo temporary enqueueing on ingress.
>
> agree. imo ifb approach is more flexible, since it has full hierarchy
> of qdiscs. As you're saying above and from the old ifb logs I thought
> that ifb is _temporary_ and long term plan is to use ingress_queue,
> but looks like this is not the case. Also not too long ago we decided
> that we don't want another ingress qdisc. Therefore I think it makes
> sense to simplify the code and boost performance.

Which performance problem did you see? Did you hit the spinlock
bottleneck? That spinlock should be eliminated before you
try to do more, and it can be replaced by RCU read lock after
John's work (I am still waiting for his patch btw).


> I'm not proposing to change tooling, of course.
> From iproute2 point of view we'll still have ingress qdisc.
> Right now we're pointlessly allocating memory for it and for
> ingress_queue, whereas we only need to call cls/act.
> I'm proposing to kill them and used tcf_proto in net_device instead.
> Right now to reach cls in critical path on ingress we do:
> rxq = skb->dev->ingress_queue
> sch = rxq->qdisc
> sch->enqueue
> sch->filter_list
> with a bunch of 'if' conditions and useless memory accesses in-between.
> If we add 'ingress_filter_list' directly to net_device,
> it will be just:
> skb->dev->ingress_filter_list
> which is huge performance boost. Code size will shrink as well.
> iproute2 and all existing tools will work as-is. Looks as win-win to me.

Nope, it breaks Qdisc abstraction, filters never attach to netdevice
directly. You should stop, since you really don't understand the code.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Daniel Borkmann]

On 04/24/2015 05:37 AM, Cong Wang wrote:
> On Thu, Apr 23, 2015 at 5:59 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
>> On 4/23/15 3:51 PM, Jamal Hadi Salim wrote:
...
>> agree. imo ifb approach is more flexible, since it has full hierarchy
>> of qdiscs. As you're saying above and from the old ifb logs I thought
>> that ifb is _temporary_ and long term plan is to use ingress_queue,
>> but looks like this is not the case. Also not too long ago we decided
>> that we don't want another ingress qdisc. Therefore I think it makes
>> sense to simplify the code and boost performance.
>
> Which performance problem did you see? Did you hit the spinlock
> bottleneck? That spinlock should be eliminated before you
> try to do more  [...]

Those are already the next steps for improving performance after
that, by attempting to get rid of these additional indirections
and shrinking code.

>> I'm not proposing to change tooling, of course.
>>  From iproute2 point of view we'll still have ingress qdisc.
>> Right now we're pointlessly allocating memory for it and for
>> ingress_queue, whereas we only need to call cls/act.
>> I'm proposing to kill them and used tcf_proto in net_device instead.
>> Right now to reach cls in critical path on ingress we do:
>> rxq = skb->dev->ingress_queue
>> sch = rxq->qdisc
>> sch->enqueue
>> sch->filter_list
>> with a bunch of 'if' conditions and useless memory accesses in-between.
>> If we add 'ingress_filter_list' directly to net_device,
>> it will be just:
>> skb->dev->ingress_filter_list
>> which is huge performance boost. Code size will shrink as well.
>> iproute2 and all existing tools will work as-is. Looks as win-win to me.
>
> Nope, it breaks Qdisc abstraction, filters never attach to netdevice
> directly. You should stop, since you really don't understand the code.

Would be great if you make netdev a better place, stop such immature
comments, and keep it technical as everyone else.

The point here is to try thinking outside the box, so lets see how
the result in the end looks like, then we can discuss.

Re: [RFC 2/3] tc: deprecate TC_ACT_QUEUED

[Jamal Hadi Salim]

On 04/23/15 20:59, Alexei Starovoitov wrote:
> On 4/23/15 3:51 PM, Jamal Hadi Salim wrote:
>>
>> So you are planning to add queues? If you are that is a different
>> discussion (and the use case needs some clarity).
>
> nope. I wasn't planning to do that.
>

Then i would say, lets just keep the naming convention. Maybe
better documentation is needed.

>  > For packets being forwarded we already had egress qdiscs which had
>  > queues so it didnt seem to make sense to enqueue on ingress for such
>  > use cases.
>  > There was a use case later where multiple ingress ports had to be
>  > shared
>  > and ifb was born - which is pseudo temporary enqueueing on ingress.
>
> agree. imo ifb approach is more flexible, since it has full hierarchy
> of qdiscs. As you're saying above and from the old ifb logs I thought
> that ifb is _temporary_ and long term plan is to use ingress_queue,
> but looks like this is not the case.

ifb represented a real use case which questioned the lack
of ingress queue. But do note, that problem was solved. And in
engineering we just move on unless a compelling reason makes us
rethink.
IMO, the original use case for ifb no longer requires it but the
internets and the googles havent caught up with it yet. Refer to
my netdev01 preso/paper where i talk about "sharing".
However, ifb addressed another issue. Instead of having per port/netdev
policies we can now have per-aggregate-port-group policies. While this
comes at a small cost of redirecting packets, that penalty is only paid
for by people interested in defining such policies.
This in itself is useful.

>Also not too long ago we decided
> that we don't want another ingress qdisc. Therefore I think it makes
> sense to simplify the code and boost performance.
> I'm not proposing to change tooling, of course.
>  From iproute2 point of view we'll still have ingress qdisc.
> Right now we're pointlessly allocating memory for it and for
> ingress_queue, whereas we only need to call cls/act.
> I'm proposing to kill them and used tcf_proto in net_device instead.
> Right now to reach cls in critical path on ingress we do:
> rxq = skb->dev->ingress_queue
> sch = rxq->qdisc
> sch->enqueue
> sch->filter_list
> with a bunch of 'if' conditions and useless memory accesses in-between.
> If we add 'ingress_filter_list' directly to net_device,
> it will be just:
> skb->dev->ingress_filter_list
> which is huge performance boost. Code size will shrink as well.
> iproute2 and all existing tools will work as-is. Looks as win-win to me.
>

I hear you; any extra cycle we can remove from the equation is useful.
But do note: in this case, it comes down to the thin line between
usability and  performance. Do take a closer look at the tooling
interfaces from user space on ingress qdisc. The serialization,
the ops already provided for manipulating filters etc. Those are
for free if you use the qdisc abstraction.
If you can still keep that and get rid of the opcodes you mention
above then it is win-win. My feeling is it is a lot more challenging.

>>>> The fact that qdiscs dealt with these codes directly
>>>> allows for specialized handling. Moving them to a generic function
>>>> seems to defeat that purpose. So I am siding with Cong on this.
>>>
>>> that's not what patch 1 is doing. It is still doing specialized
>>> handling... but in light of what you said above, it looks like much
>>> bigger cleanup is needed. We'll continue arguing when I refactor
>>> this set and resubmit when net-next reopens.
>>
>> Sorry - i was refereing to the patch where you agregated things after
>> the qdisc invokes a classifier.
>
> hmm. There I also didn't convert all qdiscs into single helper.
> Only those that have exactly the same logic after tc_classify.
> All other qdiscs have custom handling.
> No worries, it's hard to review things while traveling. Been there :)
>

I am sorry again - will look when i get out of travel mode.

cheers,
jamal