Skip to contentSkip to conversation

TC: refactor act_mirred packets re-injection

PATCH Superseded Paolo Abeni 4 participants 12 messages Jul 26, 2018 – Jul 29, 2018

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

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

v4 cover TC: refactor act_mirred packets re-injection

This series is aimed at improving the act_mirred redirect performances. Such action is used by OVS to represent TC S/W flows, and it's current largest bottle-neck is the need for a skb_clone() for each packet.

The first 2 patches introduce some cleanup and safeguards to allow extending tca_result - we will use it to store RCU protected redirect information - and introduce a clear separation between user-space accessible tcfa_action values and internal values accessible only by the kernel.
Then a new tcfa_action value is introduced: TC_ACT_REINJECT, similar to TC_ACT_REDIRECT, but preserving the mirred semantic. Such value is not accessible from user-space.
The last patch exploits the newly introduced infrastructure in the act_mirred action, to avoid a skb_clone, when possible.

Overall this the above gives a ~10% performance improvement in forwarding tput, when using the TC S/W datapath.

v1 -> v2:
- preserve the rcu lock in act_bpf
- add and use a new action value to reinject the packets, preserving the mirred semantic

v2 -> v3:
- renamed to new action as TC_ACT_REINJECT
- TC_ACT_REINJECT is not exposed to user-space

v3 -> v4:
- dropped the TC_ACT_REDIRECT patch
- report failure via extack, too
- rename the new action as TC_ACT_REINSERT
- skip clone only if the control action don't touch tcf_result

Paolo Abeni (4):
net/sched: user-space can't set unknown tcfa_action values tc/act: remove unneeded RCU lock in action callback net/tc: introduce TC_ACT_REINSERT.
act_mirred: use TC_ACT_REINSERT when possible

 include/net/act_api.h        |  2 +-
 include/net/pkt_cls.h        |  3 ++
 include/net/sch_generic.h    | 21 +++++++++++++
 include/uapi/linux/pkt_cls.h |  6 ++--
 net/core/dev.c               |  6 +++-
 net/sched/act_api.c          | 17 +++++++++++
 net/sched/act_csum.c         | 12 ++------
 net/sched/act_ife.c          |  5 +--
 net/sched/act_mirred.c       | 59 +++++++++++++++++++++++++++---------
 net/sched/act_sample.c       |  4 +--
 net/sched/act_skbedit.c      | 10 ++----
 net/sched/act_skbmod.c       | 21 ++++++-------
 net/sched/act_tunnel_key.c   |  6 +---
 net/sched/act_vlan.c         | 19 +++++-------
 14 files changed, 121 insertions(+), 70 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH v4 1/4 net/sched: user-space can't set unknown tcfa_action values

Currently, when initializing an action, the user-space can specify and use arbitrary values for the tcfa_action field. If the value is unknown by the kernel, is implicitly threaded as TC_ACT_UNSPEC.

This change explicitly checks for unknown values at action creation time, and explicitly convert them to TC_ACT_UNSPEC. No functional changes are introduced, but this will allow introducing tcfa_action values not exposed to user-space in a later patch.

Note: we can't use the above to hide TC_ACT_REDIRECT from user-space, as the latter is already part of uAPI.

v3 -> v4:
- use an helper to check for action validity (JiriP)
- emit an extack for invalid actions (JiriP)

Signed-off-by Paolo Abeni <[email protected]>
---
 include/uapi/linux/pkt_cls.h |  6 ++++--
 net/sched/act_api.c          | 17 +++++++++++++++++
 2 files changed, 21 insertions(+), 2 deletions(-)
include/uapi/linux/pkt_cls.h +4 -2
index b4512254036b..48e5b5d49a34 100644
@@ -45,6 +45,7 @@ enum {
 				   * the skb and act like everything
 				   * is alright.
 				   */
+#define TC_ACT_VALUE_MAX	TC_ACT_TRAP
 
 /* There is a special kind of actions called "extended actions",
  * which need a value parameter. These have a local opcode located in

@@ -55,11 +56,12 @@ enum {
 #define __TC_ACT_EXT_SHIFT 28
 #define __TC_ACT_EXT(local) ((local) << __TC_ACT_EXT_SHIFT)
 #define TC_ACT_EXT_VAL_MASK ((1 << __TC_ACT_EXT_SHIFT) - 1)
-#define TC_ACT_EXT_CMP(combined, opcode) \
-	(((combined) & (~TC_ACT_EXT_VAL_MASK)) == opcode)
+#define TC_ACT_EXT_OPCODE(combined) ((combined) & (~TC_ACT_EXT_VAL_MASK))
+#define TC_ACT_EXT_CMP(combined, opcode) (TC_ACT_EXT_OPCODE(combined) == opcode)
 
 #define TC_ACT_JUMP __TC_ACT_EXT(1)
 #define TC_ACT_GOTO_CHAIN __TC_ACT_EXT(2)
+#define TC_ACT_EXT_OPCODE_MAX	TC_ACT_GOTO_CHAIN
 
 /* Action type identifiers*/
 enum {
net/sched/act_api.c +17 -0
index 148a89ab789b..bdccad583daf 100644
@@ -786,6 +786,15 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
 	return c;
 }
 
+static bool tcf_action_valid(int action)
+{
+	int opcode = TC_ACT_EXT_OPCODE(action);
+
+	if (!opcode)
+		return action <= TC_ACT_VALUE_MAX;
+	return opcode <= TC_ACT_EXT_OPCODE_MAX || action == TC_ACT_UNSPEC;
+}
+
 struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 				    struct nlattr *nla, struct nlattr *est,
 				    char *name, int ovr, int bind,

@@ -895,6 +904,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
 
+	if (!tcf_action_valid(a->tcfa_action)) {
+		net_warn_ratelimited("invalid %d action value, using "
+				     "TC_ACT_UNSPEC instead", a->tcfa_action);
+		NL_SET_ERR_MSG(extack, "invalid action value, using "
+			       "TC_ACT_UNSPEC instead");
+		a->tcfa_action = TC_ACT_UNSPEC;
+	}
+
 	return a;
 
 err_mod:
Signature · 1 line
2.17.1
author maintainer
#4 lore raw

PATCH v4 2/4 tc/act: remove unneeded RCU lock in action callback

In reply to Paolo Abeni

Each lockless action currently does its own RCU locking in ->act(). This allows using plain RCU accessor, even if the context is really RCU BH.

This change drops the per action RCU lock, replace the accessors with the _bh variant, cleans up a bit the surrounding code and documents the RCU status in the relevant header.
No functional nor performance change is intended.

The goal of this patch is clarifying that the RCU critical section used by the tc actions extends up to the classifier's caller.

v1 -> v2:
- preserve rcu lock in act_bpf: it's needed by eBPF helpers, as pointed out by Daniel

v3 -> v4:
- fixed some typos in the commit message (JiriP)

Signed-off-by Paolo Abeni <[email protected]>
Acked-by Jiri Pirko <[email protected]>
---
 include/net/act_api.h      |  2 +-
 include/net/sch_generic.h  |  2 ++
 net/sched/act_csum.c       | 12 +++---------
 net/sched/act_ife.c        |  5 +----
 net/sched/act_mirred.c     |  4 +---
 net/sched/act_sample.c     |  4 +---
 net/sched/act_skbedit.c    | 10 +++-------
 net/sched/act_skbmod.c     | 21 +++++++++------------
 net/sched/act_tunnel_key.c |  6 +-----
 net/sched/act_vlan.c       | 19 +++++++------------
 10 files changed, 29 insertions(+), 56 deletions(-)
include/net/act_api.h +1 -1
index 683ce41053d9..8c9bc02d05e1 100644
@@ -85,7 +85,7 @@ struct tc_action_ops {
 	size_t	size;
 	struct module		*owner;
 	int     (*act)(struct sk_buff *, const struct tc_action *,
-		       struct tcf_result *);
+		       struct tcf_result *); /* called under RCU BH lock*/
 	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
 	void	(*cleanup)(struct tc_action *);
 	int     (*lookup)(struct net *net, struct tc_action **a, u32 index,
include/net/sch_generic.h +2 -0
index 085c509c8674..c9af9ce33055 100644
@@ -285,6 +285,8 @@ struct tcf_proto {
 	/* Fast access part */
 	struct tcf_proto __rcu	*next;
 	void __rcu		*root;
+
+	/* called under RCU BH lock*/
 	int			(*classify)(struct sk_buff *,
 					    const struct tcf_proto *,
 					    struct tcf_result *);
net/sched/act_csum.c +3 -9
index 4e8c383f379e..648a3a35b720 100644
@@ -561,15 +561,14 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
 	u32 update_flags;
 	int action;
 
-	rcu_read_lock();
-	params = rcu_dereference(p->params);
+	params = rcu_dereference_bh(p->params);
 
 	tcf_lastuse_update(&p->tcf_tm);
 	bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb);
 
 	action = READ_ONCE(p->tcf_action);
 	if (unlikely(action == TC_ACT_SHOT))
-		goto drop_stats;
+		goto drop;
 
 	update_flags = params->update_flags;
 	switch (tc_skb_protocol(skb)) {

@@ -583,16 +582,11 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a,
 		break;
 	}
 
-unlock:
-	rcu_read_unlock();
 	return action;
 
 drop:
-	action = TC_ACT_SHOT;
-
-drop_stats:
 	qstats_drop_inc(this_cpu_ptr(p->common.cpu_qstats));
-	goto unlock;
+	return TC_ACT_SHOT;
 }
 
 static int tcf_csum_dump(struct sk_buff *skb, struct tc_action *a, int bind,
net/sched/act_ife.c +1 -4
index 3d6e265758c0..df4060e32d43 100644
@@ -820,14 +820,11 @@ static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
 	struct tcf_ife_params *p;
 	int ret;
 
-	rcu_read_lock();
-	p = rcu_dereference(ife->params);
+	p = rcu_dereference_bh(ife->params);
 	if (p->flags & IFE_ENCODE) {
 		ret = tcf_ife_encode(skb, a, res, p);
-		rcu_read_unlock();
 		return ret;
 	}
-	rcu_read_unlock();
 
 	return tcf_ife_decode(skb, a, res);
 }
net/sched/act_mirred.c +1 -3
index 6afd89a36c69..eeb335f03102 100644
@@ -181,11 +181,10 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 	tcf_lastuse_update(&m->tcf_tm);
 	bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
 
-	rcu_read_lock();
 	m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
 	m_eaction = READ_ONCE(m->tcfm_eaction);
 	retval = READ_ONCE(m->tcf_action);
-	dev = rcu_dereference(m->tcfm_dev);
+	dev = rcu_dereference_bh(m->tcfm_dev);
 	if (unlikely(!dev)) {
 		pr_notice_once("tc mirred: target device is gone\n");
 		goto out;

@@ -236,7 +235,6 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		if (tcf_mirred_is_act_redirect(m_eaction))
 			retval = TC_ACT_SHOT;
 	}
-	rcu_read_unlock();
 
 	return retval;
 }
net/sched/act_sample.c +1 -3
index 3079e7be5bde..2608ccc83e5e 100644
@@ -140,8 +140,7 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
 	bstats_cpu_update(this_cpu_ptr(s->common.cpu_bstats), skb);
 	retval = READ_ONCE(s->tcf_action);
 
-	rcu_read_lock();
-	psample_group = rcu_dereference(s->psample_group);
+	psample_group = rcu_dereference_bh(s->psample_group);
 
 	/* randomly sample packets according to rate */
 	if (psample_group && (prandom_u32() % s->rate == 0)) {

@@ -165,7 +164,6 @@ static int tcf_sample_act(struct sk_buff *skb, const struct tc_action *a,
 			skb_pull(skb, skb->mac_len);
 	}
 
-	rcu_read_unlock();
 	return retval;
 }
net/sched/act_skbedit.c +3 -7
index da56e6938c9e..a6db47ebec11 100644
@@ -43,8 +43,7 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 	tcf_lastuse_update(&d->tcf_tm);
 	bstats_cpu_update(this_cpu_ptr(d->common.cpu_bstats), skb);
 
-	rcu_read_lock();
-	params = rcu_dereference(d->params);
+	params = rcu_dereference_bh(d->params);
 	action = READ_ONCE(d->tcf_action);
 
 	if (params->flags & SKBEDIT_F_PRIORITY)

@@ -77,14 +76,11 @@ static int tcf_skbedit(struct sk_buff *skb, const struct tc_action *a,
 	}
 	if (params->flags & SKBEDIT_F_PTYPE)
 		skb->pkt_type = params->ptype;
-
-unlock:
-	rcu_read_unlock();
 	return action;
+
 err:
 	qstats_drop_inc(this_cpu_ptr(d->common.cpu_qstats));
-	action = TC_ACT_SHOT;
-	goto unlock;
+	return TC_ACT_SHOT;
 }
 
 static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
net/sched/act_skbmod.c +9 -12
index cdc6bacfb190..c437c6d51a71 100644
@@ -41,20 +41,14 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 	 * then MAX_EDIT_LEN needs to change appropriately
 	*/
 	err = skb_ensure_writable(skb, MAX_EDIT_LEN);
-	if (unlikely(err)) { /* best policy is to drop on the floor */
-		qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
-		return TC_ACT_SHOT;
-	}
+	if (unlikely(err)) /* best policy is to drop on the floor */
+		goto drop;
 
-	rcu_read_lock();
 	action = READ_ONCE(d->tcf_action);
-	if (unlikely(action == TC_ACT_SHOT)) {
-		qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
-		rcu_read_unlock();
-		return action;
-	}
+	if (unlikely(action == TC_ACT_SHOT))
+		goto drop;
 
-	p = rcu_dereference(d->skbmod_p);
+	p = rcu_dereference_bh(d->skbmod_p);
 	flags = p->flags;
 	if (flags & SKBMOD_F_DMAC)
 		ether_addr_copy(eth_hdr(skb)->h_dest, p->eth_dst);

@@ -62,7 +56,6 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 		ether_addr_copy(eth_hdr(skb)->h_source, p->eth_src);
 	if (flags & SKBMOD_F_ETYPE)
 		eth_hdr(skb)->h_proto = p->eth_type;
-	rcu_read_unlock();
 
 	if (flags & SKBMOD_F_SWAPMAC) {
 		u16 tmpaddr[ETH_ALEN / 2]; /* ether_addr_copy() requirement */

@@ -73,6 +66,10 @@ static int tcf_skbmod_run(struct sk_buff *skb, const struct tc_action *a,
 	}
 
 	return action;
+
+drop:
+	qstats_overlimit_inc(this_cpu_ptr(d->common.cpu_qstats));
+	return TC_ACT_SHOT;
 }
 
 static const struct nla_policy skbmod_policy[TCA_SKBMOD_MAX + 1] = {
net/sched/act_tunnel_key.c +1 -5
index f811850fd1d0..d42d9e112789 100644
@@ -31,9 +31,7 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
 	struct tcf_tunnel_key_params *params;
 	int action;
 
-	rcu_read_lock();
-
-	params = rcu_dereference(t->params);
+	params = rcu_dereference_bh(t->params);
 
 	tcf_lastuse_update(&t->tcf_tm);
 	bstats_cpu_update(this_cpu_ptr(t->common.cpu_bstats), skb);

@@ -53,8 +51,6 @@ static int tunnel_key_act(struct sk_buff *skb, const struct tc_action *a,
 		break;
 	}
 
-	rcu_read_unlock();
-
 	return action;
 }
net/sched/act_vlan.c +7 -12
index ad37f308175a..15a0ee214c9c 100644
@@ -40,11 +40,9 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
 	if (skb_at_tc_ingress(skb))
 		skb_push_rcsum(skb, skb->mac_len);
 
-	rcu_read_lock();
-
 	action = READ_ONCE(v->tcf_action);
 
-	p = rcu_dereference(v->vlan_p);
+	p = rcu_dereference_bh(v->vlan_p);
 
 	switch (p->tcfv_action) {
 	case TCA_VLAN_ACT_POP:

@@ -61,7 +59,7 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
 	case TCA_VLAN_ACT_MODIFY:
 		/* No-op if no vlan tag (either hw-accel or in-payload) */
 		if (!skb_vlan_tagged(skb))
-			goto unlock;
+			goto out;
 		/* extract existing tag (and guarantee no hw-accel tag) */
 		if (skb_vlan_tag_present(skb)) {
 			tci = skb_vlan_tag_get(skb);

@@ -86,18 +84,15 @@ static int tcf_vlan(struct sk_buff *skb, const struct tc_action *a,
 		BUG();
 	}
 
-	goto unlock;
-
-drop:
-	action = TC_ACT_SHOT;
-	qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
-
-unlock:
-	rcu_read_unlock();
+out:
 	if (skb_at_tc_ingress(skb))
 		skb_pull_rcsum(skb, skb->mac_len);
 
 	return action;
+
+drop:
+	qstats_drop_inc(this_cpu_ptr(v->common.cpu_qstats));
+	return TC_ACT_SHOT;
 }
 
 static const struct nla_policy vlan_policy[TCA_VLAN_MAX + 1] = {
Signature · 1 line
2.17.1
author maintainer
#5 lore raw

PATCH v4 3/4 net/tc: introduce TC_ACT_REINSERT.

In reply to Paolo Abeni

This is similar TC_ACT_REDIRECT, but with a slightly different semantic:
- on ingress the mirred skbs are passed to the target device network stack without any additional check not scrubbing.
- the rcu-protected stats provided via the tcf_result struct are updated on error conditions.

This new tcfa_action value is not exposed to the user-space and can be used only internally by clsact.

v1 -> v2: do not touch TC_ACT_REDIRECT code path, introduce
a new action type instead

v2 -> v3:
- rename the new action value TC_ACT_REINJECT, update the helper accordingly
- take care of uncloned reinjected packets in XDP generic hook

v3 -> v4:
- renamed again the new action value (JiriP)

Signed-off-by Paolo Abeni <[email protected]>

---
Note: this patch still touch only overlimits, even there is some agreement to touch (also) drops on reinsert/mirred failure, but such change is independent to this series

---
 include/net/pkt_cls.h     |  3 +++
 include/net/sch_generic.h | 19 +++++++++++++++++++
 net/core/dev.c            |  6 +++++-
 3 files changed, 27 insertions(+), 1 deletion(-)
include/net/pkt_cls.h +3 -0
index a3101582f642..d64915273358 100644
@@ -7,6 +7,9 @@
 #include <net/sch_generic.h>
 #include <net/act_api.h>
 
+/* TC action not accessible from user space */
+#define TC_ACT_REINSERT		(TC_ACT_VALUE_MAX + 1)
+
 /* Basic packet classifier frontend definitions. */
 
 struct tcf_walker {
include/net/sch_generic.h +19 -0
index c9af9ce33055..70270d9c1bb1 100644
@@ -235,6 +235,12 @@ struct tcf_result {
 			u32		classid;
 		};
 		const struct tcf_proto *goto_tp;
+
+		/* used by the TC_ACT_REINSERT action */
+		struct {
+			bool		ingress;
+			struct gnet_stats_queue *qstats;
+		};
 	};
 };
 

@@ -1107,4 +1113,17 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
 void mini_qdisc_pair_init(struct mini_Qdisc_pair *miniqp, struct Qdisc *qdisc,
 			  struct mini_Qdisc __rcu **p_miniq);
 
+static inline void skb_tc_reinsert(struct sk_buff *skb, struct tcf_result *res)
+{
+	struct gnet_stats_queue *stats = res->qstats;
+	int ret;
+
+	if (res->ingress)
+		ret = netif_receive_skb(skb);
+	else
+		ret = dev_queue_xmit(skb);
+	if (ret && stats)
+		qstats_overlimit_inc(res->qstats);
+}
+
 #endif
net/core/dev.c +5 -1
index 87c42c8249ae..d5faa6449fb2 100644
@@ -4252,7 +4252,7 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 	/* Reinjected packets coming from act_mirred or similar should
 	 * not get XDP generic processing.
 	 */
-	if (skb_cloned(skb))
+	if (skb_cloned(skb) || skb->tc_redirected)
 		return XDP_PASS;
 
 	/* XDP packets must be linear and must have sufficient headroom

@@ -4602,6 +4602,10 @@ sch_handle_ingress(struct sk_buff *skb, struct packet_type **pt_prev, int *ret,
 		__skb_push(skb, skb->mac_len);
 		skb_do_redirect(skb);
 		return NULL;
+	case TC_ACT_REINSERT:
+		/* this does not scrub the packet, and updates stats on error */
+		skb_tc_reinsert(skb, &cl_res);
+		return NULL;
 	default:
 		break;
 	}
Signature · 1 line
2.17.1
author maintainer
#9 lore raw

PATCH v4 4/4 act_mirred: use TC_ACT_REINSERT when possible

In reply to Paolo Abeni

When mirred is invoked from the ingress path, and it wants to redirect the processed packet, it can now use the TC_ACT_REINSERT action, filling the tcf_result accordingly, and avoiding a per packet skb_clone().

Overall this gives a ~10% improvement in forwarding performance for the TC S/W data path and TC S/W performances are now comparable to the kernel openvswitch datapath.

v1 -> v2: use ACT_MIRRED instead of ACT_REDIRECT
v2 -> v3: updated after action rename, fixed typo into the commit
message
v3 -> v4: updated again after action rename, added more comments to
the code (JiriP), skip the optimization if the control action need to touch the tcf_result (Paolo)

Signed-off-by Paolo Abeni <[email protected]>
---
 net/sched/act_mirred.c | 55 +++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 11 deletions(-)
net/sched/act_mirred.c +44 -11
index eeb335f03102..d5f1a8102b7b 100644
@@ -25,6 +25,7 @@
 #include <net/net_namespace.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <linux/tc_act/tc_mirred.h>
 #include <net/tc_act/tc_mirred.h>
 

@@ -49,6 +50,18 @@ static bool tcf_mirred_act_wants_ingress(int action)
 	}
 }
 
+static bool tcf_mirred_can_reinsert(int action)
+{
+	switch (action) {
+	case TC_ACT_SHOT:
+	case TC_ACT_STOLEN:
+	case TC_ACT_QUEUED:
+	case TC_ACT_TRAP:
+		return true;
+	}
+	return false;
+}
+
 static void tcf_mirred_release(struct tc_action *a)
 {
 	struct tcf_mirred *m = to_mirred(a);

@@ -171,10 +184,13 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		      struct tcf_result *res)
 {
 	struct tcf_mirred *m = to_mirred(a);
+	struct sk_buff *skb2 = skb;
 	bool m_mac_header_xmit;
 	struct net_device *dev;
-	struct sk_buff *skb2;
 	int retval, err = 0;
+	bool use_reinsert;
+	bool want_ingress;
+	bool is_redirect;
 	int m_eaction;
 	int mac_len;
 

@@ -196,16 +212,25 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		goto out;
 	}
 
-	skb2 = skb_clone(skb, GFP_ATOMIC);
-	if (!skb2)
-		goto out;
+	/* we could easily avoid the clone only if called by ingress and clsact;
+	 * since we can't easily detect the clsact caller, skip clone only for
+	 * ingress - that covers the TC S/W datapath.
+	 */
+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+	use_reinsert = skb_at_tc_ingress(skb) && is_redirect &&
+		       tcf_mirred_can_reinsert(retval);
+	if (!use_reinsert) {
+		skb2 = skb_clone(skb, GFP_ATOMIC);
+		if (!skb2)
+			goto out;
+	}
 
 	/* If action's target direction differs than filter's direction,
 	 * and devices expect a mac header on xmit, then mac push/pull is
 	 * needed.
 	 */
-	if (skb_at_tc_ingress(skb) != tcf_mirred_act_wants_ingress(m_eaction) &&
-	    m_mac_header_xmit) {
+	want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
+	if (skb_at_tc_ingress(skb) != want_ingress && m_mac_header_xmit) {
 		if (!skb_at_tc_ingress(skb)) {
 			/* caught at egress, act ingress: pull mac */
 			mac_len = skb_network_header(skb) - skb_mac_header(skb);

@@ -216,15 +241,23 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		}
 	}
 
+	skb2->skb_iif = skb->dev->ifindex;
+	skb2->dev = dev;
+
 	/* mirror is always swallowed */
-	if (tcf_mirred_is_act_redirect(m_eaction)) {
+	if (is_redirect) {
 		skb2->tc_redirected = 1;
 		skb2->tc_from_ingress = skb2->tc_at_ingress;
+
+		/* let's the caller reinsert the packet, if possible */
+		if (use_reinsert) {
+			res->ingress = want_ingress;
+			res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+			return TC_ACT_REINSERT;
+		}
 	}
 
-	skb2->skb_iif = skb->dev->ifindex;
-	skb2->dev = dev;
-	if (!tcf_mirred_act_wants_ingress(m_eaction))
+	if (!want_ingress)
 		err = dev_queue_xmit(skb2);
 	else
 		err = netif_receive_skb(skb2);

@@ -232,7 +265,7 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 	if (err) {
 out:
 		qstats_overlimit_inc(this_cpu_ptr(m->common.cpu_qstats));
-		if (tcf_mirred_is_act_redirect(m_eaction))
+		if (is_redirect)
 			retval = TC_ACT_SHOT;
 	}
Signature · 1 line
2.17.1
Acked-by on v4 4/4
#10 lore raw

Re: [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible

Quoted 10 lines · Paolo Abeni

When mirred is invoked from the ingress path, and it wants to redirect the processed packet, it can now use the TC_ACT_REINSERT action, filling the tcf_result accordingly, and avoiding a per packet skb_clone().

Overall this gives a ~10% improvement in forwarding performance for the TC S/W data path and TC S/W performances are now comparable to the kernel openvswitch datapath.

Quoted 8 lines

v1 -> v2: use ACT_MIRRED instead of ACT_REDIRECT
v2 -> v3: updated after action rename, fixed typo into the commit
message
v3 -> v4: updated again after action rename, added more comments to
the code (JiriP), skip the optimization if the control action need to touch the tcf_result (Paolo)

Signed-off-by Paolo Abeni <[email protected]>

Overall it looks good to me now.

Acked-by Cong Wang <[email protected]>

Thanks!

Acked-by on v4 3/4
#6 lore raw

Re: [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.

In reply to Paolo Abeni
Quoted 20 lines · Paolo Abeni

This is similar TC_ACT_REDIRECT, but with a slightly different semantic:
- on ingress the mirred skbs are passed to the target device network stack without any additional check not scrubbing.
- the rcu-protected stats provided via the tcf_result struct are updated on error conditions.

This new tcfa_action value is not exposed to the user-space and can be used only internally by clsact.

v1 -> v2: do not touch TC_ACT_REDIRECT code path, introduce
a new action type instead

v2 -> v3:
- rename the new action value TC_ACT_REINJECT, update the helper accordingly
- take care of uncloned reinjected packets in XDP generic hook

Quoted 4 lines

v3 -> v4:
- renamed again the new action value (JiriP)

Signed-off-by Paolo Abeni <[email protected]>
Acked-by Cong Wang <[email protected]>
Quoted 4 lines

---
Note: this patch still touch only overlimits, even there is some agreement to touch (also) drops on reinsert/mirred failure, but such change is independent to this series

Totally agree.

Thanks!

#2 lore raw

Re: [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values

In reply to Paolo Abeni

Hi,

On Thu, Jul 26, 2018 at 04:34:57PM +0200, Paolo Abeni wrote:
...

net/sched/act_api.c struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, · line 909

Quoted 7 lines
net/sched/act_api.c +3 -0
@@ -895,6 +904,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
 
+	if (!tcf_action_valid(a->tcfa_action)) {
+		net_warn_ratelimited("invalid %d action value, using "
+				     "TC_ACT_UNSPEC instead", a->tcfa_action);

Now that it is reporting the error via extack, do we really need this warn net_warn?
extack will be shown as a warning by iproute2 even if the command succeeds.

net/sched/act_api.c struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, · line 911

Quoted 2 lines
net/sched/act_api.c +2 -0
+		NL_SET_ERR_MSG(extack, "invalid action value, using "
+			       "TC_ACT_UNSPEC instead");

Quoted strings shouldn't be broken down into multiple lines..

Quoted 9 lines
net/sched/act_api.c +3 -0
+		a->tcfa_action = TC_ACT_UNSPEC;
+	}
+
 	return a;
 
 err_mod:
Signature · 1 line
2.17.1
author maintainer
#3 lore raw

Re: [PATCH net-next v4 1/4] net/sched: user-space can't set unknown tcfa_action values

Quoted 8 lines · Marcelo Ricardo Leitner

Hi,

On Thu, Jul 26, 2018 at 04:34:57PM +0200, Paolo Abeni wrote:
...

Quoted 4 lines
(quoted diff) +0 -0
@@ -895,6 +904,14 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 8 lines

net/sched/act_api.c struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, · line 909

Quoted 3 lines
net/sched/act_api.c +3 -0
+	if (!tcf_action_valid(a->tcfa_action)) {
+		net_warn_ratelimited("invalid %d action value, using "
+				     "TC_ACT_UNSPEC instead", a->tcfa_action);

Now that it is reporting the error via extack, do we really need this warn net_warn?
extack will be shown as a warning by iproute2 even if the command succeeds.

That was requested by Jiri (modulo misinterpretation on my side). My understanding is that the extact will warn the whoever tryed to push the bad configuration, while the net_warn is targeting the hosts administrator.

Jiri, do you have strong opinion on this or did I misinterpret your wording/ can I drop the net_warn?

Thanks!

Quoted 4 lines

net/sched/act_api.c struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, · line 911

Quoted 2 lines
net/sched/act_api.c +2 -0
+		NL_SET_ERR_MSG(extack, "invalid action value, using "
+			       "TC_ACT_UNSPEC instead");

Quoted strings shouldn't be broken down into multiple lines..

Thanks,

will fix in v5 :(

Cheers,

Paolo

build bot
#11 lore raw

Re: [PATCH net-next v4 4/4] act_mirred: use TC_ACT_REINSERT when possible

In reply to Paolo Abeni
Thank you for the patch! Perhaps something to improve:

Hi Paolo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/TC-refactor-act_mirred-packets-re-injection/20180729-102154
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

net/sched/act_mirred.c: In function 'tcf_mirred':

>> net/sched/act_mirred.c:268:6: warning: 'is_redirect' may be used uninitialized in this function [-Wmaybe-uninitialized]
      if (is_redirect)
         ^

vim +/is_redirect +268 net/sched/act_mirred.c

182
183 static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
184 struct tcf_result *res)
185 {
186 struct tcf_mirred *m = to_mirred(a);
187 struct sk_buff *skb2 = skb;
188 bool m_mac_header_xmit;
189 struct net_device *dev;
190 int retval, err = 0;
191 bool use_reinsert;
192 bool want_ingress;
193 bool is_redirect;
194 int m_eaction;
195 int mac_len;
196
197 tcf_lastuse_update(&m->tcf_tm);
198 bstats_cpu_update(this_cpu_ptr(m->common.cpu_bstats), skb);
199
200 m_mac_header_xmit = READ_ONCE(m->tcfm_mac_header_xmit);
201 m_eaction = READ_ONCE(m->tcfm_eaction);
202 retval = READ_ONCE(m->tcf_action);
203 dev = rcu_dereference_bh(m->tcfm_dev);
204 if (unlikely(!dev)) {
205 pr_notice_once("tc mirred: target device is gone\n");
206 goto out;
207 }
208
209 if (unlikely(!(dev->flags & IFF_UP))) {
210 net_notice_ratelimited("tc mirred to Houston: device %s is down\n",
211 dev->name);
212 goto out;
213 }
214
215 /* we could easily avoid the clone only if called by ingress and clsact;
216 * since we can't easily detect the clsact caller, skip clone only for
217 * ingress - that covers the TC S/W datapath.
218 */
219 is_redirect = tcf_mirred_is_act_redirect(m_eaction);
220 use_reinsert = skb_at_tc_ingress(skb) && is_redirect &&
221 tcf_mirred_can_reinsert(retval);
222 if (!use_reinsert) {
223 skb2 = skb_clone(skb, GFP_ATOMIC);
224 if (!skb2)
225 goto out;
226 }
227
228 /* If action's target direction differs than filter's direction,
229 * and devices expect a mac header on xmit, then mac push/pull is
230 * needed.
231 */
232 want_ingress = tcf_mirred_act_wants_ingress(m_eaction);
233 if (skb_at_tc_ingress(skb) != want_ingress && m_mac_header_xmit) {
234 if (!skb_at_tc_ingress(skb)) {
235 /* caught at egress, act ingress: pull mac */
236 mac_len = skb_network_header(skb) - skb_mac_header(skb);
237 skb_pull_rcsum(skb2, mac_len);
238 } else {
239 /* caught at ingress, act egress: push mac */
240 skb_push_rcsum(skb2, skb->mac_len);
241 }
242 }
243
244 skb2->skb_iif = skb->dev->ifindex;
245 skb2->dev = dev;
246
247 /* mirror is always swallowed */
248 if (is_redirect) {
249 skb2->tc_redirected = 1;
250 skb2->tc_from_ingress = skb2->tc_at_ingress;
251
252 /* let's the caller reinsert the packet, if possible */
253 if (use_reinsert) {
254 res->ingress = want_ingress;
255 res->qstats = this_cpu_ptr(m->common.cpu_qstats);
256 return TC_ACT_REINSERT;
257 }
258 }
259
260 if (!want_ingress)
261 err = dev_queue_xmit(skb2);
262 else
263 err = netif_receive_skb(skb2);
264
265 if (err) {
266 out:
267 qstats_overlimit_inc(this_cpu_ptr(m->common.cpu_qstats));

 > 268			if (is_redirect)

269 retval = TC_ACT_SHOT;
270 }
271
272 return retval;
273 }
274

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

build bot
#7 lore raw

Re: [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.

In reply to Paolo Abeni
Thank you for the patch! Yet something to improve:

Hi Paolo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/TC-refactor-act_mirred-packets-re-injection/20180729-102154
config: i386-randconfig-n0-201829 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce:

        # save the attached .config to linux build tree
        make ARCH=i386

All errors (new ones prefixed by >>):

net//core/dev.c: In function 'netif_receive_generic_xdp':

>> net//core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
     if (skb_cloned(skb) || skb->tc_redirected)
                               ^~

vim +4255 net//core/dev.c

4241
4242 static u32 netif_receive_generic_xdp(struct sk_buff *skb,
4243 struct xdp_buff *xdp,
4244 struct bpf_prog *xdp_prog)
4245 {
4246 struct netdev_rx_queue *rxqueue;
4247 void *orig_data, *orig_data_end;
4248 u32 metalen, act = XDP_DROP;
4249 int hlen, off;
4250 u32 mac_len;
4251
4252 /* Reinjected packets coming from act_mirred or similar should
4253 * not get XDP generic processing.
4254 */

> 4255		if (skb_cloned(skb) || skb->tc_redirected)

4256 return XDP_PASS;
4257
4258 /* XDP packets must be linear and must have sufficient headroom
4259 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
4260 * native XDP provides, thus we need to do it here as well.
4261 */
4262 if (skb_is_nonlinear(skb) ||
4263 skb_headroom(skb) < XDP_PACKET_HEADROOM) {
4264 int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
4265 int troom = skb->tail + skb->data_len - skb->end;
4266
4267 /* In case we have to go down the path and also linearize,
4268 * then lets do the pskb_expand_head() work just once here.
4269 */
4270 if (pskb_expand_head(skb,
4271 hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
4272 troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
4273 goto do_drop;
4274 if (skb_linearize(skb))
4275 goto do_drop;
4276 }
4277
4278 /* The XDP program wants to see the packet starting at the MAC
4279 * header.
4280 */
4281 mac_len = skb->data - skb_mac_header(skb);
4282 hlen = skb_headlen(skb) + mac_len;
4283 xdp->data = skb->data - mac_len;
4284 xdp->data_meta = xdp->data;
4285 xdp->data_end = xdp->data + hlen;
4286 xdp->data_hard_start = skb->data - skb_headroom(skb);
4287 orig_data_end = xdp->data_end;
4288 orig_data = xdp->data;
4289
4290 rxqueue = netif_get_rxqueue(skb);
4291 xdp->rxq = &rxqueue->xdp_rxq;
4292
4293 act = bpf_prog_run_xdp(xdp_prog, xdp);
4294
4295 off = xdp->data - orig_data;
4296 if (off > 0)
4297 __skb_pull(skb, off);
4298 else if (off < 0)
4299 __skb_push(skb, -off);
4300 skb->mac_header += off;
4301
4302 /* check if bpf_xdp_adjust_tail was used. it can only "shrink"
4303 * pckt.
4304 */
4305 off = orig_data_end - xdp->data_end;
4306 if (off != 0) {
4307 skb_set_tail_pointer(skb, xdp->data_end - xdp->data);
4308 skb->len -= off;
4309
4310 }
4311
4312 switch (act) {
4313 case XDP_REDIRECT:
4314 case XDP_TX:
4315 __skb_push(skb, mac_len);
4316 break;
4317 case XDP_PASS:
4318 metalen = xdp->data - xdp->data_meta;
4319 if (metalen)
4320 skb_metadata_set(skb, metalen);
4321 break;
4322 default:
4323 bpf_warn_invalid_xdp_action(act);
4324 /* fall through */
4325 case XDP_ABORTED:
4326 trace_xdp_exception(skb->dev, xdp_prog, act);
4327 /* fall through */
4328 case XDP_DROP:
4329 do_drop:
4330 kfree_skb(skb);
4331 break;
4332 }
4333
4334 return act;
4335 }
4336

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

build bot
#8 lore raw

Re: [PATCH net-next v4 3/4] net/tc: introduce TC_ACT_REINSERT.

In reply to Paolo Abeni
Thank you for the patch! Perhaps something to improve:

Hi Paolo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url: https://github.com/0day-ci/linux/commits/Paolo-Abeni/TC-refactor-act_mirred-packets-re-injection/20180729-102154
config: x86_64-randconfig-u0-07291027 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64

All warnings (new ones prefixed by >>):

In file included from arch/x86/include/asm/current.h:5:0,

                    from include/linux/sched.h:12,
                    from include/linux/uaccess.h:5,
                    from net/core/dev.c:75:
   net/core/dev.c: In function 'netif_receive_generic_xdp':
   net/core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
     if (skb_cloned(skb) || skb->tc_redirected)
                               ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^
>> net/core/dev.c:4255:2: note: in expansion of macro 'if'
     if (skb_cloned(skb) || skb->tc_redirected)
     ^
   net/core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
     if (skb_cloned(skb) || skb->tc_redirected)
                               ^
   include/linux/compiler.h:58:42: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                             ^
>> net/core/dev.c:4255:2: note: in expansion of macro 'if'
     if (skb_cloned(skb) || skb->tc_redirected)
     ^
   net/core/dev.c:4255:28: error: 'struct sk_buff' has no member named 'tc_redirected'
     if (skb_cloned(skb) || skb->tc_redirected)
                               ^
   include/linux/compiler.h:69:16: note: in definition of macro '__trace_if'
      ______r = !!(cond);     \
                   ^
>> net/core/dev.c:4255:2: note: in expansion of macro 'if'
     if (skb_cloned(skb) || skb->tc_redirected)
     ^

vim +/if +4255 net/core/dev.c

4241
4242 static u32 netif_receive_generic_xdp(struct sk_buff *skb,
4243 struct xdp_buff *xdp,
4244 struct bpf_prog *xdp_prog)
4245 {
4246 struct netdev_rx_queue *rxqueue;
4247 void *orig_data, *orig_data_end;
4248 u32 metalen, act = XDP_DROP;
4249 int hlen, off;
4250 u32 mac_len;
4251
4252 /* Reinjected packets coming from act_mirred or similar should
4253 * not get XDP generic processing.
4254 */

> 4255		if (skb_cloned(skb) || skb->tc_redirected)

4256 return XDP_PASS;
4257
4258 /* XDP packets must be linear and must have sufficient headroom
4259 * of XDP_PACKET_HEADROOM bytes. This is the guarantee that also
4260 * native XDP provides, thus we need to do it here as well.
4261 */
4262 if (skb_is_nonlinear(skb) ||
4263 skb_headroom(skb) < XDP_PACKET_HEADROOM) {
4264 int hroom = XDP_PACKET_HEADROOM - skb_headroom(skb);
4265 int troom = skb->tail + skb->data_len - skb->end;
4266
4267 /* In case we have to go down the path and also linearize,
4268 * then lets do the pskb_expand_head() work just once here.
4269 */
4270 if (pskb_expand_head(skb,
4271 hroom > 0 ? ALIGN(hroom, NET_SKB_PAD) : 0,
4272 troom > 0 ? troom + 128 : 0, GFP_ATOMIC))
4273 goto do_drop;
4274 if (skb_linearize(skb))
4275 goto do_drop;
4276 }
4277
4278 /* The XDP program wants to see the packet starting at the MAC
4279 * header.
4280 */
4281 mac_len = skb->data - skb_mac_header(skb);
4282 hlen = skb_headlen(skb) + mac_len;
4283 xdp->data = skb->data - mac_len;
4284 xdp->data_meta = xdp->data;
4285 xdp->data_end = xdp->data + hlen;
4286 xdp->data_hard_start = skb->data - skb_headroom(skb);
4287 orig_data_end = xdp->data_end;
4288 orig_data = xdp->data;
4289
4290 rxqueue = netif_get_rxqueue(skb);
4291 xdp->rxq = &rxqueue->xdp_rxq;
4292
4293 act = bpf_prog_run_xdp(xdp_prog, xdp);
4294
4295 off = xdp->data - orig_data;
4296 if (off > 0)
4297 __skb_pull(skb, off);
4298 else if (off < 0)
4299 __skb_push(skb, -off);
4300 skb->mac_header += off;
4301
4302 /* check if bpf_xdp_adjust_tail was used. it can only "shrink"
4303 * pckt.
4304 */
4305 off = orig_data_end - xdp->data_end;
4306 if (off != 0) {
4307 skb_set_tail_pointer(skb, xdp->data_end - xdp->data);
4308 skb->len -= off;
4309
4310 }
4311
4312 switch (act) {
4313 case XDP_REDIRECT:
4314 case XDP_TX:
4315 __skb_push(skb, mac_len);
4316 break;
4317 case XDP_PASS:
4318 metalen = xdp->data - xdp->data_meta;
4319 if (metalen)
4320 skb_metadata_set(skb, metalen);
4321 break;
4322 default:
4323 bpf_warn_invalid_xdp_action(act);
4324 /* fall through */
4325 case XDP_ABORTED:
4326 trace_xdp_exception(skb->dev, xdp_prog, act);
4327 /* fall through */
4328 case XDP_DROP:
4329 do_drop:
4330 kfree_skb(skb);
4331 break;
4332 }
4333
4334 return act;
4335 }
4336

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation