[ovs-dev] [PATCH] dpif-netdev: Translate Geneve options per-flow, not per-packet.

[Jesse Gross]

On Fri, Jul 10, 2015 at 11:09 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> On Jul 7, 2015, at 10:30 PM, Jesse Gross <jesse at nicira.com> wrote:
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index b53d52a..50763bc 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> +static void
>> +xlate_geneve_attr(const struct nlattr *key, uint32_t key_len,
>> +                  bool is_mask, struct flow *flow)
>> +{
>> +    const struct nlattr *geneve_key;
>> +
>> +    /* We don't actually want any translation for Geneve - just copy the
>> +     * original over. */
>> +    memset(&flow->tunnel.metadata, 0, sizeof flow->tunnel.metadata);
>
> Could we avoid zeroing the whole thing in the geneve case?

We could obviously avoid zeroing the part that we are about to copy
options over. However, this function is not performance critical (it's
not used during flow setups) so I don't know if there's any real
reason to do it piecemeal, which could be more error prone.

>> +    geneve_key = tun_metadata_find_geneve_key(key, key_len);
>> +    if (geneve_key) {
>> +        int len = nl_attr_get_size(geneve_key);
>> +        memcpy(&flow->tunnel.metadata.opts.gnv, nl_attr_get(geneve_key), len);
>> +        flow->tunnel.metadata.present.len = !is_mask ? len : 0xff;
>
> Why the len is 0xff for the is_mask case?

We need to match on the length of the options in addition to the
option data itself. Otherwise, it is possible to have a packet with
some set of options that creates a flow and then a second packet with
the same initial set options plus more after that. If we don't include
the length and mask it in the flow, then the additional options in the
second packet will be silently ignored. The kernel implementation has
similar logic.