[PATCH net-next 0/2] handle_ing update

[PATCH net-next 0/2] handle_ing update

[Daniel Borkmann]

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

[PATCH net-next 1/2] net: sched: consolidate handle_ing and ing_filter

[Daniel Borkmann]

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.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/core/dev.c | 46 ++++++++++++++++------------------------------
 1 file changed, 16 insertions(+), 30 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 862875e..8a75746 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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

[PATCH net-next 2/2] net: sched: further simplify handle_ing

[Daniel Borkmann]

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.

Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Alexei Starovoitov <ast@plumgrid.com>
---
 include/linux/netdevice.h |  4 ++++
 net/core/dev.c            | 30 ++++++++++++++----------
 net/sched/sch_ingress.c   | 58 ++++++++---------------------------------------
 3 files changed, 31 insertions(+), 61 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 1899c74..c4e1caf 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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;
diff --git a/net/core/dev.c b/net/core/dev.c
index 8a75746..e5f77c4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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;
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index a89cc32..e7c648f 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -12,16 +12,10 @@
 #include <linux/list.h>
 #include <linux/skbuff.h>
 #include <linux/rtnetlink.h>
+
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
 
-
-struct ingress_qdisc_data {
-	struct tcf_proto __rcu	*filter_list;
-};
-
-/* ------------------------- Class/flow operations ------------------------- */
-
 static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
 {
 	return NULL;
@@ -49,45 +43,11 @@ static void ingress_walk(struct Qdisc *sch, struct qdisc_walker *walker)
 static struct tcf_proto __rcu **ingress_find_tcf(struct Qdisc *sch,
 						 unsigned long cl)
 {
-	struct ingress_qdisc_data *p = qdisc_priv(sch);
-
-	return &p->filter_list;
-}
-
-/* --------------------------- Qdisc operations ---------------------------- */
+	struct net_device *dev = qdisc_dev(sch);
 
-static int ingress_enqueue(struct sk_buff *skb, struct Qdisc *sch)
-{
-	struct ingress_qdisc_data *p = qdisc_priv(sch);
-	struct tcf_result res;
-	struct tcf_proto *fl = rcu_dereference_bh(p->filter_list);
-	int result;
-
-	result = tc_classify(skb, fl, &res);
-
-	qdisc_bstats_update_cpu(sch, skb);
-	switch (result) {
-	case TC_ACT_SHOT:
-		result = TC_ACT_SHOT;
-		qdisc_qstats_drop_cpu(sch);
-		break;
-	case TC_ACT_STOLEN:
-	case TC_ACT_QUEUED:
-		result = TC_ACT_STOLEN;
-		break;
-	case TC_ACT_RECLASSIFY:
-	case TC_ACT_OK:
-		skb->tc_index = TC_H_MIN(res.classid);
-	default:
-		result = TC_ACT_OK;
-		break;
-	}
-
-	return result;
+	return &dev->ingress_cl_list;
 }
 
-/* ------------------------------------------------------------- */
-
 static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 {
 	net_inc_ingress_queue();
@@ -98,9 +58,9 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 
 static void ingress_destroy(struct Qdisc *sch)
 {
-	struct ingress_qdisc_data *p = qdisc_priv(sch);
+	struct net_device *dev = qdisc_dev(sch);
 
-	tcf_destroy_chain(&p->filter_list);
+	tcf_destroy_chain(&dev->ingress_cl_list);
 	net_dec_ingress_queue();
 }
 
@@ -111,6 +71,7 @@ static int ingress_dump(struct Qdisc *sch, struct sk_buff *skb)
 	nest = nla_nest_start(skb, TCA_OPTIONS);
 	if (nest == NULL)
 		goto nla_put_failure;
+
 	return nla_nest_end(skb, nest);
 
 nla_put_failure:
@@ -131,8 +92,6 @@ static const struct Qdisc_class_ops ingress_class_ops = {
 static struct Qdisc_ops ingress_qdisc_ops __read_mostly = {
 	.cl_ops		=	&ingress_class_ops,
 	.id		=	"ingress",
-	.priv_size	=	sizeof(struct ingress_qdisc_data),
-	.enqueue	=	ingress_enqueue,
 	.init		=	ingress_init,
 	.destroy	=	ingress_destroy,
 	.dump		=	ingress_dump,
@@ -149,6 +108,7 @@ static void __exit ingress_module_exit(void)
 	unregister_qdisc(&ingress_qdisc_ops);
 }
 
-module_init(ingress_module_init)
-module_exit(ingress_module_exit)
+module_init(ingress_module_init);
+module_exit(ingress_module_exit);
+
 MODULE_LICENSE("GPL");
-- 
1.9.3

Re: [PATCH net-next 1/2] net: sched: consolidate handle_ing and ing_filter

[Alexei Starovoitov]

On 5/9/15 1:51 PM, Daniel Borkmann wrote:
> 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.
>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>   net/core/dev.c | 46 ++++++++++++++++------------------------------
>   1 file changed, 16 insertions(+), 30 deletions(-)

lovely diffstat :)

2nd patch is even better:
  3 files changed, 31 insertions(+), 61 deletions(-)

Acked-by: Alexei Starovoitov <ast@plumgrid.com>

Re: [PATCH net-next 0/2] handle_ing update

[Pablo Neira Ayuso]

On Sat, May 09, 2015 at 10:51:30PM +0200, Daniel Borkmann wrote:
> These are a couple of cleanups to make ingress a bit more lightweight.

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

Re: [PATCH net-next 0/2] handle_ing update

[Alexei Starovoitov]

On 5/10/15 10:05 AM, Pablo Neira Ayuso wrote:
> On Sat, May 09, 2015 at 10:51:30PM +0200, Daniel Borkmann wrote:
>> These are a couple of cleanups to make ingress a bit more lightweight.
>
> 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.

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.

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

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

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

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.

Re: [PATCH net-next 0/2] handle_ing update

[Pablo Neira Ayuso]

On Sun, May 10, 2015 at 10:55:23AM -0700, Alexei Starovoitov wrote:
> you're doing exactly the same in your patch set, but with added extra
> overhead for netfilter hook.

I will *not* use that hook for Netfilter.

DOT.

Re: [PATCH net-next 0/2] handle_ing update

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Sun, 10 May 2015 19:05:50 +0200

> On Sat, May 09, 2015 at 10:51:30PM +0200, Daniel Borkmann wrote:
>> These are a couple of cleanups to make ingress a bit more lightweight.
> 
> 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*.

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.

Re: [PATCH net-next 0/2] handle_ing update

[David Miller]

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Sat,  9 May 2015 22:51:30 +0200

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

Series applied, thanks Daniel.

Re: [PATCH net-next 0/2] handle_ing update

[Cong Wang]

On Mon, May 11, 2015 at 8:09 AM, David Miller <davem@davemloft.net> wrote:
> 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.

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.

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

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.

Re: [PATCH net-next 0/2] handle_ing update

[Alexei Starovoitov]

On 5/11/15 3:03 PM, Cong Wang wrote:
> On Mon, May 11, 2015 at 8:09 AM, David Miller <davem@davemloft.net> wrote:
>> 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.
>
> With adding 8 bytes to each of the netdevice even when it doesn't
> have ingress qdisc attached at all...

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.

Re: [PATCH net-next 0/2] handle_ing update

[Daniel Borkmann]

On 05/12/2015 12:03 AM, Cong Wang wrote:
...
> BTW, it should be protected by CONFIG_NET_SCH_INGRESS
> rather than CONFIG_NET_CLS_ACT.

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

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

Thanks for your reminder then. ;)

Cheers,
Daniel

Re: [PATCH net-next 0/2] handle_ing update

[Cong Wang]

On Mon, May 11, 2015 at 3:23 PM, Alexei Starovoitov <ast@plumgrid.com> wrote:
> On 5/11/15 3:03 PM, Cong Wang wrote:
>>
>> On Mon, May 11, 2015 at 8:09 AM, David Miller <davem@davemloft.net> wrote:
>>>
>>> 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.
>>
>>
>> With adding 8 bytes to each of the netdevice even when it doesn't
>> have ingress qdisc attached at all...
>
>
> 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.

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

Re: [PATCH net-next 0/2] handle_ing update

[Cong Wang]

On Mon, May 11, 2015 at 3:33 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 05/12/2015 12:03 AM, Cong Wang wrote:
> ...
>>
>> BTW, it should be protected by CONFIG_NET_SCH_INGRESS
>> rather than CONFIG_NET_CLS_ACT.
>
>
> Sure, I agree, this can further be improved. Feel free to send a patch.

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.

Re: [PATCH net-next 0/2] handle_ing update

[Pablo Neira Ayuso]

On Mon, May 11, 2015 at 11:09:49AM -0400, David Miller wrote:
[...]
> Pablo I think you are overreacting here.

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