Re: [PATCH v5 5/6] vxlan: handle underlay VRF changes
Le 27 nov. 2018 à 06:58, Roopa Prabhu a écrit :
> On Mon, Nov 26, 2018 at 5:04 PM Alexis Bauvin 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
>> Reviewed-by: Amine Kherbouche
>> Tested-by: Amine Kherbouche
>> ---
>
> 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(>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(>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(_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, >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)
Re: [PATCH v5 5/6] vxlan: handle underlay VRF changes
On Mon, Nov 26, 2018 at 5:04 PM Alexis Bauvin 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
> Reviewed-by: Amine Kherbouche
> Tested-by: Amine Kherbouche
> ---
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.
This patch needs more thought, the rest are ok to go in if you
separate them out.
> 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(>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(>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(_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, >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 &&
> +
[PATCH v5 5/6] vxlan: handle underlay VRF changes
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
Reviewed-by: Amine Kherbouche
Tested-by: Amine Kherbouche
---
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(>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(>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(_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, >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);
}
return NOTIFY_DONE;
--
