TC: refactor act_mirred packets re-injection
Subsystems (3)
Paolo Abeni
maintainerv3 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
Paolo Abeni
maintainerPATCH 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
--- net/sched/act_api.c | 5 +++++ 1 file changed, 5 insertions(+)
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 ; + } +return a; err_mod:
Signature · 1 line
2.17.1
Jamal Hadi Salim
maintainerRe: [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
Jiri Pirko
maintainerRe: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
In reply to Paolo AbeniQuoted 21 lines · Tue, Jul 24, 2018 at 10:06:39PM CEST, [email protected] wrote:
Only
cls_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0
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
Paolo Abeni
maintainerRe: [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_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0@@ -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
Jiri Pirko
maintainerRe: [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_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0@@ -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_actionvalues 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
Paolo Abeni
maintainerRe: [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_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0@@ -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_actionvalues 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_REDIRECTto 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
Paolo Abeni
maintainerRe: [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_REDIRECTto 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
Re: [PATCH net-next v3 1/5] tc/act: user space can't use TC_ACT_REDIRECT directly
In reply to Paolo AbeniQuoted 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_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0@@ -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_actionvalues are simply accepted.If there is consensus about it, I can simply fail.
Well it was obviously wrong to expose
TC_ACT_REDIRECTto 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
Jiri Pirko
maintainerRe: [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_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0@@ -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_actionvalues 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_REDIRECTto 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_bpffrom that rejection as there may be a small chance that users could potentially use it as default action.Thanks,
Daniel
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_bpfandact_bpfcan safely use such value. If a generic action is configured by user space to returnTC_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_REDIRECTtoTC_ACT_UNSPECat action init time, making the kernel behavior more consistent.v1 -> v3: use
TC_ACT_UNSPECinstead of a newly definied act valueSigned-off-by Paolo Abeni <[email protected]>---
net/sched/act_api.c| 5 +++++1 file changed, 5 insertions(+)net/sched/act_api.c +0
-0@@ -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_actionvalues 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_REDIRECTto 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_REDIRECTto 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
Paolo Abeni
maintainerPATCH v3 2/5 net/sched: user-space can't set unknown tcfa_action values
In reply to Paolo AbeniCurrently, 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.
--- 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
@@ -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
@@ -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
Jiri Pirko
maintainerRe: [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_actionfield. If the value is unknown by the kernel, is implicitly threaded asTC_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 introducingtcfa_actionvalues not exposed to user-space in a later patch.Note: we can't use the above to hide
TC_ACT_REDIRECTfrom 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@@ -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@@ -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
Paolo Abeni
maintainerPATCH v3 3/5 tc/act: remove unneeded RCU lock in action callback
In reply to Paolo AbeniEach 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
--- 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
Jamal Hadi Salim
maintainerRe: [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
_bhvariant, 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
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
_bhvariant, 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
Re: [PATCH net-next v3 3/5] tc/act: remove unneeded RCU lock in action callback
In reply to Paolo AbeniQuoted 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
_bhvariant, 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 inact_bpf: it's needed by eBPF helpers, as pointed out by DanielSigned-off-by Paolo Abeni <[email protected]>
Paolo Abeni
maintainerPATCH v3 4/5 net/tc: introduce TC_ACT_REINJECT.
In reply to Paolo AbeniThis 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
--- 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
@@ -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
@@ -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
@@ -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
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.
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.
Paolo Abeni
maintainerRe: [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
Jamal Hadi Salim
maintainerRe: [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
8919bc13e8d92c5b082c5c0321567383a071f5bcAuthor: Jamal Hadi Salim <[email protected]> Date: Mon Aug 15 05:25:40 2011 +0000
net_sched: fix port mirror/redirect stats reportingLikely 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
Paolo Abeni
maintainerRe: [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
8919bc13e8d92c5b082c5c0321567383a071f5bcAuthor: 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
Jamal Hadi Salim
maintainerRe: [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
Re: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.
In reply to Jamal Hadi SalimQuoted 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.
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
Jamal Hadi Salim
maintainerRe: [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
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
Jamal Hadi Salim
maintainerRe: [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 thetcf_resultstruct are updated on error conditions.This new
tcfa_actionvalue is not exposed to the user-space and can be used only internally by clsact.v1 -> v2: do not touch
TC_ACT_REDIRECTcode path, introduce
a new action type insteadv2 -> v3:
- rename the new action valueTC_ACT_REINJECT, update the helper accordingly
- take care of uncloned reinjected packets in XDP generic hookSigned-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
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@@ -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@@ -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 ; }
Jiri Pirko
maintainerRe: [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
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.
Paolo Abeni
maintainerRe: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.
In reply to Jamal Hadi SalimHi,
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 thetcf_resultstruct are updated on error conditions.This new
tcfa_actionvalue is not exposed to the user-space and can be used only internally by clsact.v1 -> v2: do not touch
TC_ACT_REDIRECTcode path, introduce
a new action type insteadv2 -> v3:
- rename the new action valueTC_ACT_REINJECT, update the helper accordingly
- take care of uncloned reinjected packets in XDP generic hookSigned-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@@ -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
Jiri Pirko
maintainerRe: [PATCH net-next v3 4/5] net/tc: introduce TC_ACT_REINJECT.
In reply to Paolo AbeniQuoted 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 thetcf_resultstruct are updated on error conditions.This new
tcfa_actionvalue is not exposed to the user-space and can be used only internally by clsact.v1 -> v2: do not touch
TC_ACT_REDIRECTcode path, introduce a new action type insteadv2 -> v3:
- rename the new action valueTC_ACT_REINJECT, update the helper accordingly
- take care of uncloned reinjected packets in XDP generic hookSigned-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@@ -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@@ -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@@ -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
Paolo Abeni
maintainerPATCH v3 5/5 act_mirred: use TC_ACT_REINJECT when possible
In reply to Paolo AbeniWhen 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
--- net/sched/act_mirred.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)
net/sched/act_mirred.c +24 -10
@@ -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
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.
Paolo Abeni
maintainerRe: [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, likeTC_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
Jiri Pirko
maintainerRe: [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, likeTC_ACT_RECLASSIFY.I can make it safer, using the no clone path only if
tcf_actionisTC_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
Jamal Hadi Salim
maintainerRe: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
In reply to Cong WangQuoted 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, likeTC_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
Jiri Pirko
maintainerRe: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
In reply to Paolo AbeniQuoted 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_REINJECTaction, filling thetcf_resultaccordingly, and avoiding a per packetskb_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_MIRREDinstead ofACT_REDIRECT
v2 -> v3: updated after action rename, fixed a typo into the commit
messageSigned-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@@ -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
Paolo Abeni
maintainerRe: [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_REINJECTaction, filling thetcf_resultaccordingly, and avoiding a per packetskb_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_MIRREDinstead ofACT_REDIRECT
v2 -> v3: updated after action rename, fixed a typo into the commit
messageSigned-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@@ -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
Jiri Pirko
maintainerRe: [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_REINJECTaction, filling thetcf_resultaccordingly, and avoiding a per packetskb_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_MIRREDinstead ofACT_REDIRECT
v2 -> v3: updated after action rename, fixed a typo into the commit
messageSigned-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@@ -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
Jiri Pirko
maintainerQuoted 26 lines · Tue, Jul 24, 2018 at 10:06:38PM CEST, [email protected] wrote:
This series is aimed at improving the
act_mirredredirect performances. Such action is used by OVS to represent TC S/W flows, and it's current largest bottle-neck is the need for askb_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 accessibletcfa_actionvalue and internal values accessible only by the kernel.
Then a newtcfa_actionvalue is introduced:TC_ACT_REINJECT, similar toTC_ACT_REDIRECT, but preserving the mirred semantic. Such value is not accessible from user-space.
The last patch exploits the newly introduced infrastructure in theact_mirredaction, to avoid askb_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 inact_bpf
- add and use a new action value to reinject the packets, preserving the mirred semanticv2 -> v3:
- renamed to new action asTC_ACT_REINJECT
-TC_ACT_REINJECTis not exposed to user-space
Quoted 6 lines
Paolo Abeni (5):
tc/act: user space can't useTC_ACT_REDIRECTdirectly net/sched: user-space can't set unknowntcfa_actionvalues tc/act: remove unneeded RCU lock in action callback net/tc: introduceTC_ACT_REINJECT.
act_mirred: useTC_ACT_REINJECTwhen 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
Paolo Abeni
maintainerHi,
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
Jiri Pirko
maintainerQuoted 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_REINJECTis not accessible from user-space:act_mirredwill 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=noneQuoted 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_mirredself tests already cover this. Do you have other things in mind?
Sounds allright. Thanks for the explanation!
Quoted 5 lines
Cheers,
Paolo