[ovs-dev] [PATCH 4/4] netdev-tc-offloads: Add support for action set

[Joe Stringer]

On 17 August 2017 at 00:52, Paul Blakey <paulb at mellanox.com> wrote:
>
>
> On 15/08/2017 20:24, Joe Stringer wrote:
>>
>> On 15 August 2017 at 01:19, Paul Blakey <paulb at mellanox.com> wrote:
>>>
>>>
>>>
>>> On 15/08/2017 10:28, Paul Blakey wrote:
>>>>
>>>>
>>>>
>>>>
>>>> On 15/08/2017 04:04, Joe Stringer wrote:
>>>>>
>>>>>
>>>>> On 8 August 2017 at 07:21, Roi Dayan <roid at mellanox.com> wrote:
>>>>>>
>>>>>>
>>>>>> From: Paul Blakey <paulb at mellanox.com>
>>>>>>
>>>>>> Implement support for offloading ovs action set using
>>>>>> tc header rewrite action.
>>>>>>
>>>>>> Signed-off-by: Paul Blakey <paulb at mellanox.com>
>>>>>> Reviewed-by: Roi Dayan <roid at mellanox.com>
>>>>>> ---
>>>>>
>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>> @@ -274,6 +346,48 @@ netdev_tc_flow_dump_destroy(struct
>>>>>> netdev_flow_dump
>>>>>> *dump)
>>>>>>        return 0;
>>>>>>    }
>>>>>>
>>>>>> +static void
>>>>>> +parse_flower_rewrite_to_netlink_action(struct ofpbuf *buf,
>>>>>> +                                       struct tc_flower *flower)
>>>>>> +{
>>>>>> +    char *mask = (char *) &flower->rewrite.mask;
>>>>>> +    char *data = (char *) &flower->rewrite.key;
>>>>>> +
>>>>>> +    for (int type = 0; type < ARRAY_SIZE(set_flower_map); type++) {
>>>>>> +        char *put = NULL;
>>>>>> +        size_t nested = 0;
>>>>>> +        int len = ovs_flow_key_attr_lens[type].len;
>>>>>> +
>>>>>> +        if (len <= 0) {
>>>>>> +            continue;
>>>>>> +        }
>>>>>> +
>>>>>> +        for (int j = 0; j < ARRAY_SIZE(set_flower_map[type]); j++) {
>>>>>> +            struct netlink_field *f = &set_flower_map[type][j];
>>>>>> +
>>>>>> +            if (!f->size) {
>>>>>> +                break;
>>>>>> +            }
>>>>>
>>>>>
>>>>>
>>>>> Seems like maybe there's similar feedback here to the previous patch
>>>>> wrt sparsely populated array set_flower_map[].
>>>>>
>>>>
>>>> I'll answer there.
>>>
>>>
>>>
>>> Scratch that, I actually meant to write it here.
>>>
>>> With this map we have fast (direct lookup) access on the put path
>>> (parse_put_flow_set_masked_action) , and slow access on the reverse dump
>>> path (parse_flower_rewrite_to_netlink_action) which iterates over all
>>> indices, although it should skips them  fast if they are empty.
>>>
>>> Changing this to sparse map will slow both of the paths, but dumping
>>> would
>>> be faster. We didn't write this maps for performance but for convince of
>>> adding new supported attributes.
>>
>>
>> OK, thanks for the explanation. I figured there must be a reason it
>> was indexed that way, I just didn't see it in my first review pass
>> through the series. Obviously fast path should be fast, and typically
>> we don't worry quite as much about dumping fast (although I have in
>> the past tested this to determine how many flows we will attempt to
>> populate in adverse conditions).
>>
>>> What we can do to both maps is:
>>>
>>> 1) Using ovs thread once, we can create a fast (direct lookup) access
>>> map;
>>> for both paths - generating a reverse map at init time and using that
>>> instead for dumping.
>>
>>
>> Until there's a clear, known advantage I'm not sure if we should do this.
>>
>>> 2) Rewrite it in a non sparse way, use it for both.
>>
>>
>> I think that we generally are more interested in 'put' performance so
>> if this sacrifices it for 'dump' performance, then it's probably not a
>> good idea (though it may still be too early to really optimize these).
>>
>>> 3) Rewrite it using a switch case, the logic for now is pretty much the
>>> same
>>> for all attributes.
>>>
>>> What do you think?
>>
>>
>> Do you think that this third option would improve the readability,
>> allow more compile-time checking on the code, or more code reuse?
>>
>
> Hi Joe, Thanks for the review,
>
> Since it can be implemented by a map, as with this patch set, all the switch
> cases will have the same logic of translating from OVS action set attributes
> to flower members, so I think its not much an increase in readability unless
> you're looking at a single case.
> And regarding compile time checking, we still need to convert from flower
> key members to tc action pedit raw offsets/values so we won't have that
> there.
>
> I'd like to remove the tc flower struct in the future
> and work straight on struct match and actions
> so then we will have only the set action to pedit conversion (and one
> conversion for all the rest).
>
> I think I'd rather keep the map till it won't fit for the task or we do the
> above if that's OK with you.

That seems reasonable enough to me.

I'm not sure exactly what you have in mind for removing the tc flower
struct in future, but I recognise that there's a bunch of inefficient
translation between different formats in this path today so I hope
we'll be able to improve this in the future. One of the things I've
had in mind for some time is to update the dpif layer to use 'struct
flow' for matches rather than OVS netlink-formatted matches, so then
the conversion can be pushed closer down to the kernel and we might
also be able to get rid of some of those conversions in this TC code.
We'll see.