Date:   Tue, 27 Nov 2018 13:55:26 +0100
From:   Alexis Bauvin <abauvin@...leway.com>
To:     Roopa Prabhu <roopa@...ulusnetworks.com>
Cc:     David Ahern <dsa@...ulusnetworks.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        netdev <netdev@...r.kernel.org>, akherbouche@...leway.com
Subject: Re: [PATCH v5 5/6] vxlan: handle underlay VRF changes

Le 27 nov. 2018 à 06:58, Roopa Prabhu <roopa@...ulusnetworks.com> a écrit :
> On Mon, Nov 26, 2018 at 5:04 PM Alexis Bauvin <abauvin@...leway.com> wrote:
>> 
>> When underlay VRF changes, either because the lower device itself changed,
>> or its VRF changed, this patch releases the current socket of the VXLAN
>> device and recreates another one in the right VRF. This allows for
>> on-the-fly change of the underlay VRF of a VXLAN device.
>> 
>> Signed-off-by: Alexis Bauvin <abauvin@...leway.com>
>> Reviewed-by: Amine Kherbouche <akherbouche@...leway.com>
>> Tested-by: Amine Kherbouche <akherbouche@...leway.com>
>> ---
> 
> re-iterating my comments on the patch this time.
> 
> this version still unconditionally calls reopen even if the current
> state of the device is closed (eg vxlan_stop).
> generally not in favor of the unconditional open/close in the driver.
> Lets see if there are other options.
> I interpreted one of Davids suggestions to force the change ordering
> from user-space by returning an error.
> ie Make the user do a down and up of the vxlan device if he wants to
> change the vrf of the default remote dev.

Yes, this is already how this works without patch 5.

> This patch needs more thought, the rest are ok to go in if you
> separate them out.

Ooops I wanted to send v5 without patch 5, my mistake. Sending v6
without patch 5!

>> drivers/net/vxlan.c | 82 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 82 insertions(+)
>> 
>> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
>> index 8ba0a57ff958..131ee80a38f9 100644
>> --- a/drivers/net/vxlan.c
>> +++ b/drivers/net/vxlan.c
>> @@ -3720,6 +3720,33 @@ struct net_device *vxlan_dev_create(struct net *net, const char *name,
>> }
>> EXPORT_SYMBOL_GPL(vxlan_dev_create);
>> 
>> +static int vxlan_reopen(struct vxlan_net *vn, struct vxlan_dev *vxlan)
>> +{
>> +       int ret = 0;
>> +
>> +       if (vxlan_addr_multicast(&vxlan->default_dst.remote_ip) &&
>> +           !vxlan_group_used(vn, vxlan))
>> +               ret = vxlan_igmp_leave(vxlan);
>> +       vxlan_sock_release(vxlan);
>> +
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       ret = vxlan_sock_add(vxlan);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (vxlan_addr_multicast(&vxlan->default_dst.remote_ip)) {
>> +               ret = vxlan_igmp_join(vxlan);
>> +               if (ret == -EADDRINUSE)
>> +                       ret = 0;
>> +               if (ret)
>> +                       vxlan_sock_release(vxlan);
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
>>                                             struct net_device *dev)
>> {
>> @@ -3742,6 +3769,55 @@ static void vxlan_handle_lowerdev_unregister(struct vxlan_net *vn,
>>        unregister_netdevice_many(&list_kill);
>> }
>> 
>> +static void vxlan_handle_change_upper(struct vxlan_net *vn,
>> +                                     struct net_device *dev)
>> +{
>> +       struct vxlan_dev *vxlan, *next;
>> +
>> +       list_for_each_entry_safe(vxlan, next, &vn->vxlan_list, next) {
>> +               struct net_device *lower;
>> +               int err;
>> +
>> +               lower = __dev_get_by_index(vxlan->net,
>> +                                          vxlan->cfg.remote_ifindex);
>> +               if (!netdev_is_upper_master(lower, dev))
>> +                       continue;
>> +
>> +               err = vxlan_reopen(vn, vxlan);
>> +               if (err < 0)
>> +                       netdev_err(vxlan->dev, "Failed to reopen socket: %d\n",
>> +                                  err);
>> +       }
>> +}
>> +
>> +static void vxlan_handle_change(struct vxlan_net *vn, struct net_device *dev)
>> +{
>> +       struct vxlan_dev *vxlan = netdev_priv(dev);
>> +       struct vxlan_sock *sock;
>> +       int l3mdev_index = 0;
>> +
>> +#if IS_ENABLED(CONFIG_IPV6)
>> +       bool metadata = vxlan->cfg.flags & VXLAN_F_COLLECT_METADATA;
>> +       bool ipv6 = vxlan->cfg.flags & VXLAN_F_IPV6 || metadata;
>> +
>> +       sock = ipv6 ? rcu_dereference(vxlan->vn6_sock)
>> +                   : rcu_dereference(vxlan->vn4_sock);
>> +#else
>> +       sock = rcu_dereference(vxlan->vn4_sock);
>> +#endif
>> +
>> +       if (vxlan->cfg.remote_ifindex)
>> +               l3mdev_index = l3mdev_master_upper_ifindex_by_index(
>> +                       vxlan->net, vxlan->cfg.remote_ifindex);
>> +       if (sock->sock->sk->sk_bound_dev_if != l3mdev_index) {
>> +               int ret = vxlan_reopen(vn, vxlan);
>> +
>> +               if (ret < 0)
>> +                       netdev_err(vxlan->dev, "Failed to reopen socket: %d\n",
>> +                                  ret);
>> +       }
>> +}
>> +
>> static int vxlan_netdevice_event(struct notifier_block *unused,
>>                                 unsigned long event, void *ptr)
>> {
>> @@ -3756,6 +3832,12 @@ static int vxlan_netdevice_event(struct notifier_block *unused,
>>        } else if (event == NETDEV_UDP_TUNNEL_PUSH_INFO ||
>>                   event == NETDEV_UDP_TUNNEL_DROP_INFO) {
>>                vxlan_offload_rx_ports(dev, event == NETDEV_UDP_TUNNEL_PUSH_INFO);
>> +       } else if (event == NETDEV_CHANGEUPPER) {
>> +               vxlan_handle_change_upper(vn, dev);
>> +       } else if (event == NETDEV_CHANGE) {
>> +               if (dev->rtnl_link_ops &&
>> +                   !strcmp(dev->rtnl_link_ops->kind, vxlan_link_ops.kind))
>> +                       vxlan_handle_change(vn, dev);
> 
> This should move to the rtnl changelink handler
> 
> 
>>        }
>> 
>>        return NOTIFY_DONE;
>> --

Powered by blists - more mailing lists