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

[Joe Stringer]

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?