[ovs-dev] Tunnel matching and remote_ip=flow (was Re: [ovs-discuss] vxlan remote_ip flow help)

Ben Pfaff blp at nicira.com
Sat Feb 1 00:59:28 UTC 2014


On Fri, Jan 31, 2014 at 10:38:20AM -0800, Jesse Gross wrote:
> On Thu, Jan 30, 2014 at 1:31 PM, Thomas Morin <thomas.morin at orange.com> wrote:
> > Hi,
> >
> > I ran into the same issue as the one described below (post on ovs-discuss a
> > few months back).
> >
> > The goal is to use a vport tunnel configured to use a flow-based remote IP,
> > e.g:
> >     ovs-vscl add-port br-vnet tun-port -- set Interface tun-port type=gre
> > options:remote_ip=flow
> >
> > This works fine on the sending OVS, but on the receiving OVS the same vport
> > won't receive any traffic (packets received trigger a "receive tunnel port
> > not found" warning, and are not further processed).
> >
> > The original poster had found a workaround relying on also using
> > options:key=flow, but as far as I understand, this work around should not be
> > necessary: it just seems that the tunnel matching code is missing a match.
> >
> > --- a/ofproto/tunnel.c
> > +++ b/ofproto/tunnel.c
> > @@ -427,8 +427,9 @@ tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
> >          { false, false, IP_SRC_ANY },  /* remote_ip, in_key. */
> >          { true,  false, IP_SRC_CFG },  /* remote_ip, local_ip. */
> >          { true,  false, IP_SRC_ANY },  /* remote_ip. */
> > -        { true,  true,  IP_SRC_ANY },  /* Flow-based remote. */
> > +        { true,  true,  IP_SRC_ANY },  /* Flow-based remote and flow-based
> > key. */
> >          { true,  true,  IP_SRC_FLOW }, /* Flow-based everything. */
> > +        { false, true,  IP_SRC_ANY },  /* Flow-based remote */
> >      };
> >
> >      const struct tnl_match_pattern *p;
> >
> > With this patch, a tunnel configured with just remote_ip=flow will work fine
> > on the receiving end.
> > One thing that I'm not sure is at which position this pattern should appear
> > in the list.
> > The patch also changes the comment for true/true/IP_SRC_ANY.
> 
> This looks fine to me although I think that it the new entry belongs
> first in the list of flow-based types. Can you submit a formal patch
> with this change?

By my count there are 12 possible entries in the list of
possibilities.  I don't know if there's any point in adding them one
by one.  I guess we might as well add all of them at one
systematically.

How about this, then?  It's not correct--I haven't debugged it--but it
gives the flavor of the idea.

diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index 09497a3..608f861 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -60,8 +60,20 @@ struct tnl_port {
 
 static struct ovs_rwlock rwlock = OVS_RWLOCK_INITIALIZER;
 
-static struct hmap tnl_match_map__ = HMAP_INITIALIZER(&tnl_match_map__);
-static struct hmap *tnl_match_map OVS_GUARDED_BY(rwlock) = &tnl_match_map__;
+enum ip_src_type {
+    IP_SRC_CFG,             /* ip_src must equal configured address. */
+    IP_SRC_ANY,             /* Any ip_src is acceptable. */
+    IP_SRC_FLOW             /* ip_src is handled in flow table. */
+};
+
+#define N_MATCH_TYPES (3 * 2 * 2)
+
+static struct hmap *tnl_match_maps[N_MATCH_TYPES] OVS_GUARDED_BY(rwlock);
+static struct hmap **
+tnl_match_map(bool in_key_flow, bool ip_dst_flow, enum ip_src_type ip_src)
+{
+    return &tnl_match_maps[(in_key_flow * 2 + ip_dst_flow) * 3 + ip_src];
+}
 
 static struct hmap ofport_map__ = HMAP_INITIALIZER(&ofport_map__);
 static struct hmap *ofport_map OVS_GUARDED_BY(rwlock) = &ofport_map__;
@@ -70,7 +82,7 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 static struct vlog_rate_limit dbg_rl = VLOG_RATE_LIMIT_INIT(60, 60);
 
 static struct tnl_port *tnl_find(const struct flow *) OVS_REQ_RDLOCK(rwlock);
-static struct tnl_port *tnl_find_exact(struct tnl_match *)
+static struct tnl_port *tnl_find_exact(struct tnl_match *, struct hmap *)
     OVS_REQ_RDLOCK(rwlock);
 static struct tnl_port *tnl_find_ofport(const struct ofport_dpif *)
     OVS_REQ_RDLOCK(rwlock);
@@ -92,6 +104,7 @@ tnl_port_add__(const struct ofport_dpif *ofport, const struct netdev *netdev,
     const struct netdev_tunnel_config *cfg;
     struct tnl_port *existing_port;
     struct tnl_port *tnl_port;
+    struct hmap **map;
 
     cfg = netdev_get_tunnel_config(netdev);
     ovs_assert(cfg);
@@ -110,7 +123,12 @@ tnl_port_add__(const struct ofport_dpif *ofport, const struct netdev *netdev,
     tnl_port->match.in_key_flow = cfg->in_key_flow;
     tnl_port->match.odp_port = odp_port;
 
-    existing_port = tnl_find_exact(&tnl_port->match);
+    map = tnl_match_map(tnl_port->match.in_key_flow,
+                         tnl_port->match.ip_dst_flow,
+                         (tnl_port->match.ip_dst_flow ? IP_SRC_FLOW
+                          : tnl_port->match.ip_src ? IP_SRC_CFG
+                          : IP_SRC_ANY));
+    existing_port = tnl_find_exact(&tnl_port->match, *map);
     if (existing_port) {
         if (warn) {
             struct ds ds = DS_EMPTY_INITIALIZER;
@@ -125,8 +143,12 @@ tnl_port_add__(const struct ofport_dpif *ofport, const struct netdev *netdev,
     }
 
     hmap_insert(ofport_map, &tnl_port->ofport_node, hash_pointer(ofport, 0));
-    hmap_insert(tnl_match_map, &tnl_port->match_node,
-                tnl_hash(&tnl_port->match));
+
+    if (!*map) {
+        *map = xmalloc(sizeof **map);
+        hmap_init(*map);
+    }
+    hmap_insert(*map, &tnl_port->match_node, tnl_hash(&tnl_port->match));
     tnl_port_mod_log(tnl_port, "adding");
     return true;
 }
@@ -182,8 +204,20 @@ tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
 
     tnl_port = tnl_find_ofport(ofport);
     if (tnl_port) {
+        struct hmap **map;
+
         tnl_port_mod_log(tnl_port, "removing");
-        hmap_remove(tnl_match_map, &tnl_port->match_node);
+        map = tnl_match_map(tnl_port->match.in_key_flow,
+                            tnl_port->match.ip_dst_flow,
+                            (tnl_port->match.ip_dst_flow ? IP_SRC_FLOW
+                             : tnl_port->match.ip_src ? IP_SRC_CFG
+                             : IP_SRC_ANY));
+        hmap_remove(*map, &tnl_port->match_node);
+        if (hmap_is_empty(*map)) {
+            hmap_destroy(*map);
+            free(*map);
+            *map = NULL;
+        }
         hmap_remove(ofport_map, &tnl_port->ofport_node);
         netdev_close(tnl_port->netdev);
         free(tnl_port);
@@ -396,14 +430,16 @@ tnl_find_ofport(const struct ofport_dpif *ofport) OVS_REQ_RDLOCK(rwlock)
 }
 
 static struct tnl_port *
-tnl_find_exact(struct tnl_match *match) OVS_REQ_RDLOCK(rwlock)
+tnl_find_exact(struct tnl_match *match, struct hmap *map)
+    OVS_REQ_RDLOCK(rwlock)
 {
-    struct tnl_port *tnl_port;
+    if (map) {
+        struct tnl_port *tnl_port;
 
-    HMAP_FOR_EACH_WITH_HASH (tnl_port, match_node, tnl_hash(match),
-                             tnl_match_map) {
-        if (!memcmp(match, &tnl_port->match, sizeof *match)) {
-            return tnl_port;
+        HMAP_FOR_EACH_WITH_HASH (tnl_port, match_node, tnl_hash(match), map) {
+            if (!memcmp(match, &tnl_port->match, sizeof *match)) {
+                return tnl_port;
+            }
         }
     }
     return NULL;
@@ -414,49 +450,42 @@ tnl_find_exact(struct tnl_match *match) OVS_REQ_RDLOCK(rwlock)
 static struct tnl_port *
 tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
 {
-    enum ip_src_type {
-        IP_SRC_CFG,             /* ip_src must equal configured address. */
-        IP_SRC_ANY,             /* Any ip_src is acceptable. */
-        IP_SRC_FLOW             /* ip_src is handled in flow table. */
-    };
-
-    struct tnl_match_pattern {
-        bool in_key_flow;
-        bool ip_dst_flow;
-        enum ip_src_type ip_src;
-    };
-
-    static const struct tnl_match_pattern patterns[] = {
-        { false, false, IP_SRC_CFG },  /* remote_ip, local_ip, in_key. */
-        { false, false, IP_SRC_ANY },  /* remote_ip, in_key. */
-        { true,  false, IP_SRC_CFG },  /* remote_ip, local_ip. */
-        { true,  false, IP_SRC_ANY },  /* remote_ip. */
-        { true,  true,  IP_SRC_ANY },  /* Flow-based remote. */
-        { true,  true,  IP_SRC_FLOW }, /* Flow-based everything. */
-    };
-
-    const struct tnl_match_pattern *p;
-    struct tnl_match match;
-
-    memset(&match, 0, sizeof match);
-    match.odp_port = flow->in_port.odp_port;
-    match.pkt_mark = flow->pkt_mark;
-
-    for (p = patterns; p < &patterns[ARRAY_SIZE(patterns)]; p++) {
-        struct tnl_port *tnl_port;
-
-        match.in_key_flow = p->in_key_flow;
-        match.in_key = p->in_key_flow ? 0 : flow->tunnel.tun_id;
-
-        match.ip_dst_flow = p->ip_dst_flow;
-        match.ip_dst = p->ip_dst_flow ? 0 : flow->tunnel.ip_src;
-
-        match.ip_src_flow = p->ip_src == IP_SRC_FLOW;
-        match.ip_src = p->ip_src == IP_SRC_CFG ? flow->tunnel.ip_dst : 0;
-
-        tnl_port = tnl_find_exact(&match);
-        if (tnl_port) {
-            return tnl_port;
+    enum ip_src_type ip_src;
+    int in_key_flow;
+    int ip_dst_flow;
+    int i;
+
+    i = 0;
+    for (in_key_flow = 0; in_key_flow < 2; in_key_flow++) {
+        for (ip_dst_flow = 0; ip_dst_flow < 2; ip_dst_flow++) {
+            for (ip_src = 0; ip_src < 3; ip_src++) {
+                struct hmap *map = tnl_match_maps[i];
+
+                if (map) {
+                    struct tnl_port *tnl_port;
+                    struct tnl_match match;
+
+                    memset(&match, 0, sizeof match);
+
+                    match.in_key = in_key_flow ? 0 : flow->tunnel.tun_id;
+                    match.ip_src = (ip_src == IP_SRC_CFG
+                                    ? flow->tunnel.ip_dst
+                                    : 0);
+                    match.ip_dst = ip_dst_flow ? 0 : flow->tunnel.ip_src;
+                    match.odp_port = flow->in_port.odp_port;
+                    match.pkt_mark = flow->pkt_mark;
+                    match.in_key_flow = in_key_flow;
+                    match.ip_dst_flow = ip_dst_flow;
+                    match.ip_src_flow = ip_src == IP_SRC_FLOW;
+
+                    tnl_port = tnl_find_exact(&match, map);
+                    if (tnl_port) {
+                        return tnl_port;
+                    }
+                }
+
+                i++;
+            }
         }
     }
 
-- 
1.7.10.4




More information about the dev mailing list