[ovs-dev] [add-remove 7/7] vswitchd: Better tolerate changes in datapath ports.

[Jesse Gross]

On Tue, Sep 28, 2010 at 4:11 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Tue, Sep 28, 2010 at 03:54:20PM -0700, Jesse Gross wrote:
>> On Tue, Sep 28, 2010 at 11:58 AM, Ben Pfaff <blp at nicira.com> wrote:
>> > This commit fixes the problem by adding the ifindex to the callbacks.  In
>> > step 3, bridge_port_changed_ofhook_cb sees that the port that was deleted
>> > was not the same as the one that it has in its interface and ignores it.
>> >
>> > Thanks to Jesse Gross for identifying the problem.
>>
>> I've only skimmed through this set very quickly, so this isn't a full
>> review yet and it is possible that I missed something.  However,
>> ifindex is only available for devices that exist in the system but not
>> virtual ports that we create.  Therefore, won't this reject all
>> virtual ports because they always return -EOPNOTSUPP when their
>> ifindex is requested?
>
> It doesn't have a way to uniquely identify instantiations of a vport But
> it accepts EOPNOTSUPP as an indication that it can't tell them apart
> (see the change to make_ofport()).  So it will always consider any two
> non-netdev based vports to be the same.

Oops, you're right.  I misread the != -EOPNOTSUPP.

However, this problem was originally reported for GRE ports and that
seems like the place where it is most likely to happen given that the
ports can be created out of thin air and therefore are more likely to
change.  I think we need to cover vports as well.

>
> Is there a way to uniquely identify an instantiation of a vport?  Should
> we invent one?

No, there's nothing analogous that comes to mind (really, the OpenFlow
port number is the closest equivalent but that is obviously
insufficient).

What would a vport identifier look like?  Just a monotonically
increasing number?  In that case, would it possible to make the
OpenFlow port number have similar properties?

>
> I suppose that two vports with the same name, port number, and
> configuration could be considered the same vport.  But currently there
> is no way, as far as I know, to retrieve the configuration of a vport
> from the kernel.  (I added one in the netlink series.)  If we could do
> that, then we could at least determine whether vports were equivalent.

We do a similar thing inside netdev.c to check whether to reconfigure
based on changes in the arguments.  I suppose we could do something
along those lines.  I think there would still be a corner case if you
delete and add a port with the same configuration at the same time.
It's illogical but not specifically disallowed or checked for.

However, at another level it seems a bit weird to have to do that.
After all, userspace is the one making these changes and receiving the
notification, it's not as if any of this is truly "unexpected" as the
log message indicates.

Ideally, I would like to see this handled by closer sharing of
information between userspace components.  Passing information between
the different pieces through the kernel has always struck me as a bit
odd.  Maybe making that change isn't worth it to fix this bug but the
loose coupling seems inherently racy.