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

[Roi Dayan]

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.