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

[Paul Blakey]

On 15/08/2017 20:30, 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>
> 
> Another couple of bits I noticed while responding..
> 
>> @@ -454,10 +572,56 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>>   }
>>
>>   static int
>> +parse_put_flow_set_masked_action(struct tc_flower *flower,
>> +                                 const struct nlattr *set,
>> +                                 size_t set_len,
>> +                                 bool hasmask)
>> +{
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>> +    const struct nlattr *set_attr;
>> +    char *key = (char *) &flower->rewrite.key;
>> +    char *mask = (char *) &flower->rewrite.mask;
>> +    size_t set_left;
>> +    int i, j;
>> +
>> +    NL_ATTR_FOR_EACH_UNSAFE(set_attr, set_left, set, set_len) {
>> +        int type = nl_attr_type(set_attr);
>> +        size_t size = nl_attr_get_size(set_attr) / 2;
>> +        char *set_data = CONST_CAST(char *, nl_attr_get(set_attr));
>> +        char *set_mask = set_data + size;
>> +
>> +        if (type >= ARRAY_SIZE(set_flower_map)) {
>> +            VLOG_DBG_RL(&rl, "unsupported set action type: %d", type);
>> +            return EOPNOTSUPP;
> 
> I think that this assumes that 'set_flower_map' has every entry
> populated up until the maximum supported key field - but are some
> missing - for example OVS_KEY_ATTR_VLAN. Could we also check by
> indexing into set_flower_map and checking if it's a valid entry?

Good catch, thanks, will be fixed.

> 
>> +        }
>> +
>> +        for (i = 0; i < ARRAY_SIZE(set_flower_map[type]); i++) {
>> +            struct netlink_field *f = &set_flower_map[type][i];
> 
> Separate thought - you're iterating nlattrs above then iterating all
> the key attributes in the flower_map here. Couldn't you just directly
> index into the packet field that this action will modify?
> 
>> +
>> +            if (!f->size) {
>> +                break;
>> +            }
> 
> I think that headers that are not in the set_flower_map will hit this,
> which would be unsupported (similar to above comment).

This is for null entries in the map, like OVS_KEY_ATTR_IPV6 has only
two entries in the second dimension (src and dst) but it can have up to 
3 so the third size is zeroed to skip it.

> 
>> +
>> +            for (j = 0; j < f->size; j++) {
>> +                char maskval = hasmask ? set_mask[f->offset + j] : 0xFF;
>> +
>> +                key[f->flower_offset + j] = maskval & set_data[f->offset + j];
>> +                mask[f->flower_offset + j] = maskval;
>> +            }
>> +        }
>> +    }
>> +
>> +    if (!is_all_zeros(&flower->rewrite, sizeof flower->rewrite)) {
>> +        flower->rewrite.rewrite = true;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int
>>   parse_put_flow_set_action(struct tc_flower *flower, const struct nlattr *set,
>>                             size_t set_len)
>>   {
>> -    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>>       const struct nlattr *set_attr;
>>       size_t set_left;
>>


Thanks for the review guys, We'll send a new patch set.