On 7/28/20 4:16 PM, Eli Britstein wrote:
>
> it is not "wildcarded". "wildcarded" means it had a match and it was removed.
> the case here it was only not "expanded".
>
>> It actually had a match on a filed and that match was removed from
>> the original flow while committing set() action. That is the bug that
>> this patch intended to fix. See the example below. There was a match
>> on source MAC, but it was removed by commit_set_ether_action().
> Right. My mistake before.
>>
>>>> match key and will never be matched, i.e. flows with any destination
>>>>
>>>> I'm a bit warried about this solution since we're not clearing out all the
>>>> masks, so memory sanitizers might bite us on that in case where will be
>>>> some
>>>> holes in the datastructures even though we're ORing them, but not actually
>>>> using these uninitialized bytes. To not use the unconditional OR we will
>>>> need to introduce new functions like 'or_ethernet_mask()' and this will
>>>> grow
>>>> the code size, which doesn't look very pretty.
>>>>
>>>> What do you think?
> That was my thought too when I did that work. For that, I introduced the
> generic 'struct offsetof_sizeof' array, and wildcarding the mask code is
> mutual for all attributes (ETH, IPv4,...). Maybe (I haven't thought it
> through) we can leverage the generic "commit" function that already gets that
> array to do the "ORs". This way we will do only for the fields and not tough
> the "holes".
Yes, that a good point. What about incremental change like this
(incremental to the previous diff I sent):
---
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 364a6fbe1..e54a78b43 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -7701,6 +7701,28 @@ struct offsetof_sizeof {
int size;
};
+
+/* Performs bitwise OR over the fields in 'dst_' and 'src_' specified in
+ * 'offsetof_sizeof_arr' array. Result is stored in 'dst_'. */
+static void
+or_masks(void *dst_, const void *src_,
+ struct offsetof_sizeof *offsetof_sizeof_arr)
+{
+ int field, size, offset;
+ const uint8_t *src = src_;
+ uint8_t *dst = dst_;
+
+ for (field = 0; ; field++) {
+ size = offsetof_sizeof_arr[field].size;
+ offset = offsetof_sizeof_arr[field].offset;
+
+ if (!size) {
+ return;
+ }
+ or_bytes(dst + offset, src + offset, size);
+ }
+}
+
/* Compares each of the fields in 'key0' and 'key1'. The fields are specified
* in 'offsetof_sizeof_arr', which is an array terminated by a 0-size field.
* Returns true if all of the fields are equal, false if at least one differs.
@@ -7796,7 +7818,7 @@ commit_set_ether_action(const struct flow *flow, struct
flow *base_flow,
&key, &base, &mask, sizeof key,
ovs_key_ethernet_offsetof_sizeof_arr, odp_actions)) {
put_ethernet_key(&base, base_flow);
- or_bytes(&mask, &orig_mask, sizeof mask);
+ or_masks(&mask, &orig_mask, ovs_key_ethernet_offsetof_sizeof_arr);
put_ethernet_key(&mask, &wc->masks);
}
}
---
And the same for all other callers of or_bytes().
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
