Message-Id: <23FB5447-CE8C-4EB4-B3FC-4B5BBCEE03AF@scaleway.com>
Date:   Mon, 26 Nov 2018 20:06:26 +0100
From:   Alexis Bauvin <abauvin@...leway.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>,
        David Ahern <dsa@...ulusnetworks.com>
Cc:     netdev <netdev@...r.kernel.org>, akherbouche@...leway.com,
        Alexis Bauvin <abauvin@...leway.com>
Subject: Re: [RFC v4 3/5] vxlan: add support for underlay in non-default VRF

Le 26 nov. 2018 à 19:26, Roopa Prabhu <roopa@...ulusnetworks.com> a écrit :
> 
> On Mon, Nov 26, 2018 at 9:54 AM David Ahern <dsa@...ulusnetworks.com> wrote:
>> 
>> On 11/26/18 9:32 AM, Alexis Bauvin wrote:
>>> Thanks for the review. I’ll send a v5 if you have no other comment on
>>> this version!
>> 
>> A few comments on the test script; see attached which has the changes.
>> 
>> Mainly the cleanup does not need to be called at the end since you setup
>> the exit trap. The cleanup calls ip to delete veth-hv-1 and veth-tap but
>> those are moved to other namespaces. 'ip netns exec NAME ip ...' is more
>> efficiently done as 'ip -netns NAME ...'. The test results should align
>> like this:
>> 
>> Checking HV connectivity                                          [ OK ]
>> Check VM connectivity through VXLAN (underlay in the default VRF) [ OK ]
>> Check VM connectivity through VXLAN (underlay in a VRF)           [ OK ]
>> 
>> So it is easy for users to see the PASS/FAIL.
>> 
>> It would be good to copy the topology ascii art into the test script as
>> well for future users.
>> 
>> Also, add the test as a separate patch at the end and include it in
>> tools/testing/selftests/net/Makefile
>> 
>> Finally, I think you should drop the RFC and send it as a 'ready for
>> inclusion'.
> 
> I cant seem to find patch 5 in my mail box... so commenting here
> (Using reference to patch5 from here
> https://marc.info/?l=linux-netdev&m=154284885815549&w=2)
> 
> Still not convinced that the auto reopen is justified here IMO because
> it can be done from user-space and there are many cases where this is
> already done from user-space. A few questions for alexis on that,

I do agree on this. The test shows that a simple down/up is enough, and
the patch was written as a mere convenience.

> - What is the reason for handling NETDEV_CHANGE on the vxlan device
> from the notifier handler. It can be really done in the changelink
> handler, correct  ?

Looks correct to me. The reason is nothing more than me not thinking
about the netlink handlers.

> - Also, IIUC, patch5 blindly re-opens the vxlan device without
> considering if the admin had set it to down last (ie the last state on
> it was vxlan_close). is that correct ?

It is correct. This is a big oversight from my side, that could have
led to crashes. Fortunately the underlying code will check if the
sockets are null (which they are if the interface is down) before
accessing them.

> (Don't want to block the entire series for just patch5. Patch5 can be
> done incrementally after we converge on it. The rest of the series
> looks good as David has already reviewed.  And nice to see the test!).

Thanks!

Given the aforementioned oversight when handling a down interface, it is
best to wait for a better solution for this patch.
Moreover, the issue of mixing default and non-default vrf needs to be
addressed. For now it is stale, as I don’t see any solution (except for
rewriting the whole thing as you suggested before) to address the
"Address already in use" made by a socket of the default vrf owning the
port across all vrfs.
I tested both Vyatta’s changes and SO_REUSEPORT, and neither of them seem
to work for this case.

Powered by blists - more mailing lists