[ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action

Simon Horman simon.horman at netronome.com
Mon Nov 20 12:30:38 UTC 2017


On Sun, Nov 19, 2017 at 08:45:19AM +0200, Roi Dayan wrote:
> 
> 
> On 16/11/2017 18:29, Simon Horman wrote:
> > On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote:
> > > 
> > > 
> > > On 27/09/2017 12:08, Simon Horman wrote:
> > > > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
> > > > > 
> > > > > 
> > > > > On 18/09/2017 18:01, Simon Horman wrote:
> > > > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> > > > > > > From: Paul Blakey <paulb at mellanox.com>
> > > > > > > 
> > > > > > > To be later used to implement ovs action set offloading.
> > > > > > > 
> > > > > > > Signed-off-by: Paul Blakey <paulb at mellanox.com>
> > > > > > > Reviewed-by: Roi Dayan <roid at mellanox.com>
> > > > > > > ---
> > > > > > >    lib/tc.c | 372 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > > > > >    lib/tc.h |  16 +++
> > > > > > >    2 files changed, 385 insertions(+), 3 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > > > > index c9cada2..743b2ee 100644
> > > > > > > --- a/lib/tc.c
> > > > > > > +++ b/lib/tc.c
> > > > > > > @@ -21,8 +21,10 @@
> > > > > > >    #include <errno.h>
> > > > > > >    #include <linux/if_ether.h>
> > > > > > >    #include <linux/rtnetlink.h>
> > > > > > > +#include <linux/tc_act/tc_csum.h>
> > > > > > >    #include <linux/tc_act/tc_gact.h>
> > > > > > >    #include <linux/tc_act/tc_mirred.h>
> > > > > > > +#include <linux/tc_act/tc_pedit.h>
> > > > > > >    #include <linux/tc_act/tc_tunnel_key.h>
> > > > > > >    #include <linux/tc_act/tc_vlan.h>
> > > > > > >    #include <linux/gen_stats.h>
> > > > > > > @@ -33,11 +35,14 @@
> > > > > > >    #include "netlink-socket.h"
> > > > > > >    #include "netlink.h"
> > > > > > >    #include "openvswitch/ofpbuf.h"
> > > > > > > +#include "openvswitch/util.h"
> > > > > > >    #include "openvswitch/vlog.h"
> > > > > > >    #include "packets.h"
> > > > > > >    #include "timeval.h"
> > > > > > >    #include "unaligned.h"
> > > > > > > +#define MAX_PEDIT_OFFSETS 8
> > > > > > 
> > > > > > Why 8?
> > > > > We don't expect anything more right now (ipv6 src/dst rewrite requires 8
> > > > > pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> > > > > that's makes sens. do you suggest we increase it? to what?
> > > > 
> > > > It seems strange to me to place a somewhat arbitrary small limit
> > > > when none exists in the pedit API being used. I would at prefer if
> > > > it was at least a bigger, say 16 or 32.
> > > 
> > 
> > > Hi Simon,
> > > 
> > > Sorry for the late reply due to holidays and vacations.
> > > Me & Paul going to go over this and do the fixes needed and
> > > also rebase over latest master and run tests again.
> > > 
> > > I'll answer what I'm more familiar with now and Paul will continue.
> > > The 8 here is too low and you right. We used this definition
> > > for allocation of the pedit keys on the stack in
> > > nl_msg_put_flower_rewrite_pedits()
> > > 
> > > It was for convenience instead of calculating the maximum possible
> > > keys that could exists and allocating it there and freeing it at
> > > the end.
> > > 
> > > Increasing it to 32 is probably more than enough and wont waste much.
> > 
> > I updated the value to 32 when applying the patch.
> > 
> > ...
> > 
> > > > > > If I understand the above correctly it is designed to make
> > > > > > pedit actions disjoint. If so, why is that necessary? >
> > > > > 
> > > > > It's not, as a single flower key rewrite can be split to multiple pedit
> > > > > actions it finds the overlap between a flower key and a pedit action, if
> > > > > they do overlap it translates it to the correct offset and masks it out.
> > > > 
> > > > Thanks, understood.
> > > > 
> > > > > 
> > > > > > > +        } else {
> > > > > > > +            VLOG_ERR_RL(&error_rl, "unable to parse legacy pedit type: %d",
> > > > > > > +                        nl_attr_type(nla));
> > > > > > > +            return EOPNOTSUPP;
> > > > > > > +        }
> > > > > > 
> > > > > > I think the code could exit early here as
> > > > > > nl_msg_put_flower_rewrite_pedits() does below.
> > > > > > 
> > > > > 
> > > > > Sorry, didn't understand. can you give an example?
> > > > > 
> > > > 
> > > > I meant something like this.
> > > > 
> > > >               if (nl_attr_type(nla) != TCA_PEDIT_KEY_EX) {
> > > >                   VLOG_ERR_RL(...);
> > > >                   return EOPNOTSUPP;
> > > >               }
> > > 
> > > understood. we'll do that. thanks.
> > 
> > I also fixed this when applying the patch.
> > 
> > ...
> > 
> 
> 
> Thanks Simon. sorry we were not responsive enough with this feature.
> We'll improve that.
> 

No problem.

Could you work on a fix for the travis failure that Eric Garver flagged?


More information about the dev mailing list