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 <pa...@mellanox.com> > > > > > > > > > > To be later used to implement ovs action set offloading. > > > > > > > > > > Signed-off-by: Paul Blakey <pa...@mellanox.com> > > > > > Reviewed-by: Roi Dayan <r...@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. ... _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev