Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
Wed, Jul 25, 2018 at 04:04:03PM CEST, pab...@redhat.com wrote:
>On Wed, 2018-07-25 at 15:52 +0200, Jiri Pirko wrote:
>> Tue, Jul 24, 2018 at 10:06:43PM CEST, pab...@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
>> > ---
>> > 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
>> > #include
>> > #include
>> > +#include
>> > #include
>> > #include
>> >
>> > @@ -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
>
Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
On Wed, 2018-07-25 at 15:52 +0200, Jiri Pirko wrote:
> Tue, Jul 24, 2018 at 10:06:43PM CEST, pab...@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
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -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
Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
Tue, Jul 24, 2018 at 10:06:43PM CEST, pab...@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
>---
> 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
> #include
> #include
>+#include
> #include
> #include
>
>@@ -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
>
Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
Wed, Jul 25, 2018 at 12:14:32PM CEST, pab...@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 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
>
Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
On 24/07/18 05:15 PM, Cong Wang wrote:
On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni 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
Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
On Tue, 2018-07-24 at 14:15 -0700, Cong Wang wrote:
> On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni 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
Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
On Tue, Jul 24, 2018 at 1:07 PM Paolo Abeni 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.
[PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible
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
---
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
#include
#include
+#include
#include
#include
@@ -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);
--
2.17.1
