Skip to contentSkip to conversation

TC: refactor act_mirred packets re-injection

PATCH Superseded Paolo Abeni 6 participants 43 messages Jul 24, 2018 – Jul 27, 2018

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

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

v3 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 3 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 value 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

Paolo Abeni (5):
tc/act: user space can't use TC_ACT_REDIRECT directly 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_REINJECT.
act_mirred: use TC_ACT_REINJECT 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          | 15 +++++++++++++-
 net/sched/act_csum.c         | 12 +++---------
 net/sched/act_ife.c          |  5 +----
 net/sched/act_mirred.c       | 38 ++++++++++++++++++++++++------------
 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, 98 insertions(+), 70 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH v3 1/5 tc/act: user space can't use TC_ACT_REDIRECT directly

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

Signed-off-by Paolo Abeni <[email protected]>
---
 net/sched/act_api.c | 5 +++++
 1 file changed, 5 insertions(+)
net/sched/act_api.c +5 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
 
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");
+		a->tcfa_action = TC_ACT_UNSPEC;
+	}
+
 	return a;
 
 err_mod:
Signature · 1 line
2.17.1
maintainer
#2 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

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

Quoted 11 lines · Paolo Abeni
net/sched/act_api.c +5 -0
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
  		}
  	}
 
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");
+		a->tcfa_action = TC_ACT_UNSPEC;
+	}
+

Why not just reject the rule instead of changing the code underneath the user?

cheers,
jamal

maintainer
#3 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

In reply to Paolo Abeni
Quoted 21 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644

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

Quoted 5 lines
net/sched/act_api.c +2 -0
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
 
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

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
#4 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

Hi,

Quoted 25 lines · Jiri Pirko
Quoted 24 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 7 lines

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

Quoted 2 lines
net/sched/act_api.c +2 -0
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

Jiri, Jamal, thank you for the feedback.

Please allow me to answer both of you here, since you raised similar concers.

I thought about rejecting the action, but that change of behavior could break some users, as currently most kind of invalid tcfa_action values are simply accepted.

If there is consensus about it, I can simply fail.

Thanks,

Paolo

maintainer
#5 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

Quoted 38 lines · Wed, Jul 25, 2018 at 02:54:04PM CEST, [email protected] wrote:

Hi,

Quoted 25 lines · Jiri Pirko
Quoted 24 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 7 lines

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

Quoted 2 lines
net/sched/act_api.c +2 -0
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

Jiri, Jamal, thank you for the feedback.

Quoted 8 lines

Please allow me to answer both of you here, since you raised similar concers.

I thought about rejecting the action, but that change of behavior could break some users, as currently most kind of invalid tcfa_action values are simply accepted.

If there is consensus about it, I can simply fail.

Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it really has no meaning for anyone to use it throughout its whole history. I would vote for "fail", yet I admit that I am usually alone in opinion about similar uapi changes :)

Quoted 5 lines

Thanks,

Paolo

author maintainer
#6 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

Quoted 46 lines · Jiri Pirko
Quoted 45 lines · Wed, Jul 25, 2018 at 02:54:04PM CEST, [email protected] wrote:

Hi,

Quoted 25 lines · Jiri Pirko
Quoted 24 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 7 lines

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

Quoted 2 lines
net/sched/act_api.c +2 -0
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

Jiri, Jamal, thank you for the feedback.

Please allow me to answer both of you here, since you raised similar concers.

I thought about rejecting the action, but that change of behavior could break some users, as currently most kind of invalid tcfa_action values are simply accepted.

Quoted 6 lines
Quoted 1 line

If there is consensus about it, I can simply fail.

Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it really has no meaning for anyone to use it throughout its whole history. I would vote for "fail", yet I admit that I am usually alone in opinion about similar uapi changes :)

Since even Jamal suggested the same, unless someone else voice some opposition soon, in v4 I'll opt for rejecting actions using TC_ACT_REDIRECT.

Thanks,

Paolo

author maintainer
#7 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

Quoted 9 lines · Paolo Abeni
Quoted 4 lines · Jiri Pirko

Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it really has no meaning for anyone to use it throughout its whole history. I would vote for "fail", yet I admit that I am usually alone in opinion about similar uapi changes :)

Since even Jamal suggested the same, unless someone else voice some opposition soon, in v4 I'll opt for rejecting actions using TC_ACT_REDIRECT.

Thinking again about it, I'm going to drop this patch from this series. Since v2 is not strictly needed anymore and actually quite unrelated.

Thanks and sorry for the reiterated noise ;)

Paolo

#8 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

In reply to Paolo Abeni
Quoted 41 lines · Paolo Abeni
Quoted 40 lines · Jiri Pirko
Quoted 39 lines · Wed, Jul 25, 2018 at 02:54:04PM CEST, [email protected] wrote:
Quoted 25 lines · Jiri Pirko
Quoted 24 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 7 lines

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

Quoted 2 lines
net/sched/act_api.c +2 -0
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

Jiri, Jamal, thank you for the feedback.

Please allow me to answer both of you here, since you raised similar concers.

Quoted 8 lines
Quoted 8 lines
Quoted 5 lines

I thought about rejecting the action, but that change of behavior could break some users, as currently most kind of invalid tcfa_action values are simply accepted.

If there is consensus about it, I can simply fail.

Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it really has no meaning for anyone to use it throughout its whole history.

That claim is completely wrong.

Quoted 6 lines
Quoted 2 lines

I would vote for "fail", yet I admit that I am usually alone in opinion about similar uapi changes :)

Since even Jamal suggested the same, unless someone else voice some opposition soon, in v4 I'll opt for rejecting actions using TC_ACT_REDIRECT.

You should probably leave out act_bpf from that rejection as there may be a small chance that users could potentially use it as default action.

Thanks,
Daniel

maintainer
#9 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

Quoted 46 lines · Wed, Jul 25, 2018 at 06:29:54PM CEST, [email protected] wrote:
Quoted 45 lines · Paolo Abeni
Quoted 44 lines · Jiri Pirko
Quoted 43 lines · Wed, Jul 25, 2018 at 02:54:04PM CEST, [email protected] wrote:
Quoted 25 lines · Jiri Pirko
Quoted 24 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 7 lines

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

Quoted 2 lines
net/sched/act_api.c +2 -0
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

Jiri, Jamal, thank you for the feedback.

Please allow me to answer both of you here, since you raised similar concers.

I thought about rejecting the action, but that change of behavior could break some users, as currently most kind of invalid tcfa_action values are simply accepted.

Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line

If there is consensus about it, I can simply fail.

Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it really has no meaning for anyone to use it throughout its whole history.

That claim is completely wrong.

Why? Does addition of TC_ACT_REDIRECT to uapi have any meaning?

Quoted 13 lines
Quoted 7 lines
Quoted 3 lines

I would vote for "fail", yet I admit that I am usually alone in opinion about similar uapi changes :)

Since even Jamal suggested the same, unless someone else voice some opposition soon, in v4 I'll opt for rejecting actions using TC_ACT_REDIRECT.

You should probably leave out act_bpf from that rejection as there may be a small chance that users could potentially use it as default action.

Thanks,
Daniel

#10 lore raw

Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly

Quoted 47 lines · Jiri Pirko
Quoted 46 lines · Wed, Jul 25, 2018 at 06:29:54PM CEST, [email protected] wrote:
Quoted 45 lines · Paolo Abeni
Quoted 44 lines · Jiri Pirko
Quoted 43 lines · Wed, Jul 25, 2018 at 02:54:04PM CEST, [email protected] wrote:
Quoted 25 lines · Jiri Pirko
Quoted 24 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:

Only cls_bpf and act_bpf can safely use such value. If a generic action is configured by user space to return TC_ACT_REDIRECT, the usually visible behavior is passing the skb up the stack - as for unknown action, but, with complex configuration, more random results can be obtained.

This patch forcefully converts TC_ACT_REDIRECT to TC_ACT_UNSPEC at action init time, making the kernel behavior more consistent.

v1 -> v3: use TC_ACT_UNSPEC instead of a newly definied act value

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

---
net/sched/act_api.c | 5 +++++

1 file changed, 5 insertions(+)
net/sched/act_api.c +0 -0
index 148a89ab789b..24b5534967fe 100644
@@ -895,6 +895,11 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 		}
 	}
Quoted 7 lines

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

Quoted 2 lines
net/sched/act_api.c +2 -0
+	if (a->tcfa_action == TC_ACT_REDIRECT) {
+		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");

Can't you push this warning through extack?

But, wouldn't it be more appropriate to fail here? User is passing invalid configuration....

Jiri, Jamal, thank you for the feedback.

Please allow me to answer both of you here, since you raised similar concers.

I thought about rejecting the action, but that change of behavior could break some users, as currently most kind of invalid tcfa_action values are simply accepted.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line

If there is consensus about it, I can simply fail.

Well it was obviously wrong to expose TC_ACT_REDIRECT to uapi and it really has no meaning for anyone to use it throughout its whole history.

That claim is completely wrong.

Why? Does addition of TC_ACT_REDIRECT to uapi have any meaning?

BPF programs return TC_ACT_* as a verdict which is then further processed, including TC_ACT_REDIRECT. Hence, it's a contract for them similarly as e.g. helper enums (BPF_FUNC_*) and other things they make use of.

Cheers,
Daniel

author maintainer
#11 lore raw

PATCH v3 2/5 net/sched: user-space can't set unknown tcfa_action values

In reply to Paolo Abeni

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.

Signed-off-by Paolo Abeni <[email protected]>
---
 include/uapi/linux/pkt_cls.h |  6 ++++--
 net/sched/act_api.c          | 10 +++++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)
include/uapi/linux/pkt_cls.h +4 -2
index c4262d911596..c8a24861d4c8 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 +9 -1
index 24b5534967fe..5044f4809b37 100644
@@ -798,6 +798,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
+	int opcode;
 	int err;
 
 	if (name == NULL) {

@@ -884,7 +885,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (err != ACT_P_CREATED)
 		module_put(a_o->owner);
 
-	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
+	opcode = TC_ACT_EXT_OPCODE(a->tcfa_action);
+	if (opcode == TC_ACT_GOTO_CHAIN) {
 		err = tcf_action_goto_chain_init(a, tp);
 		if (err) {
 			struct tc_action *actions[] = { a, NULL };

@@ -898,6 +900,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (a->tcfa_action == TC_ACT_REDIRECT) {
 		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");
 		a->tcfa_action = TC_ACT_UNSPEC;
+	} else if ((!opcode && a->tcfa_action > TC_ACT_VALUE_MAX) ||
+		   (opcode && opcode > TC_ACT_EXT_OPCODE_MAX &&
+		    a->tcfa_action != TC_ACT_UNSPEC)) {
+		net_warn_ratelimited("invalid %d action value",
+				     a->tcfa_action);
+		a->tcfa_action = TC_ACT_UNSPEC;
 	}
 
 	return a;
Signature · 1 line
2.17.1
maintainer
#12 lore raw

Re: [PATCH net-next v3 2/5] net/sched: user-space can't set unknown tcfa_action values

Quoted 68 lines · Tue, Jul 24, 2018 at 10:06:40PM CEST, [email protected] wrote:

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.

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

---
include/uapi/linux/pkt_cls.h | 6 ++++--
net/sched/act_api.c | 10 +++++++++-

2 files changed, 13 insertions(+), 3 deletions(-)
include/uapi/linux/pkt_cls.h +4 -2
index c4262d911596..c8a24861d4c8 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 +3 -1
index 24b5534967fe..5044f4809b37 100644
@@ -798,6 +798,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	char act_name[IFNAMSIZ];
 	struct nlattr *tb[TCA_ACT_MAX + 1];
 	struct nlattr *kind;
+	int opcode;
 	int err;
 
 	if (name == NULL) {

@@ -884,7 +885,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (err != ACT_P_CREATED)
 		module_put(a_o->owner);
 
-	if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN)) {
+	opcode = TC_ACT_EXT_OPCODE(a->tcfa_action);
+	if (opcode == TC_ACT_GOTO_CHAIN) {
 		err = tcf_action_goto_chain_init(a, tp);
 		if (err) {
 			struct tc_action *actions[] = { a, NULL };

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

Quoted 9 lines
net/sched/act_api.c +6 -0
@@ -898,6 +900,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,
 	if (a->tcfa_action == TC_ACT_REDIRECT) {
 		net_warn_ratelimited("TC_ACT_REDIRECT can't be used directly");
 		a->tcfa_action = TC_ACT_UNSPEC;
+	} else if ((!opcode && a->tcfa_action > TC_ACT_VALUE_MAX) ||
+		   (opcode && opcode > TC_ACT_EXT_OPCODE_MAX &&
+		    a->tcfa_action != TC_ACT_UNSPEC)) {
+		net_warn_ratelimited("invalid %d action value",
+				     a->tcfa_action);
+		a->tcfa_action = TC_ACT_UNSPEC;

Maybe this could be a separate helper function?

Also, the warn might go along with extack to user too.

Otherwise, this looks fine to me.

Quoted 6 lines
net/sched/act_api.c +0 -0
 	}
 
 	return a;
Signature · 1 line
2.17.1
author maintainer
#13 lore raw

PATCH v3 3/5 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 is 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 _bh variant, cleans up a bit the surronding 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

Signed-off-by Paolo Abeni <[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 7432100027b7..056dc1083aa3 100644
@@ -275,6 +275,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 bd232d3bd022..4f092fbd1e20 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 = params->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 3ec585d58762..1ab959ef3547 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
maintainer
#14 lore raw

Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

Quoted 4 lines · Paolo Abeni

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

Quoted 8 lines

This change drops the per action RCU lock, replace the accessors with _bh variant, cleans up a bit the surronding 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.

This and 2/5 seems to stand on their own merit.

cheers,
jamal

#15 lore raw

Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

Quoted 10 lines · Jamal Hadi Salim
Quoted 9 lines · Paolo Abeni

Each lockless action currently does its own RCU locking in ->act(). This is 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 _bh variant, cleans up a bit the surronding code and documents the RCU status in the relevant header.
No functional nor performance change is intended.

Quoted 5 lines
Quoted 3 lines

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

This and 2/5 seems to stand on their own merit.

So does 1/5, I think.

Quoted 3 lines

cheers,
jamal

maintainer Acked-by on v3 3/5
#16 lore raw

Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback

In reply to Paolo Abeni
Quoted 6 lines · Tue, Jul 24, 2018 at 10:06:41PM CEST, [email protected] wrote:

Each lockless action currently does its own RCU locking in ->act(). This is 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 _bh variant, cleans up a bit the surronding code and documents

s/surronding/surrounding/

Quoted 11 lines

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

Signed-off-by Paolo Abeni <[email protected]>
Acked-by Jiri Pirko <[email protected]>
author maintainer
#17 lore raw

PATCH v3 4/5 net/tc: introduce TC_ACT_REINJECT.

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

Signed-off-by Paolo Abeni <[email protected]>
---
 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 2081e4219f81..36ccfe2a303a 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_REINJECT		(TC_ACT_VALUE_MAX + 1)
+
 /* Basic packet classifier frontend definitions. */
 
 struct tcf_walker {
include/net/sch_generic.h +19 -0
index 056dc1083aa3..95e81a70f549 100644
@@ -235,6 +235,12 @@ struct tcf_result {
 			u32		classid;
 		};
 		const struct tcf_proto *goto_tp;
+
+		/* used by the TC_ACT_REINJECT action */
+		struct {
+			bool		ingress;
+			struct gnet_stats_queue *qstats;
+		};
 	};
 };
 

@@ -1091,4 +1097,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_reinject(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 14a748ee8cc9..826ec74fe1d9 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_REINJECT:
+		/* this does not scrub the packet, and updates stats on error */
+		skb_tc_reinject(skb, &cl_res);
+		return NULL;
 	default:
 		break;
 	}
Signature · 1 line
2.17.1
#18 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

include/net/sch_generic.h void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp, · line 1110

Quoted 11 lines · Paolo Abeni
include/net/sch_generic.h +11 -0
+static inline void skb_tc_reinject(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);

Why increasing overlimit? Overlimit is typically increased by traffic shapers to indicate there is no bandwidth to send out the packet.

I fail to understand why overlimit is increased in your case here. I guess you want to increase 'drops' instead.

#19 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 18 lines · Cong Wang

include/net/sch_generic.h void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp, · line 1110

Quoted 11 lines · Paolo Abeni
include/net/sch_generic.h +11 -0
+static inline void skb_tc_reinject(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);

Why increasing overlimit? Overlimit is typically increased by traffic shapers to indicate there is no bandwidth to send out the packet.

Quoted 2 lines

I fail to understand why overlimit is increased in your case here. I guess you want to increase 'drops' instead.

Hmm, actually the current mirred code increases overlimit too. But I still don't think it makes sense.

author maintainer
#20 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 19 lines · Cong Wang
Quoted 18 lines · Cong Wang

include/net/sch_generic.h void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp, · line 1110

Quoted 11 lines · Paolo Abeni
include/net/sch_generic.h +11 -0
+static inline void skb_tc_reinject(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);

Why increasing overlimit? Overlimit is typically increased by traffic shapers to indicate there is no bandwidth to send out the packet.

Quoted 5 lines
Quoted 2 lines

I fail to understand why overlimit is increased in your case here. I guess you want to increase 'drops' instead.

Hmm, actually the current mirred code increases overlimit too. But I still don't think it makes sense.

Yep, I chose to increment 'overlimits' to preserve the current mirred semantic.

AFAICS, that was first introduced with:

commit 8919bc13e8d92c5b082c5c0321567383a071f5bc

Author: Jamal Hadi Salim <[email protected]>
Date:   Mon Aug 15 05:25:40 2011 +0000

net_sched: fix port mirror/redirect stats reporting

Likely increasing 'drops' would be "better", but I'm unsure we can change this established behavior without affecting some user.

Anyway, I'm fine either way. Please advice.

Cheers,

Paolo

maintainer
#21 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 1 line · Paolo Abeni

On Tue, 2018-07-24 at 13:50 -0700, Cong Wang wrote:

[..]

Quoted 11 lines
Quoted 5 lines
Quoted 2 lines

I fail to understand why overlimit is increased in your case here. I guess you want to increase 'drops' instead.

Hmm, actually the current mirred code increases overlimit too. But I still don't think it makes sense.

Yep, I chose to increment 'overlimits' to preserve the current mirred semantic.

AFAICS, that was first introduced with:

Quoted 9 lines

commit 8919bc13e8d92c5b082c5c0321567383a071f5bc

Author: Jamal Hadi Salim <[email protected]>
Date:   Mon Aug 15 05:25:40 2011 +0000

net_sched: fix port mirror/redirect stats reporting

Likely increasing 'drops' would be "better", but I'm unsure we can change this established behavior without affecting some user.

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

cheers,
jamal

author maintainer
#22 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 21 lines · Jamal Hadi Salim
Quoted 1 line · Paolo Abeni

On Tue, 2018-07-24 at 13:50 -0700, Cong Wang wrote:

[..]

Quoted 17 lines
Quoted 5 lines
Quoted 2 lines

I fail to understand why overlimit is increased in your case here. I guess you want to increase 'drops' instead.

Hmm, actually the current mirred code increases overlimit too. But I still don't think it makes sense.

Yep, I chose to increment 'overlimits' to preserve the current mirred semantic.

AFAICS, that was first introduced with:

commit 8919bc13e8d92c5b082c5c0321567383a071f5bc

Author: Jamal Hadi Salim <[email protected]>
Date:   Mon Aug 15 05:25:40 2011 +0000

net_sched: fix port mirror/redirect stats reporting

Quoted 8 lines
Quoted 3 lines

Likely increasing 'drops' would be "better", but I'm unsure we can change this established behavior without affecting some user.

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

Double-checking to avoid misinterepration on my side: you are ok with keeping the 'overlimits' increment, right?

Thanks,

Paolo

maintainer
#23 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 9 lines · Paolo Abeni
Quoted 4 lines · Jamal Hadi Salim

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

Double-checking to avoid misinterepration on my side: you are ok with keeping the 'overlimits' increment, right?

Yes.

cheers,
jamal

#24 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

In reply to Jamal Hadi Salim
Quoted 5 lines · Jamal Hadi Salim

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

There must be a limit for "overlimit" to make sense. There is no limit in mirred action's context, probably there is only such a limit in act_police. So, all rest should not touch overlimit.

#25 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 10 lines · Cong Wang
Quoted 5 lines · Jamal Hadi Salim

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

There must be a limit for "overlimit" to make sense. There is no limit in mirred action's context, probably there is only such a limit in act_police. So, all rest should not touch overlimit.

+1

maintainer
#26 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 8 lines · Marcelo Ricardo Leitner
Quoted 7 lines · Cong Wang
Quoted 5 lines · Jamal Hadi Salim

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

Quoted 6 lines
Quoted 3 lines

There must be a limit for "overlimit" to make sense. There is no limit in mirred action's context, probably there is only such a limit in act_police. So, all rest should not touch overlimit.

+1

I agree we should at least record drop count(unrelated patch though). we should keep overlimit (for no other reason other than this has been around for at least 15 years).

On why "overlimit"? It is just a name for a counter that is useless for most actions (but was still being transfered to user space). It is the closest counter to counting "this failed because of runtime errors" as opposed to "user asked us to drop this".

Probably a good alternative is to make a very small stats v3 structure (we have migrated stats structures before) and extend for each action/classifier/qdisc to add its extra counters using XSTATS.

Note:
If you are _mirroring_ packets - incrementing the drop counter is misleading because the packet is not dropped by the system. i.e the qdisc will not record it as dropped; it should for redirect policy. It is useful to be able to tell
them apart when you are collecting analytics just for actions. (if youve worked on a massive amount of machines you'll appreciate being able to debug by looking at counters that reduce ambiguity).

cheers,
jamal

#27 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Quoted 21 lines · Jamal Hadi Salim
Quoted 8 lines · Marcelo Ricardo Leitner
Quoted 7 lines · Cong Wang
Quoted 5 lines · Jamal Hadi Salim

Those changes were there from the beginning (above patch did not introduce them).
IIRC, the reason was to distinguish between policy intended drops and drops because of errors.

Quoted 6 lines
Quoted 3 lines

There must be a limit for "overlimit" to make sense. There is no limit in mirred action's context, probably there is only such a limit in act_police. So, all rest should not touch overlimit.

+1

I agree we should at least record drop count(unrelated patch though). we should keep overlimit (for no other reason other than this has been around for at least 15 years).

Quoted 8 lines

On why "overlimit"? It is just a name for a counter that is useless for most actions (but was still being transfered to user space). It is the closest counter to counting "this failed because of runtime errors" as opposed to "user asked us to drop this".

Probably a good alternative is to make a very small stats v3 structure (we have migrated stats structures before) and extend for each action/classifier/qdisc to add its extra counters using XSTATS.

Agreed.

Quoted 7 lines

Note:
If you are _mirroring_ packets - incrementing the drop counter is misleading because the packet is not dropped by the system. i.e the qdisc will not record it as dropped; it should for redirect policy. It is useful to be able to tell
them apart when you are collecting analytics just for actions.

Sounds like we just need another counter rather than re-using overlimit or drops.

Quoted 3 lines

(if youve worked on a massive amount of machines you'll appreciate being able to debug by looking at counters that reduce ambiguity).

Yes, this is how I found out the overlimit of htb qdisc is inaccurate or misleading, instead the one in htb class is accurate, see:

commit 3c75f6ee139d464351f8ab77a042dd3635769d98

Author: Eric Dumazet <[email protected]>
Date:   Mon Sep 18 12:36:22 2017 -0700

net_sched: sch_htb: add per class overlimits counter

maintainer
#28 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

In reply to Paolo Abeni

+Cc Shmulik

Paolo - please also run the tdc tests (and add anymore if you feel they dont do coverage to your changes)

Quoted 31 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

Signed-off-by Paolo Abeni <[email protected]>
---
  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 +0 -0
index 2081e4219f81..36ccfe2a303a 100644

include/net/pkt_cls.h line 11

Quoted 5 lines
include/net/pkt_cls.h +2 -0
@@ -7,6 +7,9 @@
  #include <net/sch_generic.h>
  #include <net/act_api.h>
 
+/* TC action not accessible from user space */
+#define TC_ACT_REINJECT		(TC_ACT_VALUE_MAX + 1)

Lets say in the future we add a new opcode. Will old kernel, new iproute2 (new value) work? Maybe use a negative number below -1.
I am honestly still unclear about introducing this new code.
Could you not use the result code to carry sufficient info to indicate the intent?

cheers,
jamal

Quoted 64 lines
include/net/pkt_cls.h +1 -0
+
  /* Basic packet classifier frontend definitions. */
 
  struct tcf_walker {
include/net/sch_generic.h +19 -0
index 056dc1083aa3..95e81a70f549 100644
@@ -235,6 +235,12 @@ struct tcf_result {
  			u32		classid;
  		};
  		const struct tcf_proto *goto_tp;
+
+		/* used by the TC_ACT_REINJECT action */
+		struct {
+			bool		ingress;
+			struct gnet_stats_queue *qstats;
+		};
  	};
  };
 

@@ -1091,4 +1097,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_reinject(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 14a748ee8cc9..826ec74fe1d9 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_REINJECT:
+		/* this does not scrub the packet, and updates stats on error */
+		skb_tc_reinject(skb, &cl_res);
+		return NULL;
  	default:
  		break;
  	}
maintainer
#29 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

Wed, Jul 25, 2018 at 02:16:12PM CEST, [email protected] wrote:

[...]

Quoted 4 lines
Quoted 4 lines
include/net/pkt_cls.h +0 -0
index 2081e4219f81..36ccfe2a303a 100644
Quoted 8 lines

include/net/pkt_cls.h line 10

Quoted 5 lines
include/net/pkt_cls.h +2 -0
@@ -7,6 +7,9 @@
  #include <net/sch_generic.h>
  #include <net/act_api.h>
+/* TC action not accessible from user space */
+#define TC_ACT_REINJECT		(TC_ACT_VALUE_MAX + 1)

Lets say in the future we add a new opcode.
Will old kernel, new iproute2 (new value) work?

This is safe. See patch #2.

author maintainer
#30 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

In reply to Jamal Hadi Salim

Hi,

Quoted 4 lines · Jamal Hadi Salim

+Cc Shmulik

Paolo - please also run the tdc tests (and add anymore if you feel they dont do coverage to your changes)

I run successfully tdc tests on a patched before posting. I plan to rerun them before posting the v4.

Quoted 35 lines · Paolo Abeni
Quoted 34 lines

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

Signed-off-by Paolo Abeni <[email protected]>
---
  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 +0 -0
index 2081e4219f81..36ccfe2a303a 100644
@@ -7,6 +7,9 @@
  #include <net/sch_generic.h>
  #include <net/act_api.h>
Quoted 5 lines

include/net/pkt_cls.h line 11

Quoted 2 lines
include/net/pkt_cls.h +2 -0
+/* TC action not accessible from user space */
+#define TC_ACT_REINJECT		(TC_ACT_VALUE_MAX + 1)

Lets say in the future we add a new opcode.
Will old kernel, new iproute2 (new value) work?

It will works as it currently does in similar situation: opcode unknown to the kernel are treated as TC_ACT_UNSPEC even now.

Cheers,

Paolo

maintainer
#31 lore raw

Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.

In reply to Paolo Abeni
Quoted 60 lines · Tue, Jul 24, 2018 at 10:06:42PM CEST, [email protected] wrote:

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

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

---

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 2081e4219f81..36ccfe2a303a 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_REINJECT		(TC_ACT_VALUE_MAX + 1)
+
 /* Basic packet classifier frontend definitions. */
 
 struct tcf_walker {
include/net/sch_generic.h +6 -0
index 056dc1083aa3..95e81a70f549 100644
@@ -235,6 +235,12 @@ struct tcf_result {
 			u32		classid;
 		};
 		const struct tcf_proto *goto_tp;
+
+		/* used by the TC_ACT_REINJECT action */
+		struct {
+			bool		ingress;
+			struct gnet_stats_queue *qstats;
+		};
 	};
 };
 

@@ -1091,4 +1097,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);

include/net/sch_generic.h void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp, · line 1108

Quoted 10 lines
include/net/sch_generic.h +9 -0
 
+static inline void skb_tc_reinject(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);

Hmm. "reinject" by the name tells me that the packet should be injected again. By "inject", I understand beginning of the rx path. However, this does xmit as well :/ It is a bit misleading. Maybe "reinsert" would sound better?

Quoted 32 lines
include/net/sch_generic.h +4 -0
+	if (ret && stats)
+		qstats_overlimit_inc(res->qstats);
+}
+
 #endif
net/core/dev.c +5 -1
index 14a748ee8cc9..826ec74fe1d9 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_REINJECT:
+		/* this does not scrub the packet, and updates stats on error */
+		skb_tc_reinject(skb, &cl_res);
+		return NULL;
 	default:
 		break;
 	}
Signature · 1 line
2.17.1
author maintainer
#32 lore raw

PATCH v3 5/5 act_mirred: use TC_ACT_REINJECT 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_REINJECT 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 a typo into the commit
message

Signed-off-by Paolo Abeni <[email protected]>
---
 net/sched/act_mirred.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)
net/sched/act_mirred.c +24 -10
index eeb335f03102..368187312136 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>
 

@@ -171,10 +172,12 @@ 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 want_ingress;
+	bool is_redirect;
 	int m_eaction;
 	int mac_len;
 

@@ -196,16 +199,19 @@ 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;
+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+	if (!skb_at_tc_ingress(skb) || !is_redirect) {
+		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 +222,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 reinject the packet, if possible */
+		if (skb_at_tc_ingress(skb)) {
+			res->ingress = want_ingress;
+			res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+			return TC_ACT_REINJECT;
+		}
 	}
 
-	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);
Signature · 1 line
2.17.1
#33 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

net/sched/act_mirred.c static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, · line 238

Quoted 7 lines · Paolo Abeni
net/sched/act_mirred.c +7 -0
+
+               /* let's the caller reinject the packet, if possible */
+               if (skb_at_tc_ingress(skb)) {
+                       res->ingress = want_ingress;
+                       res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+                       return TC_ACT_REINJECT;
+               }

Looks good to me, but here we no longer return user-specified return value here, I am sure it is safe for TC_ACT_STOLEN, but I am not sure if it is safe for other values, like TC_ACT_RECLASSIFY.

Jamal, is there any use case of returning !TC_ACT_STOLEN for ingress redirections?

Thanks.

author maintainer
#34 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

Quoted 9 lines · Cong Wang

net/sched/act_mirred.c static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, · line 238

Quoted 7 lines · Paolo Abeni
net/sched/act_mirred.c +7 -0
+
+               /* let's the caller reinject the packet, if possible */
+               if (skb_at_tc_ingress(skb)) {
+                       res->ingress = want_ingress;
+                       res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+                       return TC_ACT_REINJECT;
+               }
Quoted 3 lines

Looks good to me, but here we no longer return user-specified return value here, I am sure it is safe for TC_ACT_STOLEN, but I am not sure if it is safe for other values, like TC_ACT_RECLASSIFY.

I can make it safer, using the no clone path only if tcf_action is TC_ACT_STOLEN. That will still cover the relevant use-cases.

Will do that in v4.

Cheers,

Paolo

maintainer
#35 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

Quoted 10 lines · Wed, Jul 25, 2018 at 12:14:32PM CEST, [email protected] wrote:
Quoted 9 lines · Cong Wang
Quoted 7 lines · Paolo Abeni
net/sched/act_mirred.c +7 -0
+
+               /* let's the caller reinject the packet, if possible */
+               if (skb_at_tc_ingress(skb)) {
+                       res->ingress = want_ingress;
+                       res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+                       return TC_ACT_REINJECT;
+               }
Quoted 6 lines
Quoted 3 lines

Looks good to me, but here we no longer return user-specified return value here, I am sure it is safe for TC_ACT_STOLEN, but I am not sure if it is safe for other values, like TC_ACT_RECLASSIFY.

I can make it safer, using the no clone path only if tcf_action is TC_ACT_STOLEN. That will still cover the relevant use-cases.

That is a good idea. Thanks!

Quoted 7 lines

Will do that in v4.

Cheers,

Paolo

maintainer
#36 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

In reply to Cong Wang
Quoted 9 lines · Cong Wang

net/sched/act_mirred.c static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, · line 238

Quoted 7 lines · Paolo Abeni
net/sched/act_mirred.c +7 -0
+
+               /* let's the caller reinject the packet, if possible */
+               if (skb_at_tc_ingress(skb)) {
+                       res->ingress = want_ingress;
+                       res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+                       return TC_ACT_REINJECT;
+               }
Quoted 6 lines

Looks good to me, but here we no longer return user-specified return value here, I am sure it is safe for TC_ACT_STOLEN, but I am not sure if it is safe for other values, like TC_ACT_RECLASSIFY.

Jamal, is there any use case of returning !TC_ACT_STOLEN for ingress redirections?

I cant think of one off top of my head.
There maybe a future use case where it is not so - maybe just allow to return the user programmed action? that value will always be TC_ACT_STOLEN if the rule was specified via iproute2/tc.

cheers,
jamal

maintainer
#37 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

In reply to Paolo Abeni
Quoted 74 lines · Tue, Jul 24, 2018 at 10:06:43PM CEST, [email protected] wrote:

When mirred is invoked from the ingress path, and it wants to redirect the processed packet, it can now use the TC_ACT_REINJECT 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 a typo into the commit
message

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

---
net/sched/act_mirred.c | 34 ++++++++++++++++++++++++----------

1 file changed, 24 insertions(+), 10 deletions(-)
net/sched/act_mirred.c +13 -6
index eeb335f03102..368187312136 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>
 

@@ -171,10 +172,12 @@ 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 want_ingress;
+	bool is_redirect;
 	int m_eaction;
 	int mac_len;
 

@@ -196,16 +199,19 @@ 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;
+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+	if (!skb_at_tc_ingress(skb) || !is_redirect) {
+		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 +222,23 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		}
 	}
 
+	skb2->skb_iif = skb->dev->ifindex;

net/sched/act_mirred.c static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, · line 234

Quoted 10 lines
net/sched/act_mirred.c +6 -1
+	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 reinject the packet, if possible */
+		if (skb_at_tc_ingress(skb)) {

I probably missed something. Why only on ingress?

The patch looks good to me.

Quoted 16 lines
net/sched/act_mirred.c +5 -3
+			res->ingress = want_ingress;
+			res->qstats = this_cpu_ptr(m->common.cpu_qstats);
+			return TC_ACT_REINJECT;
+		}
 	}
 
-	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);
Signature · 1 line
2.17.1
author maintainer
#38 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

Quoted 74 lines · Jiri Pirko
Quoted 73 lines · Tue, Jul 24, 2018 at 10:06:43PM CEST, [email protected] wrote:

When mirred is invoked from the ingress path, and it wants to redirect the processed packet, it can now use the TC_ACT_REINJECT 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 a typo into the commit
message

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

---
net/sched/act_mirred.c | 34 ++++++++++++++++++++++++----------

1 file changed, 24 insertions(+), 10 deletions(-)
net/sched/act_mirred.c +12 -6
index eeb335f03102..368187312136 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>
 

@@ -171,10 +172,12 @@ 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 want_ingress;
+	bool is_redirect;
 	int m_eaction;
 	int mac_len;
 

@@ -196,16 +199,19 @@ 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;
+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+	if (!skb_at_tc_ingress(skb) || !is_redirect) {
+		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 +222,23 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		}
 	}
Quoted 13 lines

net/sched/act_mirred.c static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, · line 234

Quoted 11 lines
net/sched/act_mirred.c +7 -1
+	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 reinject the packet, if possible */
+		if (skb_at_tc_ingress(skb)) {

I probably missed something. Why only on ingress?

To keep the implementation as simple as possible: if I read correctly, it is impossible for a filter detect if called by the clsact or the dev root qdisc, and I think we could safely avoid the skb clone with a not invasive patch, only if called from the clsact.

[please let me know if the above is somewhat clear ;)]

Also this covers nicely the relevant use case (TC S/W datapath).

Thanks,

Paolo

maintainer
#39 lore raw

Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible

Quoted 89 lines · Wed, Jul 25, 2018 at 04:04:03PM CEST, [email protected] wrote:
Quoted 74 lines · Jiri Pirko
Quoted 73 lines · Tue, Jul 24, 2018 at 10:06:43PM CEST, [email protected] wrote:

When mirred is invoked from the ingress path, and it wants to redirect the processed packet, it can now use the TC_ACT_REINJECT 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 a typo into the commit
message

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

---
net/sched/act_mirred.c | 34 ++++++++++++++++++++++++----------

1 file changed, 24 insertions(+), 10 deletions(-)
net/sched/act_mirred.c +12 -6
index eeb335f03102..368187312136 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>
 

@@ -171,10 +172,12 @@ 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 want_ingress;
+	bool is_redirect;
 	int m_eaction;
 	int mac_len;
 

@@ -196,16 +199,19 @@ 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;
+	is_redirect = tcf_mirred_is_act_redirect(m_eaction);
+	if (!skb_at_tc_ingress(skb) || !is_redirect) {
+		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 +222,23 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
 		}
 	}
Quoted 13 lines

net/sched/act_mirred.c static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a, · line 234

Quoted 11 lines
net/sched/act_mirred.c +7 -1
+	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 reinject the packet, if possible */
+		if (skb_at_tc_ingress(skb)) {

I probably missed something. Why only on ingress?

Quoted 8 lines

To keep the implementation as simple as possible: if I read correctly, it is impossible for a filter detect if called by the clsact or the dev root qdisc, and I think we could safely avoid the skb clone with a not invasive patch, only if called from the clsact.

[please let me know if the above is somewhat clear ;)]

Also this covers nicely the relevant use case (TC S/W datapath).

Sure. I was just curious. Perhaps put a comment to this optimisation describing why it is not possible for egress. It might help future readers.

Thanks!

Quoted 5 lines

Thanks,

Paolo

maintainer
#40 lore raw
In reply to Paolo Abeni
Quoted 26 lines · Tue, Jul 24, 2018 at 10:06:38PM CEST, [email protected] wrote:

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 3 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 value 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

Quoted 6 lines

Paolo Abeni (5):
tc/act: user space can't use TC_ACT_REDIRECT directly 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_REINJECT.
act_mirred: use TC_ACT_REINJECT when possible

Could you please send the userspace iproute2 patch and list some examples of usage along with the next patchset version?

Also, I would like you to do selftest script to test this and have it as a part of this patchset.

Thanks!

Quoted 20 lines
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          | 15 +++++++++++++-
net/sched/act_csum.c         | 12 +++---------
net/sched/act_ife.c          |  5 +----
net/sched/act_mirred.c       | 38 ++++++++++++++++++++++++------------
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, 98 insertions(+), 70 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#41 lore raw

Hi,

Quoted 2 lines · Jiri Pirko

Could you please send the userspace iproute2 patch and list some examples of usage along with the next patchset version?

There are no iproute2 patches, as TC_ACT_REINJECT is not accessible from user-space: act_mirred will use such value when possible.

User-space can have any notion of the above only via the perf tool and/or performances tests ;)

An use case is the OVS TC S/W datapath. openvswitch alreday leverage this when configured with:

other_config:hw-offload=true
other_config:tc-policy=none

Quoted 2 lines

Also, I would like you to do selftest script to test this and have it as a part of this patchset.

As said, there is no user-space change. AFAICS the current act_mirred self tests already cover this. Do you have other things in mind?

Cheers,

Paolo

maintainer
#42 lore raw
Quoted 8 lines · Wed, Jul 25, 2018 at 02:07:15PM CEST, [email protected] wrote:

Hi,

Quoted 2 lines · Jiri Pirko

Could you please send the userspace iproute2 patch and list some examples of usage along with the next patchset version?

There are no iproute2 patches, as TC_ACT_REINJECT is not accessible from user-space: act_mirred will use such value when possible.

Okay.

Quoted 7 lines

User-space can have any notion of the above only via the perf tool and/or performances tests ;)

An use case is the OVS TC S/W datapath. openvswitch alreday leverage this when configured with:

Quoted 8 lines

other_config:hw-offload=true
other_config:tc-policy=none

Quoted 2 lines

Also, I would like you to do selftest script to test this and have it as a part of this patchset.

As said, there is no user-space change. AFAICS the current act_mirred self tests already cover this. Do you have other things in mind?

Sounds allright. Thanks for the explanation!

Quoted 5 lines

Cheers,

Paolo