From 44f7f1320cd2fcb369aa9631b01832dba2ef68c2 Mon Sep 17 00:00:00 2001 From: Ameya Dharkar Date: Fri, 10 Apr 2020 17:59:31 -0700 Subject: [PATCH] zebra: Address sequencing issue while sending RMAC via FPM Issue: For consecutive messages such as MAC1 -> VTEP1 add MAC1 -> VTEP2 add MAC1 -> VTEP1 add Final state, i.e. (MAC1 -> VTEP1 add) should be sent via FPM. But, with current code, FPM will send (MAC1 -> VTEP2 add) RCA: When FPM receives (MAC1, VTEP1), it stores it in the FPM processing queue and hash table. When FPM receives (MAC1, VTEP2), this entry is stored as another node as hash table key is (mac, vtep and vni) IF FPM again receives (MAC1, VTEP1), we fetch this node in the hash table which is already enqueued. When the FPM queue is processed, we will send FPM message for (MAC1, VTEP1) first and then for (MAC1, VTEP2) This sequencing issue happened because the key of the table is (MAC, VTEP, VNI) Fix: Change the key of the hash table to (MAC, VNI) So, every time we receive a new update for (MAC1, VNI1), we will find a node in the processing queue corresponding to MAC1 if present. We will update this same node for every operation related to (MAC1, VNI1) Thus, at the time when FPM processes this node, it will have latest MAC1 info. Signed-off-by: Ameya Dharkar --- zebra/zebra_fpm.c | 61 +++++++++++++++-------------------------------- 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/zebra/zebra_fpm.c b/zebra/zebra_fpm.c index 0190ee2b8d..10cdf49d12 100644 --- a/zebra/zebra_fpm.c +++ b/zebra/zebra_fpm.c @@ -1470,8 +1470,6 @@ static int zfpm_trigger_update(struct route_node *rn, const char *reason) /* * Generate Key for FPM MAC info hash entry - * Key is generated using MAC address and VNI id which should be sufficient - * to provide uniqueness */ static unsigned int zfpm_mac_info_hash_keymake(const void *p) { @@ -1494,8 +1492,6 @@ static bool zfpm_mac_info_cmp(const void *p1, const void *p2) if (memcmp(fpm_mac1->macaddr.octet, fpm_mac2->macaddr.octet, ETH_ALEN) != 0) return false; - if (fpm_mac1->r_vtep_ip.s_addr != fpm_mac2->r_vtep_ip.s_addr) - return false; if (fpm_mac1->vni != fpm_mac2->vni) return false; @@ -1521,7 +1517,6 @@ static void *zfpm_mac_info_alloc(void *p) fpm_mac = XCALLOC(MTYPE_FPM_MAC_INFO, sizeof(struct fpm_mac_info_t)); memcpy(&fpm_mac->macaddr, &key->macaddr, ETH_ALEN); - memcpy(&fpm_mac->r_vtep_ip, &key->r_vtep_ip, sizeof(struct in_addr)); fpm_mac->vni = key->vni; return (void *)fpm_mac; @@ -1552,6 +1547,7 @@ static int zfpm_trigger_rmac_update(zebra_mac_t *rmac, zebra_l3vni_t *zl3vni, char buf[ETHER_ADDR_STRLEN]; struct fpm_mac_info_t *fpm_mac, key; struct interface *vxlan_if, *svi_if; + bool mac_found = false; /* * Ignore if the connection is down. We will update the FPM about @@ -1572,56 +1568,34 @@ static int zfpm_trigger_rmac_update(zebra_mac_t *rmac, zebra_l3vni_t *zl3vni, memset(&key, 0, sizeof(struct fpm_mac_info_t)); memcpy(&key.macaddr, &rmac->macaddr, ETH_ALEN); - key.r_vtep_ip.s_addr = rmac->fwd_info.r_vtep_ip.s_addr; key.vni = zl3vni->vni; /* Check if this MAC is already present in the queue. */ fpm_mac = zfpm_mac_info_lookup(&key); if (fpm_mac) { - if (!!CHECK_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_DELETE_FPM) - == delete) { - /* - * MAC is already present in the queue - * with the same op as this one. Do nothing - */ - zfpm_g->stats.redundant_triggers++; - return 0; - } + mac_found = true; /* - * A new op for an already existing fpm_mac_info_t node. - * Update the existing node for the new op. + * If the enqueued op is "add" and current op is "delete", + * this is a noop. So, Unset ZEBRA_MAC_UPDATE_FPM flag. + * While processing FPM queue, we will silently delete this + * MAC entry without sending any update for this MAC. */ - if (!delete) { - /* - * New op is "add". Previous op is "delete". - * Update the fpm_mac_info_t for the new add. - */ - fpm_mac->zebra_flags = rmac->flags; - - fpm_mac->vxlan_if = vxlan_if ? vxlan_if->ifindex : 0; - fpm_mac->svi_if = svi_if ? svi_if->ifindex : 0; - - UNSET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_DELETE_FPM); - SET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_UPDATE_FPM); - } else { - /* - * New op is "delete". Previous op is "add". - * Thus, no-op. Unset ZEBRA_MAC_UPDATE_FPM flag. - */ + if (!CHECK_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_DELETE_FPM) && + delete == 1) { SET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_DELETE_FPM); UNSET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_UPDATE_FPM); + return 0; } - - return 0; + } else { + fpm_mac = hash_get(zfpm_g->fpm_mac_info_table, &key, + zfpm_mac_info_alloc); + if (!fpm_mac) + return 0; } - fpm_mac = hash_get(zfpm_g->fpm_mac_info_table, &key, - zfpm_mac_info_alloc); - if (!fpm_mac) - return 0; - + fpm_mac->r_vtep_ip.s_addr = rmac->fwd_info.r_vtep_ip.s_addr; fpm_mac->zebra_flags = rmac->flags; fpm_mac->vxlan_if = vxlan_if ? vxlan_if->ifindex : 0; fpm_mac->svi_if = svi_if ? svi_if->ifindex : 0; @@ -1629,8 +1603,11 @@ static int zfpm_trigger_rmac_update(zebra_mac_t *rmac, zebra_l3vni_t *zl3vni, SET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_UPDATE_FPM); if (delete) SET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_DELETE_FPM); + else + UNSET_FLAG(fpm_mac->fpm_flags, ZEBRA_MAC_DELETE_FPM); - TAILQ_INSERT_TAIL(&zfpm_g->mac_q, fpm_mac, fpm_mac_q_entries); + if (!mac_found) + TAILQ_INSERT_TAIL(&zfpm_g->mac_q, fpm_mac, fpm_mac_q_entries); zfpm_g->stats.updates_triggered++;