handle_ing update

These are a couple of cleanups to make ingress a bit more lightweight.

Thanks!

Daniel Borkmann (2):
net: sched: consolidate handle_ing and ing_filter
net: sched: further simplify handle_ing

 include/linux/netdevice.h |  4 ++++
 net/core/dev.c            | 48 ++++++++++++++++-----------------------
 net/sched/sch_ingress.c   | 58 ++++++++---------------------------------------
 3 files changed, 33 insertions(+), 77 deletions(-)

1.9.3

Given quite some code has been removed from ing_filter(), we can just consolidate that function into handle_ing() and get rid of a few instructions at the same time.

---
 net/core/dev.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

index 862875e..8a75746 100644
@@ -3521,37 +3521,19 @@ EXPORT_SYMBOL_GPL(br_fdb_test_addr_hook);
 #endif
 
 #ifdef CONFIG_NET_CLS_ACT
-/* TODO: Maybe we should just force sch_ingress to be compiled in
- * when CONFIG_NET_CLS_ACT is? otherwise some useless instructions
- * a compare and 2 stores extra right now if we dont have it on
- * but have CONFIG_NET_CLS_ACT
- * NOTE: This doesn't stop any functionality; if you dont have
- * the ingress scheduler, you just can't add policies on ingress.
- *
- */
-static int ing_filter(struct sk_buff *skb, struct netdev_queue *rxq)
-{
-	int result = TC_ACT_OK;
-	struct Qdisc *q;
-
-	skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
-
-	q = rcu_dereference(rxq->qdisc);
-	if (q != &noop_qdisc) {
-		if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state)))
-			result = qdisc_enqueue_root(skb, q);
-	}
-
-	return result;
-}
-
 static inline struct sk_buff *handle_ing(struct sk_buff *skb,
 					 struct packet_type **pt_prev,
 					 int *ret, struct net_device *orig_dev)
 {
 	struct netdev_queue *rxq = rcu_dereference(skb->dev->ingress_queue);
+	struct Qdisc *q;
 
-	if (!rxq || rcu_access_pointer(rxq->qdisc) == &noop_qdisc)
+	/* If there's at least one ingress present somewhere (so
+	 * we get here via enabled static key), remaining devices
+	 * that are not configured with an ingress qdisc will bail
+	 * out w/o the rcu_dereference().
+	 */
+	if (!rxq || (q = rcu_dereference(rxq->qdisc)) == &noop_qdisc)
 		return skb;
 
 	if (*pt_prev) {

@@ -3559,11 +3541,15 @@ static inline struct sk_buff *handle_ing(struct sk_buff *skb,
 		*pt_prev = NULL;
 	}
 
-	switch (ing_filter(skb, rxq)) {
-	case TC_ACT_SHOT:
-	case TC_ACT_STOLEN:
-		kfree_skb(skb);
-		return NULL;
+	skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
+
+	if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
+		switch (qdisc_enqueue_root(skb, q)) {
+		case TC_ACT_SHOT:
+		case TC_ACT_STOLEN:
+			kfree_skb(skb);
+			return NULL;
+		}
 	}
 
 	return skb;

1.9.3

Ingress qdisc has no other purpose than calling into tc_classify() that executes attached classifier(s) and action(s).

It has a 1:1 relationship to dev->ingress_queue. After having commit 087c1a601ad7 ("net: sched: run ingress qdisc without locks") removed the central ingress lock, one major contention point is gone.

The extra indirection layers however, are not necessary for calling into ingress qdisc. pktgen calling locally into netif_receive_skb() with a dummy u32, single CPU result on a Supermicro X10SLM-F, Xeon
E3-1240: before ~21,1 Mpps, after patch ~22,9 Mpps.

We can redirect the private classifier list to the netdev directly, without changing any classifier API bits (!) and execute on that from handle_ing() side. The __QDISC_STATE_DEACTIVATE test can be removed, ingress qdisc doesn't have a queue and thus dev_deactivate_queue() is also not applicable, ingress_cl_list provides similar behaviour. In other words, ingress qdisc acts like TCQ_F_BUILTIN qdisc.

One next possible step is the removal of the dev's ingress (dummy) netdev_queue, and to only have the list member in the netdevice itself.

Note, the filter chain is RCU protected and individual filter elements are being kfree'd by sched subsystem after RCU grace period. RCU read lock is being held by __netif_receive_skb_core().

Joint work with Alexei Starovoitov.

---
 include/linux/netdevice.h |  4 ++++
 net/core/dev.c            | 30 ++++++++++++++----------
 net/sched/sch_ingress.c   | 58 ++++++++---------------------------------------
 3 files changed, 31 insertions(+), 61 deletions(-)

index 1899c74..c4e1caf 100644
@@ -1655,7 +1655,11 @@ struct net_device {
 	rx_handler_func_t __rcu	*rx_handler;
 	void __rcu		*rx_handler_data;
 
+#if CONFIG_NET_CLS_ACT
+	struct tcf_proto __rcu  *ingress_cl_list;
+#endif
 	struct netdev_queue __rcu *ingress_queue;
+
 	unsigned char		broadcast[MAX_ADDR_LEN];
 #ifdef CONFIG_RFS_ACCEL
 	struct cpu_rmap		*rx_cpu_rmap;

index 8a75746..e5f77c4 100644
@@ -3525,31 +3525,37 @@ static inline struct sk_buff *handle_ing(struct sk_buff *skb,
 					 struct packet_type **pt_prev,
 					 int *ret, struct net_device *orig_dev)
 {
-	struct netdev_queue *rxq = rcu_dereference(skb->dev->ingress_queue);
-	struct Qdisc *q;
+	struct tcf_proto *cl = rcu_dereference_bh(skb->dev->ingress_cl_list);
+	struct tcf_result cl_res;
 
 	/* If there's at least one ingress present somewhere (so
 	 * we get here via enabled static key), remaining devices
 	 * that are not configured with an ingress qdisc will bail
-	 * out w/o the rcu_dereference().
+	 * out here.
 	 */
-	if (!rxq || (q = rcu_dereference(rxq->qdisc)) == &noop_qdisc)
+	if (!cl)
 		return skb;
-
 	if (*pt_prev) {
 		*ret = deliver_skb(skb, *pt_prev, orig_dev);
 		*pt_prev = NULL;
 	}
 
+	qdisc_bstats_update_cpu(cl->q, skb);
 	skb->tc_verd = SET_TC_AT(skb->tc_verd, AT_INGRESS);
 
-	if (likely(!test_bit(__QDISC_STATE_DEACTIVATED, &q->state))) {
-		switch (qdisc_enqueue_root(skb, q)) {
-		case TC_ACT_SHOT:
-		case TC_ACT_STOLEN:
-			kfree_skb(skb);
-			return NULL;
-		}
+	switch (tc_classify(skb, cl, &cl_res)) {
+	case TC_ACT_OK:
+	case TC_ACT_RECLASSIFY:
+		skb->tc_index = TC_H_MIN(cl_res.classid);
+		break;
+	case TC_ACT_SHOT:
+		qdisc_qstats_drop_cpu(cl->q);
+	case TC_ACT_STOLEN:
+	case TC_ACT_QUEUED:
+		kfree_skb(skb);
+		return NULL;
+	default:
+		break;
 	}
 
 	return skb;

1.9.3

lovely diffstat :)

2nd patch is even better:

 3 files changed, 31 insertions(+), 61 deletions(-)

This is plain wrong at many levels.

You're persisting on embedding the ingress code into the core, and you have to remember that most users don't need this. Modules allows people to get the code that they need into the core, with this approach, they have no other choice other than disable from .config this if they don't need it.

This has to be done the other way around. I just sent a patchset to clean up this that in exactly the other direction, as a result, performance is improved for users that don't need this.

We should do things to make users aware that when they request features, they have to pay a performance cost, and that happens by when you invoke:

tc qdisc add dev eth0 handle ffff: ingress

David already stated before that ingress path is performance critical, but you insist on trying to get qdisc ingress faster *at any cost*.

I think you're misreading the patch set. Where do you see that this is pushed on all users?
if (static_key_false(&ingress_needed))
still protects all of these bits.
When ingress qdisc is added, the key gets enabled and it only needs one deref to not go any further. Much faster than it is today.

you're doing exactly the same in your patch set, but with added extra overhead for netfilter hook.

Nope. We're cleaning up ingress qdisc path _without_ affecting anything else, whereas your netfilter hook creates binary choice for users whether they want nft or tc.
Please just add your own netfilter hook to netif_receive_skb and let's be done with this back and forth arguments.

I will *not* use that hook for Netfilter.

DOT.

From: Pablo Neira Ayuso <[email protected]>
Date: Sun, 10 May 2015 19:05:50 +0200

Pablo I think you are overreacting here.

What Daniel and Alexei are doing here is quite reasonable.

There is no difference between having ingress qdisc hanging off of netdevice vs. the classifier list. They both serve the same purpose and the change to use the classifier list merely removes one level of indirection.

I also am confident that the cost for non-users is equivalent before and after this patch series. Or at least, it very much should be.

Therefore I am going to apply these patches, and I wish you would work more closely with Daniel and Alexei because I am more than convinced that you share the same exact goals.

Thanks.

From: Daniel Borkmann <[email protected]>
Date: Sat, 9 May 2015 22:51:30 +0200

Series applied, thanks Daniel.

With adding 8 bytes to each of the netdevice even when it doesn't have ingress qdisc attached at all...

Also breaks the abstraction of Qdisc's which potentially breaks existing or future code, since now we have two different *layers* to hold tp_proto.

BTW, it should be protected by CONFIG_NET_SCH_INGRESS rather than CONFIG_NET_CLS_ACT.

If people really cared about ingress qdisc, that spinlock should have gone much earlier than in 4.x release, rather than waiting for me to remind.

we already said several times that the work is already on the way to remove 'ingress_queue' from net_device. It will save much more memory than addition of 8 bytes.

On 05/12/2015 12:03 AM, Cong Wang wrote:
...

Sure, I agree, this can further be improved. Feel free to send a patch.

Thanks for your reminder then. ;)

Cheers,
Daniel

This sounds like adding 8 bytes is a must step for removing more, I seriously doubt.

I never agree on that approach, it makes the code even messy, less readable. Correctness and readability are much more important than a little performance gain, for me, not for you of course.

On Mon, May 11, 2015 at 11:09:49AM -0400, David Miller wrote:
[...]

Sorry, I'll do my best to calm things down here.