[PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag

[PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag

[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.

Thanks,
Jarek P.

----------------->

Subject: net_sched: Add qdisc __NET_XMIT_STOLEN flag

Patrick McHardy <kaber@trash.net> 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 <davem@davemloft.net> 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.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 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(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index b4d056c..188e4e4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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 */
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index b5f40d7..f139009 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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,
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 6b517b9..27dd773 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -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++;
 	}
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 14954bf..765ae56 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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;
 	}
 
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index a935676..7170275 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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;
 	}
 
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 0ae7d19..5cf9ae7 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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;
 	}
 
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 75a4095..538d79b 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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);
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index a590857..6cd6f2b 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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;
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index f849243..adb1a52 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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;
 }
 
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 3f2d1d7..5da0583 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -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++;
 	}
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 8589da6..3a456e1 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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;
 		}
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index b296672..7d3b7ff 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -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;
 	}
 

Re: [PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag

[Patrick McHardy]

Jarek Poplawski wrote:
> 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:

> +#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;

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.

Re: [PATCH] net_sched: Add qdisc __NET_XMIT_STOLEN flag

[Jarek Poplawski]

On Fri, Aug 01, 2008 at 08:15:50AM +0200, Patrick McHardy wrote:
> Jarek Poplawski wrote:
>> 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:
>
>> +#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;
>
> 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.

OK, I think, I'll prepare a separate patch for this.

Thanks,
Jarek P.

[PATCH] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jarek Poplawski]

Here 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 <kaber@trash.net> 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().


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 (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(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f139009..e41629f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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
diff --git a/net/core/dev.c b/net/core/dev.c
index 69320a5..e670504 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1805,7 +1805,6 @@ gso:
 
 		spin_unlock(root_lock);
 
-		rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
 		goto out;
 	}
 
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 27dd773..43d3725 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -457,7 +457,7 @@ drop: __maybe_unused
 		return 0;
 	}
 	tasklet_schedule(&p->task);
-	return NET_XMIT_BYPASS;
+	return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 }
 
 /*
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 765ae56..4e261ce 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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;
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 7170275..edd1298 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 5cf9ae7..c2b8d9c 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 538d79b..be35422 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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;
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6cd6f2b..fb0294d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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);
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index adb1a52..eac1976 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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;
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 3a456e1..6e041d1 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Fri, 1 Aug 2008 10:19:29 +0000

> @@ -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 take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jarek Poplawski]

On Sun, Aug 03, 2008 at 06:25:24PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Fri, 1 Aug 2008 10:19:29 +0000
> 
> > @@ -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.

Thanks for spotting this!

> 
> 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 <kaber@trash.net> 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 <davem@davemloft.net> spotted a serious bug in the first
version of this patch.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 (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(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f139009..b2a1e8f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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
diff --git a/net/core/dev.c b/net/core/dev.c
index 69320a5..e670504 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1805,7 +1805,6 @@ gso:
 
 		spin_unlock(root_lock);
 
-		rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
 		goto out;
 	}
 
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 27dd773..43d3725 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -457,7 +457,7 @@ drop: __maybe_unused
 		return 0;
 	}
 	tasklet_schedule(&p->task);
-	return NET_XMIT_BYPASS;
+	return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 }
 
 /*
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 765ae56..4e261ce 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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;
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 7170275..edd1298 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 5cf9ae7..c2b8d9c 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 538d79b..be35422 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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;
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6cd6f2b..fb0294d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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);
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index adb1a52..eac1976 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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;
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 3a456e1..6e041d1 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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 take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 4 Aug 2008 06:28:13 +0000

> 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.

I understand.

But be aware that all of these things are related :-)

> Subject: net_sched: Add qdisc __NET_XMIT_BYPASS flag
> 
> Patrick McHardy <kaber@trash.net> 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 <davem@davemloft.net> spotted a serious bug in the first
> version of this patch.
> 
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
 ...
> @@ -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.

[PATCH take 3] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jarek Poplawski]

OOPS!!! Forget take!

Sorry,
Jarek P.

------------------> (take 3)

Subject: net_sched: Add qdisc __NET_XMIT_BYPASS flag

Patrick McHardy <kaber@trash.net> 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 <davem@davemloft.net> spotted a serious bug in the first
version of this patch.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 (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(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f139009..b2a1e8f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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
diff --git a/net/core/dev.c b/net/core/dev.c
index 69320a5..e670504 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1805,7 +1805,6 @@ gso:
 
 		spin_unlock(root_lock);
 
-		rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
 		goto out;
 	}
 
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 27dd773..43d3725 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -457,7 +457,7 @@ drop: __maybe_unused
 		return 0;
 	}
 	tasklet_schedule(&p->task);
-	return NET_XMIT_BYPASS;
+	return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 }
 
 /*
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 765ae56..4e261ce 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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;
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 7170275..edd1298 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 5cf9ae7..c2b8d9c 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 538d79b..be35422 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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;
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6cd6f2b..fb0294d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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);
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index adb1a52..eac1976 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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;
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 3a456e1..6e041d1 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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 take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jarek Poplawski]

On Sun, Aug 03, 2008 at 11:42:00PM -0700, David Miller wrote:
...
> > -#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.

OK, I'll soon respin this all!

Jarek P.

[PATCH 1/2 respin] net_sched: Add qdisc __NET_XMIT_STOLEN flag

[Jarek Poplawski]

On Sun, Aug 03, 2008 at 11:42:00PM -0700, David Miller wrote:
...
> >  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.

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 <kaber@trash.net> 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 <davem@davemloft.net> 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.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 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(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index ee583f6..abbf5d5 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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 */
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c5bb130..f15b045 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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,
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 6b517b9..27dd773 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -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++;
 	}
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 14954bf..765ae56 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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;
 	}
 
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index a935676..7170275 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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;
 	}
 
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 0ae7d19..5cf9ae7 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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;
 	}
 
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 75a4095..538d79b 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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);
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index a590857..6cd6f2b 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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;
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index f849243..adb1a52 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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;
 }
 
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 3f2d1d7..5da0583 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -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++;
 	}
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 8589da6..3a456e1 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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;
 		}
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index b296672..7d3b7ff 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -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;
 	}
 

[PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jarek Poplawski]

Subject: net_sched: Add qdisc __NET_XMIT_BYPASS flag

Patrick McHardy <kaber@trash.net> 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 <davem@davemloft.net> spotted a serious bug in the first
version of this patch.


Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

---

 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(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index f15b045..a7abfda 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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
diff --git a/net/core/dev.c b/net/core/dev.c
index cbf8009..6b93821 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1805,7 +1805,6 @@ gso:
 
 		spin_unlock(root_lock);
 
-		rc = rc == NET_XMIT_BYPASS ? NET_XMIT_SUCCESS : rc;
 		goto out;
 	}
 
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 27dd773..43d3725 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -457,7 +457,7 @@ drop: __maybe_unused
 		return 0;
 	}
 	tasklet_schedule(&p->task);
-	return NET_XMIT_BYPASS;
+	return NET_XMIT_SUCCESS | __NET_XMIT_BYPASS;
 }
 
 /*
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 765ae56..4e261ce 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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;
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 7170275..edd1298 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 5cf9ae7..c2b8d9c 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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;
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 538d79b..be35422 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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;
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 6cd6f2b..fb0294d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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);
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index adb1a52..eac1976 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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;
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 3a456e1..6e041d1 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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 take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jussi Kivilinna]

Quoting "Jarek Poplawski" <jarkao2@gmail.com>:

>
> 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.
>

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

[Jarek Poplawski]

On Mon, Aug 04, 2008 at 09:35:35PM +0300, Jussi Kivilinna wrote:
> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
>
>>
>> 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.
>>
>
> 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 1/2 respin] net_sched: Add qdisc __NET_XMIT_STOLEN flag

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 4 Aug 2008 08:55:09 +0000

> net_sched: Add qdisc __NET_XMIT_STOLEN flag

Applied, thanks Jarek.

Re: [PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Mon, 4 Aug 2008 08:57:34 +0000

> net_sched: Add qdisc __NET_XMIT_BYPASS flag
> 
> Patrick McHardy <kaber@trash.net> 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 <davem@davemloft.net> spotted a serious bug in the first
> version of this patch.
> 
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>

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 <davem@davemloft.net>
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.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index abbf5d5..488c56e 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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 */
diff --git a/include/net/dst.h b/include/net/dst.h
index c5c318a..8a8b71e 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Tue, 5 Aug 2008 06:34:23 +0000

> 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?

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

[Jarek Poplawski]

On Mon, Aug 04, 2008 at 11:14:49PM -0700, David Miller wrote:
...
> 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 <davem@davemloft.net>
> 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.
>     
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index abbf5d5..488c56e 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -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 */
> diff --git a/include/net/dst.h b/include/net/dst.h
> index c5c318a..8a8b71e 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -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);
>  }

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.

Re: [PATCH 2/2 respin] net_sched: Add qdisc __NET_XMIT_BYPASS flag

[Jarek Poplawski]

On Mon, Aug 04, 2008 at 11:31:22PM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Tue, 5 Aug 2008 06:34:23 +0000
> 
> > 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?
> 
> 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.

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

[Jussi Kivilinna]

Quoting "Jarek Poplawski" <jarkao2@gmail.com>:

> On Mon, Aug 04, 2008 at 09:35:35PM +0300, Jussi Kivilinna wrote:
>> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
>>
>> 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.

---

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c5bb130..9cf06fc 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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

[Jarek Poplawski]

On Tue, Aug 05, 2008 at 03:43:50PM +0300, Jussi Kivilinna wrote:
> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
>
>> On Mon, Aug 04, 2008 at 09:35:35PM +0300, Jussi Kivilinna wrote:
>>> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
>>>
>>> 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.

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.

>
> ---
>
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index c5bb130..9cf06fc 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -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

[David Miller]

From: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
Date: Tue, 05 Aug 2008 15:43:50 +0300

> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
> 
> > On Mon, Aug 04, 2008 at 09:35:35PM +0300, Jussi Kivilinna wrote:
> >> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
> >>
> >> 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.

That's two extra atomic operations on every transmitted packet,
no thanks :-)

qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb (Was: Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag)

[Jussi Kivilinna]

Quoting "Jarek Poplawski" <jarkao2@gmail.com>:

>>
>> 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:
---
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index a7abfda..ca083c6 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -175,6 +175,7 @@ struct tcf_proto

  struct 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,
diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c
index 507fb48..13230bd 100644
--- a/net/sched/sch_blackhole.c
+++ b/net/sched/sch_blackhole.c
@@ -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)

[Jarek Poplawski]

On Wed, Aug 06, 2008 at 10:42:48PM +0300, Jussi Kivilinna wrote:
...
> 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).

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.

> This could be fixed by delaying kfree_skb to exit on qdisc_enqueue_root,
> here's (completely untested) patch:
> ---
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index a7abfda..ca083c6 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -175,6 +175,7 @@ struct tcf_proto
>
>  struct 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,
> diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c
> index 507fb48..13230bd 100644
> --- a/net/sched/sch_blackhole.c
> +++ b/net/sched/sch_blackhole.c
> @@ -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).

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.

> 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.

I guess some wiser guys should decide how serious problem it is.

>
>  - 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?
>

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.

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[David Miller]

I still haven't seen anyone suggest creating a __NET_XMIT_KFREE_SKB to
fix this most rediculious problem.

I'm still waiting...

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)

> 
> I still haven't seen anyone suggest creating a __NET_XMIT_KFREE_SKB to
> 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.

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index a7abfda..4bfee1b 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 43d3725..696b4ee 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -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)
diff --git a/net/sched/sch_blackhole.c b/net/sched/sch_blackhole.c
index 507fb48..7018675 100644
--- a/net/sched/sch_blackhole.c
+++ b/net/sched/sch_blackhole.c
@@ -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)
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index 4e261ce..27fc053 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -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
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index edd1298..fbc318b 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -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)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 7cf83b3..753ee7f 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -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)
diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index c1ad6b8..70c2f58 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c
@@ -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)
diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index c2b8d9c..9d2059f 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -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);
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index be35422..3164500 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -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)) {
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index fb0294d..928855d 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -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);
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index eac1976..3284555 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -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
 
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c
index 5da0583..adc71f6 100644
--- a/net/sched/sch_red.c
+++ b/net/sched/sch_red.c
@@ -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)
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 6e041d1..11b3098 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -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
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 7d3b7ff..4f84ea6 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -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);
diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
index 2c35c67..befc919 100644
--- a/net/sched/sch_teql.c
+++ b/net/sched/sch_teql.c
@@ -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

[Jarek Poplawski]

On Wed, Aug 06, 2008 at 10:09:11PM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)
> 
> > 
> > I still haven't seen anyone suggest creating a __NET_XMIT_KFREE_SKB to
> > 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.
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index a7abfda..4bfee1b 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -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);

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.

> +
> +	return ret & NET_XMIT_MASK;
>  }
...

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[Jarek Poplawski]

On Wed, Aug 06, 2008 at 10:09:11PM -0700, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)
> 
> > 
> > I still haven't seen anyone suggest creating a __NET_XMIT_KFREE_SKB to
> > 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

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 7 Aug 2008 10:09:10 +0000

> On Wed, Aug 06, 2008 at 10:09:11PM -0700, David Miller wrote:
> > From: David Miller <davem@davemloft.net>
> > Date: Wed, 06 Aug 2008 20:26:36 -0700 (PDT)
> > 
> > > 
> > > I still haven't seen anyone suggest creating a __NET_XMIT_KFREE_SKB to
> > > 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?

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.

> 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 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.

> 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.

That might even be useful.

As it stands it was borderline buggy.

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[Jarek Poplawski]

On Thu, Aug 07, 2008 at 03:10:58AM -0700, David Miller wrote:
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Thu, 7 Aug 2008 10:09:10 +0000
...
> > 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?
> 
> 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.

Hmm... I'm not sure we're thinking about the same function?

Jarek P.

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[David Miller]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 7 Aug 2008 10:31:30 +0000

> On Thu, Aug 07, 2008 at 03:10:58AM -0700, David Miller wrote:
> > From: Jarek Poplawski <jarkao2@gmail.com>
> > Date: Thu, 7 Aug 2008 10:09:10 +0000
> ...
> > > 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?
> > 
> > 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.
> 
> 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

[Jussi Kivilinna]

Quoting "Jarek Poplawski" <jarkao2@gmail.com>:

> 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 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

[Jarek Poplawski]

On Thu, Aug 07, 2008 at 03:45:45AM -0700, David Miller wrote:
...
> 
> 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.

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)

[Jussi Kivilinna]

Quoting "Jarek Poplawski" <jarkao2@gmail.com>:

>> 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?
>>
>
> 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.

>
> 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

[Jarek Poplawski]

On Thu, Aug 07, 2008 at 02:36:54PM +0300, Jussi Kivilinna wrote:
> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
>
>> 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 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).

Yes, good point again: if ->enqueue() could be mixed with ->requeue()
there is more than uniformity argument to do the change everywhere.

Jarek P.

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb (Was: Re: [PATCH take 2] net_sched: Add qdisc __NET_XMIT_BYPASS flag)

[Jarek Poplawski]

On Thu, Aug 07, 2008 at 02:40:48PM +0300, Jussi Kivilinna wrote:
> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
>
>>> 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?
>>>
>>
>> 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.

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[David Miller]

From: "Jussi Kivilinna" <jussi.kivilinna@mbnet.fi>
Date: Thu, 07 Aug 2008 14:36:54 +0300

> Quoting "Jarek Poplawski" <jarkao2@gmail.com>:
> 
> > 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 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).

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

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
> 
> 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.

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[Patrick McHardy]

Herbert Xu wrote:
> David Miller <davem@davemloft.net> wrote:
>> 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.
> 
> 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.

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

[Herbert Xu]

On Tue, Aug 19, 2008 at 03:08:57PM +0200, Patrick McHardy wrote:
>
> >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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[Patrick McHardy]

Herbert Xu wrote:
> On Tue, Aug 19, 2008 at 03:08:57PM +0200, Patrick McHardy wrote:
>>> 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

[Herbert Xu]

On Tue, Aug 19, 2008 at 03:20:04PM +0200, Patrick McHardy wrote:
> 
> 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.

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,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[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 :-)

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[Jarek Poplawski]

On Tue, Aug 19, 2008 at 11:10:52PM +0300, Denys Fedoryshchenko wrote:
> 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.

Re: qdisc_enqueue, NET_XMIT_SUCCESS and kfree_skb

[David Miller]

From: Denys Fedoryshchenko <denys@visp.net.lb>
Date: Tue, 19 Aug 2008 23:10:52 +0300

> Thanks for great code and support :-)

Thanks for all of your excellent testing :-)