vxlan: allow specifying multiple default destinations

Quoted 8 lines · On Wed, 29 May 2013 13:00:15 +0300 Mike Rapoport…

drivers/net/vxlan.c +2 -3

-/* Add/update destinations for multicast */
-static int vxlan_fdb_append(struct vxlan_fdb *f,
-                         __be32 ip, __be16 port, __u32 vni, __u32 ifindex)
+static int vxlan_rdst_append(struct vxlan_rdst *rdst, __be32 ip, __be16 port,
+                          __u32 vni, __u32 ifindex)
 {
      struct vxlan_rdst *rd_prev, *rd;

drivers/net/vxlan.c static struct vxlan_fdb *vxlan_find_mac(struct vxlan_dev *vxlan, · line 375

Quoted 3 lines

      rd_prev = NULL;

drivers/net/vxlan.c +1 -1

-     for (rd = &f->remote; rd; rd = rd->remote_next) {
+     for (rd = rdst; rd; rd = rd->remote_next) {

The remote destinations should be using one of our nice list macros.

And what about locking? or RCU?

drivers/net/vxlan.c +24 -0

+static int vxlan_remote_add(struct vxlan_dev *vxlan, struct nlattr *attr)
+{
+	struct nlattr *i;
+	__be32 ip = htonl(INADDR_NONE);
+	__be16 port;
+	u32 ifindex, vni;
+	int rem, err = 0;
+
+	port = vxlan->dst_port;
+	vni = vxlan->default_dst.remote_vni;
+	ifindex = vxlan->default_dst.remote_ifindex;
+
+	nla_for_each_nested(i, attr, rem) {
+		switch (nla_type(i)) {
+		case IFLA_VXLAN_REMOTE_ADDR:
+			ip = nla_get_be32(i);
+			break;
+		case IFLA_VXLAN_REMOTE_PORT:
+			port = nla_get_be16(i);
+			break;
+		case IFLA_VXLAN_REMOTE_VNI:
+			vni = nla_get_u32(i);
+			break;
+		case IFLA_VXLAN_REMOTE_IFINDEX:

drivers/net/vxlan.c +6 -0

@@ -1512,6 +1646,46 @@ static struct vxlan_sock *vxlan_socket_create(struct net *net, __be16 port)
 	return vs;
 }
 
+static int vxlan_remotes_update(struct vxlan_dev *vxlan, struct nlattr *attr)
+{
+	struct nlattr *i;
+	int rem, err = 0;
+
+	nla_for_each_nested(i, attr, rem) {

drivers/net/vxlan.c +8 -0

+static int vxlan_fill_remotes_info(struct sk_buff *skb,
+				   const struct vxlan_dev *vxlan)
+{
+	struct vxlan_rdst *rdst;
+	struct nlattr *nest, *rdst_nest;
+	__be32 ip;
+	int i;
+

Looks much better, thanks for taking the feedback. Some additional feedback inlined:

Quoted 7 lines

drivers/net/vxlan.c +7 -0

+   const struct vxlan_dev *vxlan)
+{
+ struct vxlan_rdst *rdst;
+ struct nlattr *nest, *rdst_nest;
+ __be32 ip;
+ int i;
+

drivers/net/vxlan.c static size_t vxlan_get_size(const struct net_device *dev) · line 1884

Quoted 10 lines

drivers/net/vxlan.c +10 -0

+ if (vxlan->remote_cnt) {
+ nest = nla_nest_start(skb, IFLA_VXLAN_REMOTES);
+ if (nest == NULL)
+ goto nla_put_failure;
+
+ for (rdst = vxlan->default_dst.remote_next, i = 0; rdst;
+     rdst = rdst->remote_next, i++) {
+ ip = rdst->remote_ip;
+
+ rdst_nest = nla_nest_start(skb, i);

Attribute type '0' is reserved, please don't use it. Start with '1'.

include/uapi/linux/if_link.h enum { · line 322

Quoted 4 lines

include/uapi/linux/if_link.h +4 -0

+enum {
+ IFLA_VXLAN_REMOTE_NEW,
+ IFLA_VXLAN_REMOTE_DEL,
+};

Same here, attribute type '0' is reserved.

Quoted 23 lines · Thomas Graf

Looks much better, thanks for taking the feedback. Some additional feedback inlined:

Quoted 7 lines

drivers/net/vxlan.c +7 -0

+   const struct vxlan_dev *vxlan)
+{
+ struct vxlan_rdst *rdst;
+ struct nlattr *nest, *rdst_nest;
+ __be32 ip;
+ int i;
+

drivers/net/vxlan.c static size_t vxlan_get_size(const struct net_device *dev) · line 1884

Quoted 10 lines

drivers/net/vxlan.c +10 -0

+ if (vxlan->remote_cnt) {
+ nest = nla_nest_start(skb, IFLA_VXLAN_REMOTES);
+ if (nest == NULL)
+ goto nla_put_failure;
+
+ for (rdst = vxlan->default_dst.remote_next, i = 0; rdst;
+     rdst = rdst->remote_next, i++) {
+ ip = rdst->remote_ip;
+
+ rdst_nest = nla_nest_start(skb, i);

Attribute type '0' is reserved, please don't use it. Start with '1'.

Quoted 6 lines

include/uapi/linux/if_link.h enum { · line 322

Quoted 4 lines

include/uapi/linux/if_link.h +4 -0

+enum {
+ IFLA_VXLAN_REMOTE_NEW,
+ IFLA_VXLAN_REMOTE_DEL,
+};

Same here, attribute type '0' is reserved.

Does it mean I have to add _UNSPEC and _MAX? And, consequently, rename either REMOTE_{NEW,DEL} or REMOTE_ADDR and friends to avoid redefinition of _MAX attribute?

Looking at this code in more detail, I see a slew of problems.

First the list of destinations isn't really a list. The default one is still embedded in the fdb entry. This means you can't change it safely.

Also the notification via netlink only sends back a single destination value.

And the lack of locking on the open coded link list means it is not safe since the forwarding table is used with RCU. In order to be safe, proper RCU barriers would be needed or better yet convert to list_rcu..

Overall, I feel guilty for not inspecting this more closely and am surprised that others did not catch the lack of locking.

From 4de2001606a258462369520231643874e3e5c14c Mon Sep 17 00:00:00 2001

Quoted 8 lines · Stephen Hemminger

Looking at this code in more detail, I see a slew of problems.

First the list of destinations isn't really a list. The default one is still embedded in the fdb entry. This means you can't change it safely.

Also the notification via netlink only sends back a single destination value.

Quoted 6 lines

And the lack of locking on the open coded link list means it is not safe since the forwarding table is used with RCU. In order to be safe, proper RCU barriers would be needed or better yet convert to list_rcu..

Overall, I feel guilty for not inspecting this more closely and am surprised that others did not catch the lack of locking.

I've tried to convert remotes list to use hlist_rcu. The patch below implements the conversion, but it does not address the netlink notification issue.

Quoted 13 lines · On Sun, 2 Jun 2013 13:29:42 +0300 Mike Rapoport…

Quoted 12 lines · Stephen Hemminger

Looking at this code in more detail, I see a slew of problems.

First the list of destinations isn't really a list. The default one is still embedded in the fdb entry. This means you can't change it safely.

Also the notification via netlink only sends back a single destination value.

And the lack of locking on the open coded link list means it is not safe since the forwarding table is used with RCU. In order to be safe, proper RCU barriers would be needed or better yet convert to list_rcu..

Quoted 4 lines

Quoted 2 lines

Overall, I feel guilty for not inspecting this more closely and am surprised that others did not catch the lack of locking.

I've tried to convert remotes list to use hlist_rcu. The patch below implements the conversion, but it does not address the netlink notification issue.

Ok, let me have a go at this.
1. Using list rather than hlist makes more sense for handling simple list

2. The complexity is in how do do the netlink API.

3. If we can't work this out for 3.11, the multiple remotes may have to be reverted; I don't want the existing API to be exposed in a release

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +2 -2

@@ -101,7 +101,7 @@ struct vxlan_rdst {
    __be16          remote_port;
    u32          remote_vni;
    u32          remote_ifindex;
-   struct vxlan_rdst   *remote_next;
+   struct list_head    list;
 };
 
 /* Forwarding table entry */

@@ -110,7 +110,7 @@ struct vxlan_fdb {
    struct rcu_head     rcu;
    unsigned long     updated;   /* jiffies */
    unsigned long     used;
-   struct vxlan_rdst remote;
+   struct list_head  remotes;
    u16        state;   /* see ndm_state */
    u8        flags;   /* see ndm_flags */
    u8        eth_addr[ETH_ALEN];

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +2 -0

+}
+
 /* Find VXLAN socket based on network namespace and UDP port */
 static struct vxlan_sock *vxlan_find_port(struct net *net, __be16 port)
 {

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +8 -12

+
    ndm->ndm_state = fdb->state;
    ndm->ndm_ifindex = vxlan->dev->ifindex;
    ndm->ndm_flags = fdb->flags;

@@ -228,18 +235,20 @@ static int vxlan_fdb_info(struct sk_buff
    if (send_eth && nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->eth_addr))
       goto nla_put_failure;
 
-   if (send_ip && nla_put_be32(skb, NDA_DST, rdst->remote_ip))
-      goto nla_put_failure;
-
-   if (rdst->remote_port && rdst->remote_port != vxlan->dst_port &&
-       nla_put_be16(skb, NDA_PORT, rdst->remote_port))
-      goto nla_put_failure;
-   if (rdst->remote_vni != vxlan->default_dst.remote_vni &&
-       nla_put_be32(skb, NDA_VNI, rdst->remote_vni))
-      goto nla_put_failure;
-   if (rdst->remote_ifindex &&
-       nla_put_u32(skb, NDA_IFINDEX, rdst->remote_ifindex))
-      goto nla_put_failure;
+   if (rdst) {
+      if (send_ip && nla_put_be32(skb, NDA_DST, rdst->remote_ip))
+         goto nla_put_failure;
+
+      if (rdst->remote_port && rdst->remote_port != vxlan->dst_port &&
+          nla_put_be16(skb, NDA_PORT, rdst->remote_port))
+         goto nla_put_failure;

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +1 -1

@@ -1173,32 +1192,40 @@ static netdev_tx_t vxlan_xmit(struct sk_
          f = vxlan_find_mac(vxlan, eth->h_dest);
    }
 
-   if (f == NULL) {
+   if (f) {

1. eliminate second rcu for rdst free

2. handle possilble races where fdb notify called but no rdst

3. only update snooped entries for dynamic entries, shouldn't modify

4. clean up the multi-dest tx code

Quoted 21 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +2 -2

@@ -101,7 +101,7 @@ struct vxlan_rdst {
    __be16          remote_port;
    u32          remote_vni;
    u32          remote_ifindex;
-   struct vxlan_rdst   *remote_next;
+   struct list_head    list;
 };
 
 /* Forwarding table entry */

@@ -110,7 +110,7 @@ struct vxlan_fdb {
    struct rcu_head     rcu;
    unsigned long     updated;   /* jiffies */
    unsigned long     used;
-   struct vxlan_rdst remote;
+   struct list_head  remotes;
    u16        state;   /* see ndm_state */
    u8        flags;   /* see ndm_flags */
    u8        eth_addr[ETH_ALEN];

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 static inline struct hlist_head *vs_head · line 169

Quoted 7 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +4 -0

@@ -163,6 +163,12 @@ static inline struct hlist_head *vs_head
    return &vn->sock_list[hash_32(ntohs(port), PORT_HASH_BITS)];
 }
 
+/* First remote destination for a forwarding entry (or NULL) */
+static inline struct vxlan_rdst *first_remote(struct vxlan_fdb *fdb)
+{
+   return list_first_or_null_rcu(&fdb->remotes, struct vxlan_rdst, 

list);

Quoted 7 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +2 -0

+}
+
 /* Find VXLAN socket based on network namespace and UDP port */
 static struct vxlan_sock *vxlan_find_port(struct net *net, __be16 port)
 {

@@ -216,10 +222,11 @@ static int vxlan_fdb_info(struct sk_buff
 

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 static int vxlan_fdb_info(struct sk_buff · line 228

Quoted 7 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +1 -1

    if (type == RTM_GETNEIGH) {
       ndm->ndm_family   = AF_INET;
-      send_ip = rdst->remote_ip != htonl(INADDR_ANY);
+      send_ip = rdst && rdst->remote_ip != htonl(INADDR_ANY);
       send_eth = !is_zero_ether_addr(fdb->eth_addr);
    } else
       ndm->ndm_family   = AF_BRIDGE;

rdst cannot be NULL in the original code, and no fdb entry should ever have a NULL destination, so I'm not sure I understand why you appear to be sending netlink messages at all if rdst is NULL.

Quoted 36 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +15 -12

+
    ndm->ndm_state = fdb->state;
    ndm->ndm_ifindex = vxlan->dev->ifindex;
    ndm->ndm_flags = fdb->flags;

@@ -228,18 +235,20 @@ static int vxlan_fdb_info(struct sk_buff
    if (send_eth && nla_put(skb, NDA_LLADDR, ETH_ALEN, &fdb->eth_addr))
       goto nla_put_failure;
 
-   if (send_ip && nla_put_be32(skb, NDA_DST, rdst->remote_ip))
-      goto nla_put_failure;
-
-   if (rdst->remote_port && rdst->remote_port != vxlan->dst_port &&
-       nla_put_be16(skb, NDA_PORT, rdst->remote_port))
-      goto nla_put_failure;
-   if (rdst->remote_vni != vxlan->default_dst.remote_vni &&
-       nla_put_be32(skb, NDA_VNI, rdst->remote_vni))
-      goto nla_put_failure;
-   if (rdst->remote_ifindex &&
-       nla_put_u32(skb, NDA_IFINDEX, rdst->remote_ifindex))
-      goto nla_put_failure;
+   if (rdst) {
+      if (send_ip && nla_put_be32(skb, NDA_DST, rdst->remote_ip))
+         goto nla_put_failure;
+
+      if (rdst->remote_port && rdst->remote_port != vxlan->dst_port &&
+          nla_put_be16(skb, NDA_PORT, rdst->remote_port))
+         goto nla_put_failure;
+      if (rdst->remote_vni != vxlan->default_dst.remote_vni &&
+          nla_put_be32(skb, NDA_VNI, rdst->remote_vni))
+         goto nla_put_failure;
+      if (rdst->remote_ifindex &&
+          nla_put_u32(skb, NDA_IFINDEX, rdst->remote_ifindex))
+         goto nla_put_failure;
+   }
 

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 static int vxlan_fdb_info(struct sk_buff · line 254

Quoted 2 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +0 -0

    ci.ndm_used    = jiffies_to_clock_t(now - fdb->used);
    ci.ndm_confirmed = 0;

Again, if rdst == NULL, why are we sending anything? It should never happen, so should be an error case if you check at all, shouldn't it?

Quoted 5 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +0 -0

@@ -1173,32 +1192,40 @@ static netdev_tx_t vxlan_xmit(struct sk_
          f = vxlan_find_mac(vxlan, eth->h_dest);
    }
 

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 static netdev_tx_t vxlan_xmit(struct sk_ · line 1205

Quoted 12 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +11 -1

-   if (f == NULL) {
+   if (f) {
+      struct vxlan_rdst *rdst;
+
+      rdst0 = NULL;
+      list_for_each_entry_rcu(rdst, &f->remotes, list) {
+         if (rdst0) {
+            struct sk_buff *skb1;
+            skb1 = skb_clone(skb, GFP_ATOMIC);
+            if (skb1)
+               vxlan_xmit_one(skb1, dev,
+                         rdst0, did_rsc);

This ignores the return value of vxlan_xmit_one(); the original code returns an error if any of the destinations fail, while this code ignores errors for all but the last destination.

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 static netdev_tx_t vxlan_xmit(struct sk_ · line 1217

Quoted 12 lines

drivers/net/vxlan.c 2013-06-03 11:11:37.220114181 -0700 renamed from drivers/net/vxlan.c 2013-06-03 11:08:39.214230482 -0700 +12 -0

+            else
+               ++dev->stats.tx_dropped;
+         }
+         rdst0 = rdst;
+      }
+
+      if (!rdst0) {
+         /* forwarding entry but destination list empty */
+         ++dev->stats.tx_dropped;
+         kfree_skb(skb);
+         return NETDEV_TX_OK;
+      }

This should be a panic, in the original code -- it can't
happen unless the fdb table is corrupted.

You say:

My changes:

Quoted 1 line

1. eliminate second rcu for rdst free

Why is this a good thing?

Quoted 2 lines

2. handle possilble races where fdb notify called but no rdst

How can an fdb entry ever not have an rdst? Isn't that a reason to remove the fdb entry? I don't count "0.0.0.0" as not having an rdst, since that is explicitly used for ARP reduction suppression, but rather I mean: when can a netlink message adding an fdb entry not have an NDA_DST (even if it is 0.0.0.0) and why would that not be an error to add an fdb entry without a dst?

Quoted 1 line

3. only update snooped entries for dynamic entries, shouldn't modify

static entries

I agree with this part.

Quoted 1 line

4. clean up the multi-dest tx code

Everybody likes their own code, of course, but I think the original was cleaner. A vxlan_rdst was part of an fdb structure because all fdbs require a remote destination, while the clean-up changes that to a list which may then be NULL, and in my opinion is notably less clean. I don't know the reasoning for that-- perhaps some thread I missed?

So, I guess I don't see what you're trying to fix here, other than preventing snoop updates on static entries, which I agree with.

                                                        +-DLS

Just being safe. need to audit to make sure not possible for all destinations to be removed from FDB entry via netlink.

Quoted 3 lines

Again, if rdst == NULL, why are we sending anything? It should never happen, so should be an error case if you check at all, shouldn't it?

It maybe possible to create or modify existing fdb entry so no

left.

Quoted 3 lines

This ignores the return value of vxlan_xmit_one(); the original code returns an error if any of the destinations fail, while this code ignores errors for all but the last destination.

Return value doesn't really matter here anyway, and certainly not in the fanout case.

But i don't think any of this fdb code relates to the default destination directly, since the generic fdb code does not support default, or any other aggregate matching. VXLAN can define the netlink API for default destinations without affecting anything else, because it is already a per-device feature.

                                                        +-DLS

Quoted 6 lines

But i don't think any of this fdb code relates to the default destination directly, since the generic fdb code does not support default, or any other aggregate matching. VXLAN can define the netlink API for default destinations without affecting anything else, because it is already a per-device feature.

Quoted 1 line

                                                        +-DLS

The only relation is the 'struct vxlan_rdst' which I intended to use for default destinations list. So, until there's a consensus about remote destinations list in fdb, I cannot continue to work on multiple default destinations.

Mike Rapoport <[email protected]> wrote on 06/04/2013 05:18:08 AM:

Actually, we could make it an fdb and also have "use" and "updated" stats that way, and/or use the existing fdb code and add an entry with MAC "00:00:00:00:00:00" [which is disallowed via the generic fdb code, so can't be in the fdb table otherwise].

From: Mike Rapoport <[email protected]>

If I've understood you right it would be something like this:

(quoted diff) +0 -2

@@ -1025,13 +1027,10 @@ static netdev_tx_t vxlan_xmit_one(struct
 sk_buff *skb, struct net_device *dev,
  vni = rdst->remote_vni;
  dst = rdst->remote_ip;
 
- if (!dst) {
- if (did_rsc) {

[email protected] wrote on 06/04/2013 01:20:22 PM:

Quoted 4 lines

From: Mike Rapoport <[email protected]>

If I've understood you right it would be something like this:

All the tabs are removed in the code I got-- don't know if it's your mailer or my reader....

This isn't correct -- I led you into that one,
but the original code is what we'd need -- it isn't "rdst NULL" this is checking, but really "dst == INADDR_ANY", which is probably what it ought to be, instead of "0". At any rate, the check there works and needs to stay; dst == INADDR_ANY is a distinct case from rdst == NULL.

Also looks like you're missing a brace on the L2MISS check -- the miss and the drop are only if the all-zeroes lookup fails.

But the general idea looks good. If you remove the is_zero_ether_addr() check in rtnl_fdb_add and rtnl_fdb_del, then the existing fdb_add/fdb_append/fdb_del code code would allow changes, including multiple destinations, to the default destinations as well.

But there's more to it, too -- default_dst and all its references would need to be replaced for joining groups and the netlink part, and removed from vxlan_dev.

                                                        +-DLS

Quoted 1 line

4. clean up the multi-dest tx code

Everybody likes their own code, of course, but I think the original was cleaner. A vxlan_rdst was part of an fdb structure because all fdbs require a remote destination, while the clean-up changes that to a list which may then be NULL, and in my opinion is notably less clean. I don't know the reasoning for that-- perhaps some thread I missed?

So, I guess I don't see what you're trying to fix here, other than preventing snoop updates on static entries, which I agree with.

                                                        +-DLS

I am still not 100% convinced of the use case of providing multiple destinations.
It seems like either a solution for migrating guests, a way to do SPAN, or a weird
form of multicast.

Wouldn't it be better to use something like TC actions for these special cases and
handle it at the IP layer rather than having to add so much complexity into the
VXLAN driver itself.

Stephen Hemminger <[email protected]> wrote on 06/04/2013 12:00:31 PM:

Quoted 5 lines

I am still not 100% convinced of the use case of providing multiple destinations.
It seems like either a solution for migrating guests, a way to do SPAN, or a weird
form of multicast.

The use is, as you would say it, a weird form of multicast. It is to allow forwarding MAC multicasts without relying on the underlying network having multicast routers in the same domain.

Quoted 5 lines

Wouldn't it be better to use something like TC actions for these special cases and
handle it at the IP layer rather than having to add so much complexity into the
VXLAN driver itself.

This isn't a "special case" for DOVE -- it is the mechanism for doing MAC multicasts. I don't agree that the code is complex -- it simply replaces what was a vxlan_rdst with a list of vxlan_rdsts. And, no, I don't believe it can be done with equivalent performance using external mechanisms.

Didn't we have this discussion when the patch went in?

                                                        +-DLS

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signed-off-by Mike Rapoport <[email protected]>

---
 ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

ip/iplink_vxlan.c +0 -0

index 1025326..be6c0ac 100644

Quoted 5 lines · Mike Rapoport

Signed-off-by Mike Rapoport <[email protected]>

---
 ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

ip/iplink_vxlan.c static void explain(void) · line 32

Quoted 10 lines

ip/iplink_vxlan.c +2 -0

index 1025326..be6c0ac 100644
@@ -28,11 +28,87 @@ static void explain(void)
      fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
      fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
      fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
+     fprintf(stderr, "                 [ dstadd DST ]\n");
+     fprintf(stderr, "                 [ dstdel ADDR ]\n");

Excuse me, but this looks like a design failure as you manipulate remotes with ip link while creating vxlan devices, shouldn't this be in a standard alone tool if we can't reuse any existing tool? Or am I missing anything?

From one side it seems very logical to use ip/iplink_vxlan for vxlan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Cong Wang

Quoted 5 lines · Mike Rapoport

Signed-off-by Mike Rapoport <[email protected]>

---
 ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

Quoted 15 lines

ip/iplink_vxlan.c static void explain(void) · line 32

Quoted 10 lines

ip/iplink_vxlan.c +2 -0

index 1025326..be6c0ac 100644
@@ -28,11 +28,87 @@ static void explain(void)
      fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
      fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
      fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
+     fprintf(stderr, "                 [ dstadd DST ]\n");
+     fprintf(stderr, "                 [ dstdel ADDR ]\n");

Excuse me, but this looks like a design failure as you manipulate remotes with ip link while creating vxlan devices, shouldn't this be in a standard alone tool if we can't reuse any existing tool? Or am I missing anything?

Frankly, I had a long hesitation about the userspace implementation. From one side it seems very logical to use ip/iplink_vxlan for vxlan device manipulations. Moreover, since the remotes are used pretty much the same way as the group address, adding the remotes management to ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone tool for remote list manipulation in vxlan seemed to me little bit far fetched.

On the other hand, I quite agree with you that
ip link add vxlan0 ... dstadd 192.168.1.1
or
ip link set vxlan0 ... dstdel 192.168.1.1
looks weird at least.

Quoted 31 lines · On Wed, 29 May 2013 13:52:55 +0300 Mike Rapoport…

Quoted 6 lines · Cong Wang

Quoted 5 lines · Mike Rapoport

Signed-off-by Mike Rapoport <[email protected]>

---
 ip/iplink_vxlan.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

Quoted 15 lines

ip/iplink_vxlan.c static void explain(void) · line 32

Quoted 10 lines

ip/iplink_vxlan.c +2 -0

index 1025326..be6c0ac 100644
@@ -28,11 +28,87 @@ static void explain(void)
      fprintf(stderr, "                 [ port MIN MAX ] [ [no]learning ]\n");
      fprintf(stderr, "                 [ [no]proxy ] [ [no]rsc ]\n");
      fprintf(stderr, "                 [ [no]l2miss ] [ [no]l3miss ]\n");
+     fprintf(stderr, "                 [ dstadd DST ]\n");
+     fprintf(stderr, "                 [ dstdel ADDR ]\n");

Excuse me, but this looks like a design failure as you manipulate remotes with ip link while creating vxlan devices, shouldn't this be in a standard alone tool if we can't reuse any existing tool? Or am I missing anything?

Frankly, I had a long hesitation about the userspace implementation. From one side it seems very logical to use ip/iplink_vxlan for vxlan device manipulations. Moreover, since the remotes are used pretty much the same way as the group address, adding the remotes management to ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone tool for remote list manipulation in vxlan seemed to me little bit far fetched.

Quoted 5 lines

On the other hand, I quite agree with you that
ip link add vxlan0 ... dstadd 192.168.1.1
or
ip link set vxlan0 ... dstdel 192.168.1.1
looks weird at least.

Don't like add/delete semantics here either.
Maybe replace or modify,

or has this grown enough that having its own
command line tool "vxlan ..." makes sense?

Quoted 16 lines · Stephen Hemminger

Quoted 13 lines · On Wed, 29 May 2013 13:52:55 +0300 Mike Rapoport…

Frankly, I had a long hesitation about the userspace implementation. From one side it seems very logical to use ip/iplink_vxlan for vxlan device manipulations. Moreover, since the remotes are used pretty much the same way as the group address, adding the remotes management to ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone tool for remote list manipulation in vxlan seemed to me little bit far fetched.

On the other hand, I quite agree with you that
ip link add vxlan0 ... dstadd 192.168.1.1
or
ip link set vxlan0 ... dstdel 192.168.1.1
looks weird at least.

Quoted 2 lines

Don't like add/delete semantics here either.
Maybe replace or modify,

I think that replace or modify do not express the actual operation meaning. My intention with dstadd was "add remote host X to pseudo-multicast group". Replace/modify maybe nice to have features to avoid doing delete+ add.

Quoted 2 lines

or has this grown enough that having its own
command line tool "vxlan ..." makes sense?

Say, misc/vxlan that will handle remote destinations management? Or should it take care of some vxlan parameters currently implemented in ip/iplink_vxlan and bridge/fdb?

Quoted 18 lines · Mike Rapoport

Quoted 16 lines · Stephen Hemminger

Quoted 13 lines · On Wed, 29 May 2013 13:52:55 +0300 Mike Rapoport…

Frankly, I had a long hesitation about the userspace implementation. From one side it seems very logical to use ip/iplink_vxlan for vxlan device manipulations. Moreover, since the remotes are used pretty much the same way as the group address, adding the remotes management to ip/iplink_vxlan makes a lot of sense. Besides, creation of stand alone tool for remote list manipulation in vxlan seemed to me little bit far fetched.

On the other hand, I quite agree with you that
ip link add vxlan0 ... dstadd 192.168.1.1
or
ip link set vxlan0 ... dstdel 192.168.1.1
looks weird at least.

Quoted 7 lines

Quoted 2 lines

Don't like add/delete semantics here either.
Maybe replace or modify,

I think that replace or modify do not express the actual operation meaning. My intention with dstadd was "add remote host X to pseudo-multicast group". Replace/modify maybe nice to have features to avoid doing delete+ add.

The alternative would be to require iproute2 to always provide the full list of remote addresses like we do we route nexthops.

I do like the add/del though and don't see a problem with requiring an ''ip link set [..] dstadd/dstdel''

Quoted 6 lines

Quoted 2 lines

or has this grown enough that having its own
command line tool "vxlan ..." makes sense?

Say, misc/vxlan that will handle remote destinations management? Or should it take care of some vxlan parameters currently implemented in ip/iplink_vxlan and bridge/fdb?

What do we gain from a separate tool?

I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother me is that you can't have different parameters for "ip link add" and "ip link set" for vxlan (and other iplink) utility. So, one can use ip link add [..] dstdel
which does not make sense...

Quoted 5 lines · Mike Rapoport

I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother me is that you can't have different parameters for "ip link add" and "ip link set" for vxlan (and other iplink) utility. So, one can use ip link add [..] dstdel
which does not make sense...

You can easily pass an additional argument into iplink_modify() and exclude certain options in the "add" use case.

Quoted 9 lines · Thomas Graf

Quoted 5 lines · Mike Rapoport

I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother me is that you can't have different parameters for "ip link add" and "ip link set" for vxlan (and other iplink) utility. So, one can use ip link add [..] dstdel
which does not make sense...

You can easily pass an additional argument into iplink_modify() and exclude certain options in the "add" use case.

I think there's no need to pass an additional argument to iplink_modify. The vxlan_parse_opts may check the flags in nlmsghdr to distinguish between the "add" and "set" cases.
Than we'll have 'ip link add [..]' as it was and the 'ip link set [..]' will be used to manage default destinations.

--
Sincerely yours,
Mike.

Quoted 17 lines · On Sun, 2 Jun 2013 10:09:23 +0300 Mike Rapoport…

Quoted 9 lines · Thomas Graf

Quoted 5 lines · Mike Rapoport

I'm feeling Ok about "ip link set [..] dstadd/dstdel". What does bother me is that you can't have different parameters for "ip link add" and "ip link set" for vxlan (and other iplink) utility. So, one can use ip link add [..] dstdel
which does not make sense...

You can easily pass an additional argument into iplink_modify() and exclude certain options in the "add" use case.

I think there's no need to pass an additional argument to iplink_modify. The vxlan_parse_opts may check the flags in nlmsghdr to distinguish between the "add" and "set" cases.
Than we'll have 'ip link add [..]' as it was and the 'ip link set [..]' will be used to manage default destinations.

Quoted 3 lines

--
Sincerely yours,
Mike.

I think multiple destinations should be handled like multipath routes. I.e you don't specify multiple destinations on the command line, you specify them individually and can add/delete them

If you delete the last destination then the forwarding entry should disappear. The collapsing of multiple entries into one entry in table is an internal data structure choice of vxlan and shouldn't be part of the netlink API requirement.

The API to iproute2/netlink should look like routing (through bridge fdb command). Feel free to reject this if since I don't actually use this stuff.

Quoted 3 lines

Say, misc/vxlan that will handle remote destinations management? Or should it take care of some vxlan parameters currently implemented in ip/iplink_vxlan and bridge/fdb?

What do we gain from a separate tool?