On 7/29/20 6:55 AM, Eli Britstein wrote:
>
> On 7/28/2020 7:32 PM, Ilya Maximets wrote:
>> 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?
> Seems OK. Can you please post v3?
Sure. Sent.
>>
>> Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
