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

[Paul Blakey]

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.

>> +
>> +            if (!is_all_zeros(mask + f->flower_offset, f->size)) {
>> +                if (!put) {
>> +                    nested = nl_msg_start_nested(buf,
>> +                                                 OVS_ACTION_ATTR_SET_MASKED);
>> +                    put = nl_msg_put_unspec_zero(buf, type, len * 2);
> 
> Do we ever have multiple of these attributes in a single nested
> OVS_ACTION_ATTR_SET_MASKED? If so, won't the following memcpys avoid
> putting the netlink header for the subsequent sets?
> 

  OVS_ACTION_ATTR_SET_MASKED always has a single nested OVS_KEY_ATTR_* 
attribute so we put it (all zeros) the first time we find a sub 
attribute (e.g ipv4_dst of ipv4) that isn't zero and use memcpy below to 
overwrite the non zero sub attributes.

>> +                }
>> +
>> +                memcpy(put + f->offset, data + f->flower_offset, f->size);
>> +                memcpy(put + len + f->offset,
>> +                       mask + f->flower_offset, f->size);
> 
> Could we these combine with the nl_msg_put_unspec() above?
> 

We'd have to accumulate the data as we go over the inner loop and then 
write it.
We could, for example, create a stack buffer like char 
buff[MAX_ATTR_LEN] (kinda ugly), write it there, and if we wrote 
something to buff, flush it to a nested attribute in a single write.
Would that be better? It might be less readable and it is less flexible 
(defining of MAX_ATTR_LEN).