[ovs-dev] [PATCH] datapath: do not add Geneve attributes if vport does not exist

[Ansis Atteka]

On Thu, Jul 17, 2014 at 4:08 PM, Jesse Gross <jesse at nicira.com> wrote:
> On Thu, Jul 17, 2014 at 3:46 PM, Ansis Atteka <aatteka at nicira.com> wrote:
>> diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>> index 5f975a1..a4108c0 100644
>> --- a/datapath/flow_netlink.c
>> +++ b/datapath/flow_netlink.c
>> @@ -1034,7 +1034,7 @@ int ovs_nla_put_flow(struct datapath *dp, const struct sw_flow_key *swkey,
>>                         struct vport *in_port;
>>
>>                         in_port = ovs_vport_ovsl_rcu(dp, swkey->phy.in_port);
>> -                       if (in_port->ops->type == OVS_VPORT_TYPE_GENEVE)
>> +                       if (in_port && in_port->ops->type == OVS_VPORT_TYPE_GENEVE)
>
> I think is probably not only an issue for Geneve ports (since the
> crash happens before we get to check the type) but for tunnel ports in
> general. I also guess that the scenario is not that the port hasn't
> been created yet but that the port has been deleted and the flow still
> exists.
I somehow remember reproducing this issue only when switching from gre
to geneve. Either, that implies:
1. issue with my test; or
2. when switching from gre to geneve, then the window during which
vport could be NULL is longer.

Wouldn't be surprised that it is #1.

>
> One thing that I worry about is that this has the possibility to
> change how the flow is reported - before the flow deletion it has
> Geneve options but immediately after the flow still exists but without
> options. OVS will likely deal with this but it doesn't seem like a
> great thing.
I talked with Pravin on how to solve this "flow changing while vport
gets added" issue and he seemed to prefer the idea where we simply
look into tun_opts_len to figure out whether to append Geneve
attributes.

Basically, the new patch makes assumption that if tun_opts_len>0 then
that is Geneve port that could potentially have Geneve options. Also,
I remember you mentioning that it might be good to distinguish between
the case when there are no tunnel options at all and the case when
Geneve attributes contain an empty set. Patch v2 does not address
that. How important is that in your opinion?