From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Abeni Subject: Re: [PATCH net-next v3 5/5] act_mirred: use TC_ACT_REINJECT when possible Date: Wed, 25 Jul 2018 16:04:03 +0200 Message-ID: <721f42fb778546dfc1f9e2ae1309fcf4476d3c84.camel@redhat.com> References: <612f6fb5b0cdddcbbc2d99f65b148b7f3b8b9a27.1532437050.git.pabeni@redhat.com> <20180725135215.GM2164@nanopsycho> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, Jamal Hadi Salim , Cong Wang , Daniel Borkmann , Marcelo Ricardo Leitner , Eyal Birger , "David S. Miller" To: Jiri Pirko Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:40500 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1727504AbeGYPP4 (ORCPT ); Wed, 25 Jul 2018 11:15:56 -0400 In-Reply-To: <20180725135215.GM2164@nanopsycho> Sender: netdev-owner@vger.kernel.org List-ID: 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 > > --- > > 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