On 8 August 2017 at 07:21, Roi Dayan <[email protected]> wrote:
> From: Paul Blakey <[email protected]>
>
> Implement support for offloading ovs action set using
> tc header rewrite action.
>
> Signed-off-by: Paul Blakey <[email protected]>
> Reviewed-by: Roi Dayan <[email protected]>
> ---
<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?
> + }
> +
> + 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).
> +
> + 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;
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
