[ovs-dev] [PATCH v2] odp-util: Fix clearing match mask if set action is partially unnecessary.

[Ilya Maximets]

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.