| Message ID | 612f6fb5b0cdddcbbc2d99f65b148b7f3b8b9a27.1532437050.git.pabeni@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | TC: refactor act_mirred packets re-injection | expand |
On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni <pabeni@redhat.com> wrote: > + > + /* 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.
On Tue, 2018-07-24 at 14:15 -0700, Cong Wang wrote: > On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni <pabeni@redhat.com> wrote: > > + > > + /* 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. 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
On 24/07/18 05:15 PM, Cong Wang wrote: > On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni <pabeni@redhat.com> wrote: >> + >> + /* 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? 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
Wed, Jul 25, 2018 at 12:14:32PM CEST, pabeni@redhat.com wrote: >On Tue, 2018-07-24 at 14:15 -0700, Cong Wang wrote: >> On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni <pabeni@redhat.com> wrote: >> > + >> > + /* 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. > >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! > >Will do that in v4. > >Cheers, > >Paolo >
Tue, Jul 24, 2018 at 10:06:43PM CEST, pabeni@redhat.com 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 <pabeni@redhat.com> >--- > net/sched/act_mirred.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > >diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c >index eeb335f03102..368187312136 100644 >--- a/net/sched/act_mirred.c >+++ b/net/sched/act_mirred.c >@@ -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)) { I probably missed something. Why only on ingress? The patch looks good to me. >+ 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); >-- >2.17.1 >
On Wed, 2018-07-25 at 15:52 +0200, Jiri Pirko wrote: > Tue, Jul 24, 2018 at 10:06:43PM CEST, pabeni@redhat.com 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 <pabeni@redhat.com> > > --- > > net/sched/act_mirred.c | 34 ++++++++++++++++++++++++---------- > > 1 file changed, 24 insertions(+), 10 deletions(-) > > > > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c > > index eeb335f03102..368187312136 100644 > > --- a/net/sched/act_mirred.c > > +++ b/net/sched/act_mirred.c > > @@ -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)) { > > 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
Wed, Jul 25, 2018 at 04:04:03PM CEST, pabeni@redhat.com wrote: >On Wed, 2018-07-25 at 15:52 +0200, Jiri Pirko wrote: >> Tue, Jul 24, 2018 at 10:06:43PM CEST, pabeni@redhat.com 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 <pabeni@redhat.com> >> > --- >> > net/sched/act_mirred.c | 34 ++++++++++++++++++++++++---------- >> > 1 file changed, 24 insertions(+), 10 deletions(-) >> > >> > diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c >> > index eeb335f03102..368187312136 100644 >> > --- a/net/sched/act_mirred.c >> > +++ b/net/sched/act_mirred.c >> > @@ -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)) { >> >> 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). 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! > >Thanks, > >Paolo >
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c index eeb335f03102..368187312136 100644 --- a/net/sched/act_mirred.c +++ b/net/sched/act_mirred.c @@ -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);
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 <pabeni@redhat.com> --- net/sched/act_mirred.c | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-)