Fix bug that causes the static-mac-move test to fail (#128)

Problem:

The [static-mac mac moves] test fails against a P4-enabled OVS
build because the mac_learning_static_none_move counter is 2
instead of 1.

Analysis:

is_mac_learning_update_needed() increments the counter in addition
to determining whether an update needs to be made. The P4 code adds
a second call to this function, resulting in the counter being
incremented twice for the same packet.

Solution:

- Removed the counter increment from is_mac_learning_update_needed()
  and modified the function to return a Boolean parameter indicating
  whether the event is a static mac move.

- Modified mac_learning_update() to supply the new parameter and
  increment the counter if its returned value is True.

- Modified xlate_normal() to supply the new parameter and ignore
  the returned value

Outcome:

With this change in place, the test no longer fails.

Signed-off-by: Derek Foster <[email protected]>

lib/mac-learning.c

@@ -456,12 +456,15 @@ bool
 is_mac_learning_update_needed(const struct mac_learning *ml,
                               struct eth_addr src, int vlan,
                               bool is_gratuitous_arp, bool is_bond,
-                              void *in_port)
+                              const void *in_port, bool *is_static_move)
     OVS_REQ_RDLOCK(ml->rwlock)
 {
     struct mac_entry *mac;
+    bool is_port_move;
     int age;
 
+    *is_static_move = false;
+
     if (!mac_learning_may_learn(ml, src, vlan)) {
         return false;
     }
@@ -472,14 +475,13 @@ is_mac_learning_update_needed(const struct mac_learning *ml,
         return true;
     }
 
+    /* Check whether address is on a different port. */
+    is_port_move = mac_entry_get_port(ml, mac) != in_port;
+
     age = mac_entry_age(ml, mac);
     /* If mac is a static entry, then there is no need to update. */
     if (age == MAC_ENTRY_AGE_STATIC_ENTRY) {
-        /* Coverage counter to increment when a packet with same
-         * static-mac appears on a different port. */
-        if (mac_entry_get_port(ml, mac) != in_port) {
-            COVERAGE_INC(mac_learning_static_none_move);
-        }
+        *is_static_move = is_port_move;
         return false;
     }
 
@@ -500,7 +502,7 @@ is_mac_learning_update_needed(const struct mac_learning *ml,
         }
     }
 
-    return mac_entry_get_port(ml, mac) != in_port /* ofbundle */;
+    return is_port_move;
 }
 
 /* Updates MAC learning table 'ml' given that a packet matching 'src' was
@@ -568,16 +570,23 @@ mac_learning_update(struct mac_learning *ml, struct eth_addr src,
                     void *in_port)
     OVS_EXCLUDED(ml->rwlock)
 {
-    bool need_update;
+    bool is_static_move = false;
+    bool need_update = false;
     bool updated = false;
 
     /* Don't learn the OFPP_NONE port. */
     if (in_port != NULL) {
         /* First try the common case: no change to MAC learning table. */
         ovs_rwlock_rdlock(&ml->rwlock);
         need_update = is_mac_learning_update_needed(ml, src, vlan,
-                                                    is_gratuitous_arp, is_bond,
-                                                    in_port);
+                                                    is_gratuitous_arp,
+                                                    is_bond, in_port,
+                                                    &is_static_move);
+        if (is_static_move) {
+            /* Coverage counter to increment when a packet with same
+             * static-mac appears on a different port. */
+            COVERAGE_INC(mac_learning_static_none_move);
+        }
         ovs_rwlock_unlock(&ml->rwlock);
 
         if (need_update) {

lib/mac-learning.h

@@ -230,11 +230,10 @@ bool mac_learning_may_learn(const struct mac_learning *ml,
                             const struct eth_addr src_mac,
                             uint16_t vlan)
     OVS_REQ_RDLOCK(ml->rwlock);
-bool
-is_mac_learning_update_needed(const struct mac_learning *ml,
-                              struct eth_addr src, int vlan,
-                              bool is_gratuitous_arp, bool is_bond,
-                              void *in_port)
+bool is_mac_learning_update_needed(const struct mac_learning *ml,
+                                   struct eth_addr src, int vlan,
+                                   bool is_gratuitous_arp, bool is_bond,
+                                   const void *in_port, bool *is_static_move)
     OVS_REQ_RDLOCK(ml->rwlock);
 struct mac_entry *mac_learning_insert(struct mac_learning *ml,
                                       const struct eth_addr src,

ofproto/ofproto-dpif-xlate.c

@@ -3198,17 +3198,20 @@ update_ip_mac_map_info(const struct flow *flow,
         return -1;
     }
 
-    memcpy(ip_mac_map_info->src_mac_addr, flow->dl_src.ea, sizeof(ip_mac_map_info->src_mac_addr));
-    memcpy(ip_mac_map_info->dst_mac_addr, flow->dl_dst.ea, sizeof(ip_mac_map_info->dst_mac_addr));
+    memcpy(ip_mac_map_info->src_mac_addr, flow->dl_src.ea,
+           sizeof(ip_mac_map_info->src_mac_addr));
+    memcpy(ip_mac_map_info->dst_mac_addr, flow->dl_dst.ea,
+           sizeof(ip_mac_map_info->dst_mac_addr));
 
-    //Program the entiry only for an ARP response where we have valid IP's and MAC for both src and dst
+    // Program the entry only for an ARP response where we have valid IPs
+    // and MAC for both src and dst.
     if (valid_ip_addr(flow->nw_src) && !eth_addr_is_broadcast(flow->dl_src) &&
-       valid_ip_addr(flow->nw_dst) && !eth_addr_is_broadcast(flow->dl_dst)) {
-       ip_mac_map_info->src_ip_addr.family = AF_INET;
-       ip_mac_map_info->src_ip_addr.ip.v4addr.s_addr = flow->nw_src;
+        valid_ip_addr(flow->nw_dst) && !eth_addr_is_broadcast(flow->dl_dst)) {
+        ip_mac_map_info->src_ip_addr.family = AF_INET;
+        ip_mac_map_info->src_ip_addr.ip.v4addr.s_addr = flow->nw_src;
 
-       ip_mac_map_info->dst_ip_addr.family = AF_INET;
-       ip_mac_map_info->dst_ip_addr.ip.v4addr.s_addr = flow->nw_dst;
+        ip_mac_map_info->dst_ip_addr.family = AF_INET;
+        ip_mac_map_info->dst_ip_addr.ip.v4addr.s_addr = flow->nw_dst;
     }
 
     return -1;
@@ -3221,10 +3224,10 @@ xlate_normal(struct xlate_ctx *ctx)
 {
     struct flow_wildcards *wc = ctx->wc;
     struct flow *flow = &ctx->xin->flow;
+    struct xbundle *in_xbundle;
 #if defined(P4OVS)
-    bool is_mac_learn_required = false;
+    bool need_update = false;
 #endif
-    struct xbundle *in_xbundle;
     struct xport *in_port;
     struct mac_entry *mac;
     void *mac_port;
@@ -3288,18 +3291,22 @@ xlate_normal(struct xlate_ctx *ctx)
         && in_port && in_port->pt_mode != NETDEV_PT_LEGACY_L3
     ) {
 #if defined(P4OVS)
-        is_mac_learn_required = is_mac_learning_update_needed(ctx->xbridge->ml,
-                                flow->dl_src, vlan,is_grat_arp,
-                                in_xbundle->bond != NULL,
-                                in_xbundle->ofbundle);
+        bool is_static_move = false;
+        need_update = is_mac_learning_update_needed(ctx->xbridge->ml,
+                                                    flow->dl_src,
+                                                    vlan, is_grat_arp,
+                                                    in_xbundle->bond != NULL,
+                                                    in_xbundle->ofbundle,
+                                                    &is_static_move);
+        /* ignore is_static_move */
 #endif
-        //The function below calls mac_learning_insert
+        // The function below calls mac_learning_insert
         update_learning_table(ctx, in_xbundle, flow->dl_src, vlan,
                               is_grat_arp);
     }
 
 #if defined(P4OVS)
-    if (is_mac_learn_required) {
+    if (need_update) {
        /* Dynamic MAC is learnt, program P4 forwarding table */
        struct xport *ovs_port = get_ofp_port(in_xbundle->xbridge,
                                              flow->in_port.ofp_port);