On 15/08/2017 20:30, Joe Stringer wrote:
On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
From: Paul Blakey <pa...@mellanox.com>
Implement support for offloading ovs action set using
tc header rewrite action.
Signed-off-by: Paul Blakey <pa...@mellanox.com>
Reviewed-by: Roi Dayan <r...@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
