[patch net-next v3 0/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[patch net-next v3 0/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

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

-- 
2.9.5

[patch net-next v3 1/2] net: sched: introduce chain_head_change callback

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

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 <jiri@mellanox.com>
---
 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(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index bf73e16..0f99cbc 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -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)
 {
 }
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 07c179d..16d0f8f 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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 */
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d9d54b3..dfb015a 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -195,12 +195,19 @@ static struct tcf_chain *tcf_chain_create(struct tcf_block *block,
 	return chain;
 }
 
+static void tcf_chain_head_change(struct tcf_chain *chain,
+				  struct tcf_proto *tp_head)
+{
+	if (chain->chain_head_change)
+		chain->chain_head_change(tp_head,
+					 chain->chain_head_change_priv);
+}
+
 static void tcf_chain_flush(struct tcf_chain *chain)
 {
 	struct tcf_proto *tp;
 
-	if (chain->p_filter_chain)
-		RCU_INIT_POINTER(*chain->p_filter_chain, NULL);
+	tcf_chain_head_change(chain, NULL);
 	while ((tp = rtnl_dereference(chain->filter_chain)) != NULL) {
 		RCU_INIT_POINTER(chain->filter_chain, tp->next);
 		tcf_chain_put(chain);
@@ -242,13 +249,6 @@ void tcf_chain_put(struct tcf_chain *chain)
 }
 EXPORT_SYMBOL(tcf_chain_put);
 
-static void
-tcf_chain_filter_chain_ptr_set(struct tcf_chain *chain,
-			       struct tcf_proto __rcu **p_filter_chain)
-{
-	chain->p_filter_chain = p_filter_chain;
-}
-
 static void tcf_block_offload_cmd(struct tcf_block *block, struct Qdisc *q,
 				  struct tcf_block_ext_info *ei,
 				  enum tc_block_command command)
@@ -276,8 +276,7 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
 	tcf_block_offload_cmd(block, q, ei, TC_BLOCK_UNBIND);
 }
 
-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)
 {
 	struct tcf_block *block = kzalloc(sizeof(*block), GFP_KERNEL);
@@ -295,7 +294,9 @@ int tcf_block_get_ext(struct tcf_block **p_block,
 		err = -ENOMEM;
 		goto err_chain_create;
 	}
-	tcf_chain_filter_chain_ptr_set(chain, p_filter_chain);
+	WARN_ON(!ei->chain_head_change);
+	chain->chain_head_change = ei->chain_head_change;
+	chain->chain_head_change_priv = ei->chain_head_change_priv;
 	block->net = qdisc_net(q);
 	block->q = q;
 	tcf_block_offload_bind(block, q, ei);
@@ -308,12 +309,23 @@ int tcf_block_get_ext(struct tcf_block **p_block,
 }
 EXPORT_SYMBOL(tcf_block_get_ext);
 
+static void tcf_chain_head_change_dflt(struct tcf_proto *tp_head, void *priv)
+{
+	struct tcf_proto __rcu **p_filter_chain = priv;
+
+	rcu_assign_pointer(*p_filter_chain, tp_head);
+}
+
 int tcf_block_get(struct tcf_block **p_block,
 		  struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q)
 {
-	struct tcf_block_ext_info ei = {0, };
+	struct tcf_block_ext_info ei = {
+		.chain_head_change = tcf_chain_head_change_dflt,
+		.chain_head_change_priv = p_filter_chain,
+	};
 
-	return tcf_block_get_ext(p_block, p_filter_chain, q, &ei);
+	WARN_ON(!p_filter_chain);
+	return tcf_block_get_ext(p_block, q, &ei);
 }
 EXPORT_SYMBOL(tcf_block_get);
 
@@ -359,8 +371,7 @@ static void tcf_block_put_deferred(struct work_struct *work)
 	rtnl_unlock();
 }
 
-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)
 {
 	if (!block)
@@ -382,7 +393,7 @@ void tcf_block_put(struct tcf_block *block)
 {
 	struct tcf_block_ext_info ei = {0, };
 
-	tcf_block_put_ext(block, NULL, block->q, &ei);
+	tcf_block_put_ext(block, block->q, &ei);
 }
 
 EXPORT_SYMBOL(tcf_block_put);
@@ -558,9 +569,8 @@ static void tcf_chain_tp_insert(struct tcf_chain *chain,
 				struct tcf_chain_info *chain_info,
 				struct tcf_proto *tp)
 {
-	if (chain->p_filter_chain &&
-	    *chain_info->pprev == chain->filter_chain)
-		rcu_assign_pointer(*chain->p_filter_chain, tp);
+	if (*chain_info->pprev == chain->filter_chain)
+		tcf_chain_head_change(chain, tp);
 	RCU_INIT_POINTER(tp->next, tcf_chain_tp_prev(chain_info));
 	rcu_assign_pointer(*chain_info->pprev, tp);
 	tcf_chain_hold(chain);
@@ -572,8 +582,8 @@ static void tcf_chain_tp_remove(struct tcf_chain *chain,
 {
 	struct tcf_proto *next = rtnl_dereference(chain_info->next);
 
-	if (chain->p_filter_chain && tp == chain->filter_chain)
-		RCU_INIT_POINTER(*chain->p_filter_chain, next);
+	if (tp == chain->filter_chain)
+		tcf_chain_head_change(chain, next);
 	RCU_INIT_POINTER(*chain_info->pprev, next);
 	tcf_chain_put(chain);
 }
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index b599db2..8118458 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -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();
-- 
2.9.5

[patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

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 <jiri@mellanox.com>
---
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(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5e02f79..7de7656 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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. */
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 16d0f8f..f670272 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -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
diff --git a/net/core/dev.c b/net/core/dev.c
index 24ac908..1423cf4 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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:
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index aa74aa4..3839cbb 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1024,3 +1024,49 @@ void psched_ratecfg_precompute(struct psched_ratecfg *r,
 	}
 }
 EXPORT_SYMBOL(psched_ratecfg_precompute);
+
+static void mini_qdisc_rcu_func(struct rcu_head *head)
+{
+}
+
+void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
+			  struct tcf_proto *tp_head)
+{
+	struct mini_Qdisc *miniq_old = rtnl_dereference(*miniqp->p_miniq);
+	struct mini_Qdisc *miniq;
+
+	if (!tp_head) {
+		RCU_INIT_POINTER(*miniqp->p_miniq, NULL);
+		return;
+	}
+
+	miniq = !miniq_old || miniq_old == &miniqp->miniq2 ?
+		&miniqp->miniq1 : &miniqp->miniq2;
+
+	/* We need to make sure that readers won't see the miniq
+	 * we are about to modify. So wait until previous call_rcu_bh callback
+	 * is done.
+	 */
+	rcu_barrier_bh();
+	miniq->filter_list = tp_head;
+	rcu_assign_pointer(*miniqp->p_miniq, miniq);
+
+	if (miniq_old)
+		/* This is counterpart of the rcu barrier above. We need to
+		 * block potential new user of miniq_old until all readers
+		 * are not seeing it.
+		 */
+		call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func);
+}
+EXPORT_SYMBOL(mini_qdisc_pair_swap);
+
+void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
+			  struct mini_Qdisc __rcu **p_miniq)
+{
+	miniqp->miniq1.cpu_bstats = qdisc->cpu_bstats;
+	miniqp->miniq1.cpu_qstats = qdisc->cpu_qstats;
+	miniqp->miniq2.cpu_bstats = qdisc->cpu_bstats;
+	miniqp->miniq2.cpu_qstats = qdisc->cpu_qstats;
+	miniqp->p_miniq = p_miniq;
+}
+EXPORT_SYMBOL(mini_qdisc_pair_init);
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 8118458..5ecc38f 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -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)
-- 
2.9.5

Re: [patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Alexei Starovoitov]

On Tue, Oct 31, 2017 at 04:12:22PM +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> 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 <jiri@mellanox.com>
> ---
> 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.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 24ac908..1423cf4 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -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)) {

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.

Re: [patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko]

Wed, Nov 01, 2017 at 03:12:50AM CET, alexei.starovoitov@gmail.com wrote:
>On Tue, Oct 31, 2017 at 04:12:22PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> 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 <jiri@mellanox.com>
>> ---
>> 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.

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?


>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 24ac908..1423cf4 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -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)) {
>
>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.
>

Re: [patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Daniel Borkmann]

On 11/01/2017 03:12 AM, Alexei Starovoitov wrote:
> On Tue, Oct 31, 2017 at 04:12:22PM +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
[...]
> 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.

> 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

Re: [patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko]

Wed, Nov 01, 2017 at 11:25:02AM CET, daniel@iogearbox.net wrote:
>On 11/01/2017 03:12 AM, Alexei Starovoitov wrote:
>> On Tue, Oct 31, 2017 at 04:12:22PM +0100, Jiri Pirko wrote:
>> > From: Jiri Pirko <jiri@mellanox.com>
>[...]
>> 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.

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

Re: [patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Alexei Starovoitov]

On Wed, Nov 01, 2017 at 09:18:03AM +0100, Jiri Pirko wrote:
> Wed, Nov 01, 2017 at 03:12:50AM CET, alexei.starovoitov@gmail.com wrote:
> >On Tue, Oct 31, 2017 at 04:12:22PM +0100, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@mellanox.com>
> >> 
> >> 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 <jiri@mellanox.com>
> >> ---
> >> 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.
> 
> 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?

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

Re: [patch net-next v3 2/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko]

Wed, Nov 01, 2017 at 05:11:14PM CET, alexei.starovoitov@gmail.com wrote:
>On Wed, Nov 01, 2017 at 09:18:03AM +0100, Jiri Pirko wrote:
>> Wed, Nov 01, 2017 at 03:12:50AM CET, alexei.starovoitov@gmail.com wrote:
>> >On Tue, Oct 31, 2017 at 04:12:22PM +0100, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@mellanox.com>
>> >> 
>> >> 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 <jiri@mellanox.com>
>> >> ---
>> >> 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.
>> 
>> 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?
>
>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!

Re: [patch net-next v3 0/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 31 Oct 2017 16:12:20 +0100

> From: Jiri Pirko <jiri@mellanox.com>
> 
> 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.

Thanks.

Re: [patch net-next v3 0/2] net: core: introduce mini_Qdisc and eliminate usage of tp->q for clsact fastpath

[Jiri Pirko]

Fri, Nov 03, 2017 at 02:24:48AM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Tue, 31 Oct 2017 16:12:20 +0100
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> 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.