Skip to contentSkip to conversation

net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

PATCH Superseded Jiri Pirko 4 participants 11 messages Oct 31, 2017 – Nov 3, 2017

Subsystems (3)
Networking Network drivers Traffic control (tc)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Jiri Pirko

author maintainer
#0 lore raw

v3 cover net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

From: Jiri Pirko <[email protected]>

This patchset's main patch is patch number 2. It carries the description and changelog. Patch 1 is just a dependency.

Jiri Pirko (2):
net: sched: introduce chain_head_change callback
net: core: introduce mini_Qdisc and eliminate usage of tp->q for
clsact fastpath

 include/linux/netdevice.h |  9 +++++---
 include/net/pkt_cls.h     | 14 ++++++------
 include/net/sch_generic.h | 37 +++++++++++++++++++++++++++++++-
 net/core/dev.c            | 21 +++++++++---------
 net/sched/cls_api.c       | 54 ++++++++++++++++++++++++++++-------------------
 net/sched/sch_generic.c   | 46 ++++++++++++++++++++++++++++++++++++++++
 net/sched/sch_ingress.c   | 45 +++++++++++++++++++++++++--------------
 7 files changed, 166 insertions(+), 60 deletions(-)
Signature · 1 line
2.9.5
author maintainer
#1 lore raw

PATCH v3 1/2 net: sched: introduce chain_head_change callback

From: Jiri Pirko <[email protected]>

Add a callback that is to be called whenever head of the chain changes. Also provide a callback for the default case when the caller gets a block using non-extended getter.

Signed-off-by Jiri Pirko <[email protected]>
---
 include/net/pkt_cls.h     | 14 ++++++------
 include/net/sch_generic.h |  5 ++++-
 net/sched/cls_api.c       | 54 ++++++++++++++++++++++++++++-------------------
 net/sched/sch_ingress.c   | 36 +++++++++++++++++--------------
 4 files changed, 62 insertions(+), 47 deletions(-)
include/net/pkt_cls.h +6 -8
index bf73e16..0f99cbc 100644
@@ -26,6 +26,8 @@ enum tcf_block_binder_type {
 
 struct tcf_block_ext_info {
 	enum tcf_block_binder_type binder_type;
+	tcf_chain_head_change_t *chain_head_change;
+	void *chain_head_change_priv;
 };
 
 struct tcf_block_cb;

@@ -37,12 +39,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
 void tcf_chain_put(struct tcf_chain *chain);
 int tcf_block_get(struct tcf_block **p_block,
 		  struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q);
-int tcf_block_get_ext(struct tcf_block **p_block,
-		      struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
+int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q,
 		      struct tcf_block_ext_info *ei);
 void tcf_block_put(struct tcf_block *block);
-void tcf_block_put_ext(struct tcf_block *block,
-		       struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
+void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q,
 		       struct tcf_block_ext_info *ei);
 
 static inline struct Qdisc *tcf_block_q(struct tcf_block *block)

@@ -82,8 +82,7 @@ int tcf_block_get(struct tcf_block **p_block,
 }
 
 static inline
-int tcf_block_get_ext(struct tcf_block **p_block,
-		      struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
+int tcf_block_get_ext(struct tcf_block **p_block, struct Qdisc *q,
 		      struct tcf_block_ext_info *ei)
 {
 	return 0;

@@ -94,8 +93,7 @@ static inline void tcf_block_put(struct tcf_block *block)
 }
 
 static inline
-void tcf_block_put_ext(struct tcf_block *block,
-		       struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q,
+void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q,
 		       struct tcf_block_ext_info *ei)
 {
 }
include/net/sch_generic.h +4 -1
index 07c179d..16d0f8f 100644
@@ -260,9 +260,12 @@ struct qdisc_skb_cb {
 	unsigned char		data[QDISC_CB_PRIV_LEN];
 };
 
+typedef void tcf_chain_head_change_t(struct tcf_proto *tp_head, void *priv);
+
 struct tcf_chain {
 	struct tcf_proto __rcu *filter_chain;
-	struct tcf_proto __rcu **p_filter_chain;
+	tcf_chain_head_change_t *chain_head_change;
+	void *chain_head_change_priv;
 	struct list_head list;
 	struct tcf_block *block;
 	u32 index; /* chain index */
net/sched/cls_api.c +32 -22

116 lines not on this page · raw message

net/sched/sch_ingress.c +20 -16
index b599db2..8118458 100644
@@ -54,6 +54,13 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl)
 	return q->block;
 }
 
+static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
+{
+	struct tcf_proto __rcu **p_filter_chain = priv;
+
+	rcu_assign_pointer(*p_filter_chain, tp_head);
+}
+
 static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 {
 	struct ingress_sched_data *q = qdisc_priv(sch);

@@ -61,9 +68,10 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 	int err;
 
 	q->block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
+	q->block_info.chain_head_change = clsact_chain_head_change;
+	q->block_info.chain_head_change_priv = &dev->ingress_cl_list;
 
-	err = tcf_block_get_ext(&q->block, &dev->ingress_cl_list,
-				sch, &q->block_info);
+	err = tcf_block_get_ext(&q->block, sch, &q->block_info);
 	if (err)
 		return err;
 

@@ -76,10 +84,8 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 static void ingress_destroy(struct Qdisc *sch)
 {
 	struct ingress_sched_data *q = qdisc_priv(sch);
-	struct net_device *dev = qdisc_dev(sch);
 
-	tcf_block_put_ext(q->block, &dev->ingress_cl_list,
-			  sch, &q->block_info);
+	tcf_block_put_ext(q->block, sch, &q->block_info);
 	net_dec_ingress_queue();
 }
 

@@ -162,16 +168,18 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt)
 	int err;
 
 	q->ingress_block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
+	q->ingress_block_info.chain_head_change = clsact_chain_head_change;
+	q->ingress_block_info.chain_head_change_priv = &dev->ingress_cl_list;
 
-	err = tcf_block_get_ext(&q->ingress_block, &dev->ingress_cl_list,
-				sch, &q->ingress_block_info);
+	err = tcf_block_get_ext(&q->ingress_block, sch, &q->ingress_block_info);
 	if (err)
 		return err;
 
 	q->egress_block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_EGRESS;
+	q->egress_block_info.chain_head_change = clsact_chain_head_change;
+	q->egress_block_info.chain_head_change_priv = &dev->egress_cl_list;
 
-	err = tcf_block_get_ext(&q->egress_block, &dev->egress_cl_list,
-				sch, &q->egress_block_info);
+	err = tcf_block_get_ext(&q->egress_block, sch, &q->egress_block_info);
 	if (err)
 		goto err_egress_block_get;
 

@@ -183,20 +191,16 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt)
 	return 0;
 
 err_egress_block_get:
-	tcf_block_put_ext(q->ingress_block, &dev->ingress_cl_list,
-			  sch, &q->ingress_block_info);
+	tcf_block_put_ext(q->ingress_block, sch, &q->ingress_block_info);
 	return err;
 }
 
 static void clsact_destroy(struct Qdisc *sch)
 {
 	struct clsact_sched_data *q = qdisc_priv(sch);
-	struct net_device *dev = qdisc_dev(sch);
 
-	tcf_block_put_ext(q->egress_block, &dev->egress_cl_list,
-			  sch, &q->egress_block_info);
-	tcf_block_put_ext(q->ingress_block, &dev->ingress_cl_list,
-			  sch, &q->ingress_block_info);
+	tcf_block_put_ext(q->egress_block, sch, &q->egress_block_info);
+	tcf_block_put_ext(q->ingress_block, sch, &q->ingress_block_info);
 
 	net_dec_ingress_queue();
 	net_dec_egress_queue();
Signature · 1 line
2.9.5
author maintainer
#2 lore raw

PATCH v3 2/2 net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

In reply to Jiri Pirko

From: Jiri Pirko <[email protected]>

In sch_handle_egress and sch_handle_ingress tp->q is used only in order to update stats. So stats and filter list are the only things that are needed in clsact qdisc fastpath processing. Introduce new mini_Qdisc struct to hold those items. Also, introduce a helper to swap the mini_Qdisc structures in case filter list head changes.

This removes need for tp->q usage without added overhead.

Signed-off-by Jiri Pirko <[email protected]>

---
v2->v3:
- Using head change callback to replace miniq pointer every time tp head changes. This eliminates one rcu dereference and makes the claim "without added overhead" valid.
v1->v2:
- Use dev instead of skb->dev in sch_handle_egress as pointed out by Daniel
- Fixed synchronize_rcu_bh() in mini_qdisc_disable and commented

---
 include/linux/netdevice.h |  9 ++++++---
 include/net/sch_generic.h | 32 ++++++++++++++++++++++++++++++++
 net/core/dev.c            | 21 +++++++++++----------
 net/sched/sch_generic.c   | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 net/sched/sch_ingress.c   | 19 ++++++++++++++-----
 5 files changed, 109 insertions(+), 18 deletions(-)
include/linux/netdevice.h +6 -3
index 5e02f79..7de7656 100644
@@ -1559,6 +1559,8 @@ enum netdev_priv_flags {
  *
  *	@rx_handler:		handler for received packets
  *	@rx_handler_data: 	XXX: need comments on this one
+ *	@miniq_ingress:		ingress/clsact qdisc specific data for
+ *				ingress processing
  *	@ingress_queue:		XXX: need comments on this one
  *	@broadcast:		hw bcast address
  *

@@ -1576,7 +1578,8 @@ enum netdev_priv_flags {
  *	@tx_global_lock: 	XXX: need comments on this one
  *
  *	@xps_maps:	XXX: need comments on this one
- *
+ *	@miniq_egress:		clsact qdisc specific data for
+ *				egress processing
  *	@watchdog_timeo:	Represents the timeout that is used by
  *				the watchdog (see dev_watchdog())
  *	@watchdog_timer:	List of timers

@@ -1795,7 +1798,7 @@ struct net_device {
 	void __rcu		*rx_handler_data;
 
 #ifdef CONFIG_NET_CLS_ACT
-	struct tcf_proto __rcu  *ingress_cl_list;
+	struct mini_Qdisc __rcu	*miniq_ingress;
 #endif
 	struct netdev_queue __rcu *ingress_queue;
 #ifdef CONFIG_NETFILTER_INGRESS

@@ -1826,7 +1829,7 @@ struct net_device {
 	struct xps_dev_maps __rcu *xps_maps;
 #endif
 #ifdef CONFIG_NET_CLS_ACT
-	struct tcf_proto __rcu  *egress_cl_list;
+	struct mini_Qdisc __rcu	*miniq_egress;
 #endif
 
 	/* These may be needed for future network-power-down code. */
include/net/sch_generic.h +32 -0
index 16d0f8f..f670272 100644
@@ -897,4 +897,36 @@ static inline void psched_ratecfg_getrate(struct tc_ratespec *res,
 	res->linklayer = (r->linklayer & TC_LINKLAYER_MASK);
 }
 
+/* Mini Qdisc serves for specific needs of ingress/clsact Qdisc.
+ * The fast path only needs to access filter list and to update stats
+ */
+struct mini_Qdisc {
+	struct tcf_proto *filter_list;
+	struct gnet_stats_basic_cpu __percpu *cpu_bstats;
+	struct gnet_stats_queue	__percpu *cpu_qstats;
+	struct rcu_head rcu;
+};
+
+static inline void mini_qdisc_bstats_cpu_update(struct mini_Qdisc *miniq,
+						const struct sk_buff *skb)
+{
+	bstats_cpu_update(this_cpu_ptr(miniq->cpu_bstats), skb);
+}
+
+static inline void mini_qdisc_qstats_cpu_drop(struct mini_Qdisc *miniq)
+{
+	this_cpu_inc(miniq->cpu_qstats->drops);
+}
+
+struct mini_Qdisc_pair {
+	struct mini_Qdisc miniq1;
+	struct mini_Qdisc miniq2;
+	struct mini_Qdisc __rcu **p_miniq;
+};
+
+void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
+			  struct tcf_proto *tp_head);
+void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
+			  struct mini_Qdisc __rcu **p_miniq);
+
 #endif
net/core/dev.c +11 -10
index 24ac908..1423cf4 100644
@@ -3274,22 +3274,22 @@ EXPORT_SYMBOL(dev_loopback_xmit);
 static struct sk_buff *
 sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 {
-	struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
+	struct mini_Qdisc *miniq = rcu_dereference_bh(dev->miniq_egress);
 	struct tcf_result cl_res;
 
-	if (!cl)
+	if (!miniq)
 		return skb;
 
 	/* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
-	qdisc_bstats_cpu_update(cl->q, skb);
+	mini_qdisc_bstats_cpu_update(miniq, skb);
 
-	switch (tcf_classify(skb, cl, &cl_res, false)) {
+	switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {
 	case TC_ACT_OK:
 	case TC_ACT_RECLASSIFY:
 		skb->tc_index = TC_H_MIN(cl_res.classid);
 		break;
 	case TC_ACT_SHOT:
-		qdisc_qstats_cpu_drop(cl->q);
+		mini_qdisc_qstats_cpu_drop(miniq);
 		*ret = NET_XMIT_DROP;
 		kfree_skb(skb);
 		return NULL;

@@ -4189,7 +4189,7 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 		   struct net_device *orig_dev)
 {
 #ifdef CONFIG_NET_CLS_ACT
-	struct tcf_proto *cl = rcu_dereference_bh(skb->dev->ingress_cl_list);
+	struct mini_Qdisc *miniq = rcu_dereference_bh(skb->dev->miniq_ingress);
 	struct tcf_result cl_res;
 
 	/* If there's at least one ingress present somewhere (so

@@ -4197,8 +4197,9 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 	 * that are not configured with an ingress qdisc will bail
 	 * out here.
 	 */
-	if (!cl)
+	if (!miniq)
 		return skb;
+
 	if (*pt_prev) {
 		*ret = deliver_skb(skb, *pt_prev, orig_dev);
 		*pt_prev = NULL;

@@ -4206,15 +4207,15 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 
 	qdisc_skb_cb(skb)->pkt_len = skb->len;
 	skb->tc_at_ingress = 1;
-	qdisc_bstats_cpu_update(cl->q, skb);
+	mini_qdisc_bstats_cpu_update(miniq, skb);
 
-	switch (tcf_classify(skb, cl, &cl_res, false)) {
+	switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {
 	case TC_ACT_OK:
 	case TC_ACT_RECLASSIFY:
 		skb->tc_index = TC_H_MIN(cl_res.classid);
 		break;
 	case TC_ACT_SHOT:
-		qdisc_qstats_cpu_drop(cl->q);
+		mini_qdisc_qstats_cpu_drop(miniq);
 		kfree_skb(skb);
 		return NULL;
 	case TC_ACT_STOLEN:
net/sched/sch_generic.c +46 -0

49 lines not on this page · raw message

net/sched/sch_ingress.c +14 -5
index 8118458..5ecc38f 100644
@@ -21,6 +21,7 @@
 struct ingress_sched_data {
 	struct tcf_block *block;
 	struct tcf_block_ext_info block_info;
+	struct mini_Qdisc_pair miniqp;
 };
 
 static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)

@@ -56,9 +57,9 @@ static struct tcf_block *ingress_tcf_block(struct Qdisc *sch, unsigned long cl)
 
 static void clsact_chain_head_change(struct tcf_proto *tp_head, void *priv)
 {
-	struct tcf_proto __rcu **p_filter_chain = priv;
+	struct mini_Qdisc_pair *miniqp = priv;
 
-	rcu_assign_pointer(*p_filter_chain, tp_head);
+	mini_qdisc_pair_swap(miniqp, tp_head);
 }
 
 static int ingress_init(struct Qdisc *sch, struct nlattr *opt)

@@ -67,9 +68,11 @@ static int ingress_init(struct Qdisc *sch, struct nlattr *opt)
 	struct net_device *dev = qdisc_dev(sch);
 	int err;
 
+	mini_qdisc_pair_init(&q->miniqp, sch, &dev->miniq_ingress);
+
 	q->block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
 	q->block_info.chain_head_change = clsact_chain_head_change;
-	q->block_info.chain_head_change_priv = &dev->ingress_cl_list;
+	q->block_info.chain_head_change_priv = &q->miniqp;
 
 	err = tcf_block_get_ext(&q->block, sch, &q->block_info);
 	if (err)

@@ -128,6 +131,8 @@ struct clsact_sched_data {
 	struct tcf_block *egress_block;
 	struct tcf_block_ext_info ingress_block_info;
 	struct tcf_block_ext_info egress_block_info;
+	struct mini_Qdisc_pair miniqp_ingress;
+	struct mini_Qdisc_pair miniqp_egress;
 };
 
 static unsigned long clsact_find(struct Qdisc *sch, u32 classid)

@@ -167,17 +172,21 @@ static int clsact_init(struct Qdisc *sch, struct nlattr *opt)
 	struct net_device *dev = qdisc_dev(sch);
 	int err;
 
+	mini_qdisc_pair_init(&q->miniqp_ingress, sch, &dev->miniq_ingress);
+
 	q->ingress_block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS;
 	q->ingress_block_info.chain_head_change = clsact_chain_head_change;
-	q->ingress_block_info.chain_head_change_priv = &dev->ingress_cl_list;
+	q->ingress_block_info.chain_head_change_priv = &q->miniqp_ingress;
 
 	err = tcf_block_get_ext(&q->ingress_block, sch, &q->ingress_block_info);
 	if (err)
 		return err;
 
+	mini_qdisc_pair_init(&q->miniqp_egress, sch, &dev->miniq_egress);
+
 	q->egress_block_info.binder_type = TCF_BLOCK_BINDER_TYPE_CLSACT_EGRESS;
 	q->egress_block_info.chain_head_change = clsact_chain_head_change;
-	q->egress_block_info.chain_head_change_priv = &dev->egress_cl_list;
+	q->egress_block_info.chain_head_change_priv = &q->miniqp_egress;
 
 	err = tcf_block_get_ext(&q->egress_block, sch, &q->egress_block_info);
 	if (err)
Signature · 1 line
2.9.5
#3 lore raw
Quoted 8 lines · Jiri Pirko

From: Jiri Pirko <[email protected]>

In sch_handle_egress and sch_handle_ingress tp->q is used only in order to update stats. So stats and filter list are the only things that are needed in clsact qdisc fastpath processing. Introduce new mini_Qdisc struct to hold those items. Also, introduce a helper to swap the mini_Qdisc structures in case filter list head changes.

Quoted 8 lines

This removes need for tp->q usage without added overhead.

Signed-off-by Jiri Pirko <[email protected]>

---
v2->v3:
- Using head change callback to replace miniq pointer every time tp head changes. This eliminates one rcu dereference and makes the claim "without added overhead" valid.

you kidding, right?
It's still two loads.

Quoted 12 lines
net/core/dev.c +1 -1
index 24ac908..1423cf4 100644
@@ -3274,22 +3274,22 @@ EXPORT_SYMBOL(dev_loopback_xmit);
 static struct sk_buff *
 sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 {
-	struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
+	struct mini_Qdisc *miniq = rcu_dereference_bh(dev->miniq_egress);
 	struct tcf_result cl_res;

net/core/dev.c EXPORT_SYMBOL(dev_loopback_xmit); · line 3286

Quoted 10 lines
net/core/dev.c +3 -3
-	if (!cl)
+	if (!miniq)
 		return skb;
 
 	/* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
-	qdisc_bstats_cpu_update(cl->q, skb);
+	mini_qdisc_bstats_cpu_update(miniq, skb);
 
-	switch (tcf_classify(skb, cl, &cl_res, false)) {
+	switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {

I don't think it's great, but I don't have any suggestions on how to avoid it, so I'm not objecting. Just disappointed that you keep adding stuff to tc and messing with sw fast path only to make parity with some obscure hw feature.
If it keeps going like this we'd need to come up with some new fast hook for clsbpf in ingress/egress paths. We use it for every packet, so extra loads are not great.
I guess they should be cache hits, but will take extra cache line. All of the bugs in tc logic recently are not comforting either.

author maintainer
#4 lore raw
Quoted 18 lines · Wed, Nov 01, 2017 at 03:12:50AM CET, [email protected] wrote:
Quoted 8 lines · Jiri Pirko

From: Jiri Pirko <[email protected]>

In sch_handle_egress and sch_handle_ingress tp->q is used only in order to update stats. So stats and filter list are the only things that are needed in clsact qdisc fastpath processing. Introduce new mini_Qdisc struct to hold those items. Also, introduce a helper to swap the mini_Qdisc structures in case filter list head changes.

Quoted 8 lines

This removes need for tp->q usage without added overhead.

Signed-off-by Jiri Pirko <[email protected]>

---
v2->v3:
- Using head change callback to replace miniq pointer every time tp head changes. This eliminates one rcu dereference and makes the claim "without added overhead" valid.

Quoted 2 lines

you kidding, right?
It's still two loads.

I'm not.
I replace:

one rcu_dereference_bh(dev->egress_cl_list)

	by one rcu_dereference_bh(dev->miniq_egress)

one dereference cl->q

	by one dereference miniq->filter_list

What do I miss?

Quoted 34 lines
Quoted 13 lines
net/core/dev.c +1 -1
index 24ac908..1423cf4 100644
@@ -3274,22 +3274,22 @@ EXPORT_SYMBOL(dev_loopback_xmit);
 static struct sk_buff *
 sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
 {
-	struct tcf_proto *cl = rcu_dereference_bh(dev->egress_cl_list);
+	struct mini_Qdisc *miniq = rcu_dereference_bh(dev->miniq_egress);
 	struct tcf_result cl_res;

net/core/dev.c EXPORT_SYMBOL(dev_loopback_xmit); · line 3286

Quoted 10 lines
net/core/dev.c +3 -3
-	if (!cl)
+	if (!miniq)
 		return skb;
 
 	/* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
-	qdisc_bstats_cpu_update(cl->q, skb);
+	mini_qdisc_bstats_cpu_update(miniq, skb);
 
-	switch (tcf_classify(skb, cl, &cl_res, false)) {
+	switch (tcf_classify(skb, miniq->filter_list, &cl_res, false)) {

I don't think it's great, but I don't have any suggestions on how to avoid it, so I'm not objecting. Just disappointed that you keep adding stuff to tc and messing with sw fast path only to make parity with some obscure hw feature.
If it keeps going like this we'd need to come up with some new fast hook for clsbpf in ingress/egress paths. We use it for every packet, so extra loads are not great.
I guess they should be cache hits, but will take extra cache line. All of the bugs in tc logic recently are not comforting either.

#5 lore raw
Quoted 25 lines · Jiri Pirko
Quoted 18 lines · Wed, Nov 01, 2017 at 03:12:50AM CET, [email protected] wrote:
Quoted 8 lines · Jiri Pirko

From: Jiri Pirko <[email protected]>

In sch_handle_egress and sch_handle_ingress tp->q is used only in order to update stats. So stats and filter list are the only things that are needed in clsact qdisc fastpath processing. Introduce new mini_Qdisc struct to hold those items. Also, introduce a helper to swap the mini_Qdisc structures in case filter list head changes.

Quoted 8 lines

This removes need for tp->q usage without added overhead.

Signed-off-by Jiri Pirko <[email protected]>

---
v2->v3:
- Using head change callback to replace miniq pointer every time tp head changes. This eliminates one rcu dereference and makes the claim "without added overhead" valid.

Quoted 2 lines

you kidding, right?
It's still two loads.

I'm not.
I replace:

Quoted 7 lines

one rcu_dereference_bh(dev->egress_cl_list)

	by one rcu_dereference_bh(dev->miniq_egress)

one dereference cl->q

	by one dereference miniq->filter_list

What do I miss?

that in assembler code they are the same when debug is off ?

author maintainer
#6 lore raw
Quoted 29 lines · Wed, Nov 01, 2017 at 05:11:14PM CET, [email protected] wrote:
Quoted 28 lines · Jiri Pirko
Quoted 18 lines · Wed, Nov 01, 2017 at 03:12:50AM CET, [email protected] wrote:
Quoted 8 lines · Jiri Pirko

From: Jiri Pirko <[email protected]>

In sch_handle_egress and sch_handle_ingress tp->q is used only in order to update stats. So stats and filter list are the only things that are needed in clsact qdisc fastpath processing. Introduce new mini_Qdisc struct to hold those items. Also, introduce a helper to swap the mini_Qdisc structures in case filter list head changes.

Quoted 8 lines

This removes need for tp->q usage without added overhead.

Signed-off-by Jiri Pirko <[email protected]>

---
v2->v3:
- Using head change callback to replace miniq pointer every time tp head changes. This eliminates one rcu dereference and makes the claim "without added overhead" valid.

Quoted 2 lines

you kidding, right?
It's still two loads.

I'm not.
I replace:

one rcu_dereference_bh(dev->egress_cl_list)

	by one rcu_dereference_bh(dev->miniq_egress)
Quoted 6 lines
Quoted 4 lines

one dereference cl->q

	by one dereference miniq->filter_list

What do I miss?

that in assembler code they are the same when debug is off ?

Pardon my ignorance, but I don't get your point :( Could you please elaborate a bit more? Thanks!

#7 lore raw
In reply to Alexei Starovoitov
Quoted 2 lines · Alexei Starovoitov
Quoted 1 line · Jiri Pirko

From: Jiri Pirko <[email protected]>

[...]

Quoted 4 lines

I don't think it's great, but I don't have any suggestions on how to avoid it, so I'm not objecting. Just disappointed that you keep adding stuff to tc and messing with sw fast path only to make parity with some obscure hw feature.

tc became a sink for that over time.

Quoted 5 lines

If it keeps going like this we'd need to come up with some new fast hook for clsbpf in ingress/egress paths. We use it for every packet, so extra loads are not great.
I guess they should be cache hits, but will take extra cache line. All of the bugs in tc logic recently are not comforting either.

+1

author maintainer
#8 lore raw
Quoted 10 lines · Wed, Nov 01, 2017 at 11:25:02AM CET, [email protected] wrote:
Quoted 2 lines · Alexei Starovoitov
Quoted 1 line · Jiri Pirko

From: Jiri Pirko <[email protected]>

[...]

Quoted 4 lines

I don't think it's great, but I don't have any suggestions on how to avoid it, so I'm not objecting. Just disappointed that you keep adding stuff to tc and messing with sw fast path only to make parity with some obscure hw feature.

tc became a sink for that over time.

Block sharing is not only useful for offloads. But I understand.

Quoted 8 lines
Quoted 6 lines

If it keeps going like this we'd need to come up with some new fast hook for clsbpf in ingress/egress paths. We use it for every packet, so extra loads are not great.
I guess they should be cache hits, but will take extra cache line. All of the bugs in tc logic recently are not comforting either.

+1

I don't see any effect of tc changes on clsbpf ingress/egress since clsact was introduced. Could you point it out?

author maintainer
#10 lore raw
Quoted 9 lines · Fri, Nov 03, 2017 at 02:24:48AM CET, [email protected] wrote:

From: Jiri Pirko <[email protected]>
Date: Tue, 31 Oct 2017 16:12:20 +0100

Quoted 4 lines

From: Jiri Pirko <[email protected]>

This patchset's main patch is patch number 2. It carries the description and changelog. Patch 1 is just a dependency.

This no longer applies cleanly and will require a respin.

Will respin and send again. Thanks.