From 76ce54be1f192ce964215d3d4f17e709b607e0e0 Mon Sep 17 00:00:00 2001 From: Divakar Date: Thu, 18 Aug 2016 10:41:56 +0530 Subject: [PATCH] Update prefix len in Mtrie bucket as well Right now, if mtrie already contains more specific entries and if a less specific entry is added, only non-matching leaves are updated with less specific entry's prefix len. The matching buckets also need to be updated with prefix len. Not updating the prefix len in buckets, results in, not collapsing the tree, if these less specific entries are deleted. For example, in an empty tree, 8.1.1.10/32 -> nh1 is added. This results in, at level 1 bucket 8, level 2 bucket 1, level 3 bucket 1 to contain prefix len as 0 and level 4 bucket 10 to contain prefix len as 32. The other leaves in level 4 (like 8.1.1.1 till 8.1.1.255 except 10) also contain prefix len as 0. After this 8.1.1.0/24 -> nh2 is added. Currently the prefix len of 24 is updated only in the leaves like, 8.1.1.1 till 8.1.1.255. Ideally the prefix len 24 should be updated in level 3 bucket 1 as well. Not updating this, leads to issues while collapsing level 3 buckets. Fixes: 1) Prefix len is updated in the matching buckets as well 2) In the label flags, if LABEL_VALID flag not set, the label stored does not have any meaning. But while collapsing the bucket the labels are also compared leading to non-collapse. The labels need to be compared only if the LABEL_VALID flag is set. To over come this, if the LABEL_VALID is not set, the label is stored as -1, so that they are comparable even if flag is not set 3) In 'rt' utility, the replacment len incase of delete is wrongly initialised to 100. This is rectified. Change-Id: I05968c2663d31d43c67eb2de5fd6bfc720459dec closes-bug: #1605748 --- dp-core/vr_ip_mtrie.c | 101 +++++++++++++++++++----------------------- utils/rt.c | 9 ++-- 2 files changed, 52 insertions(+), 58 deletions(-) diff --git a/dp-core/vr_ip_mtrie.c b/dp-core/vr_ip_mtrie.c index 2c7db6854..18902affa 100644 --- a/dp-core/vr_ip_mtrie.c +++ b/dp-core/vr_ip_mtrie.c @@ -316,28 +316,34 @@ add_to_tree(struct ip_bucket_entry *ent, int level, struct vr_route_req *rt) struct ip_bucket *bkt; struct mtrie_bkt_info *ip_bkt_info; + if (ent->entry_prefix_len > rt->rtr_req.rtr_prefix_len) + return; + + + ent->entry_prefix_len = rt->rtr_req.rtr_prefix_len; + + if (ENTRY_IS_NEXTHOP(ent)) { + /* a less specific entry, which needs to be replaced */ + set_entry_to_nh(ent, rt->rtr_nh); + ent->entry_label_flags = rt->rtr_req.rtr_label_flags; + ent->entry_label = rt->rtr_req.rtr_label; + ent->entry_bridge_index = rt->rtr_req.rtr_index; + + return; + } + if (level >= (ip_bkt_get_max_level(rt->rtr_req.rtr_family) - 1)) - /* assert here ? */ return; ip_bkt_info = ip_bkt_info_get(rt->rtr_req.rtr_family); - /* assured that the first one is a bucket */ + /* Assured that this is valid bucket now */ bkt = entry_to_bucket(ent); level++; for (i = 0; i < ip_bkt_info[level].bi_size; i++) { ent = index_to_entry(bkt, i); - if (!ENTRY_IS_NEXTHOP(ent)) - add_to_tree(ent, level, rt); - else if (ent->entry_prefix_len <= rt->rtr_req.rtr_prefix_len) { - /* a less specific entry, which needs to be replaced */ - set_entry_to_nh(ent, rt->rtr_nh); - ent->entry_prefix_len = rt->rtr_req.rtr_prefix_len; - ent->entry_label_flags = rt->rtr_req.rtr_label_flags; - ent->entry_label = rt->rtr_req.rtr_label; - ent->entry_bridge_index = rt->rtr_req.rtr_index; - } + add_to_tree(ent, level, rt); } return; @@ -384,11 +390,11 @@ mtrie_reset_entry(struct ip_bucket_entry *ent, int level, static int __mtrie_add(struct ip_mtrie *mtrie, struct vr_route_req *rt) { - int ret, index = 0, level, err_level = 0; - unsigned char i, fin = 0; - struct ip_bucket *bkt; - struct ip_bucket_entry *ent, *err_ent = NULL; - struct vr_nexthop *nh, *err_nh = NULL; + int ret, index = 0, level, err_level = 0, fin; + unsigned char i; + struct ip_bucket *bkt; + struct ip_bucket_entry *ent, *err_ent = NULL; + struct vr_nexthop *nh, *err_nh = NULL; struct mtrie_bkt_info *ip_bkt_info = ip_bkt_info_get(rt->rtr_req.rtr_family); ent = &mtrie->root; @@ -426,42 +432,20 @@ __mtrie_add(struct ip_mtrie *mtrie, struct vr_route_req *rt) * cover all the indices for which this route is the best * prefix match */ + + fin = ip_bkt_info[level].bi_size; + if ((rt->rtr_req.rtr_prefix_len > (ip_bkt_info[level].bi_pfx_len - ip_bkt_info[level].bi_bits)) && (rt->rtr_req.rtr_prefix_len <= ip_bkt_info[level].bi_pfx_len)) { fin = 1 << (ip_bkt_info[level].bi_pfx_len - rt->rtr_req.rtr_prefix_len); } - - /* - * Run through the loop 'fin' times only - * If fin is 0, it actually means 256 ('char' overflow), so run the - * loop 256 times - */ - for (i = index; i <= (ip_bkt_info[level].bi_size-1); i++) { + i = index; + for (; ((i <= (ip_bkt_info[level].bi_size-1)) && fin); + i++, fin--) { ent = index_to_entry(bkt, i); - if (ENTRY_IS_BUCKET(ent)) - add_to_tree(ent, level, rt); - else if (ent->entry_prefix_len <= rt->rtr_req.rtr_prefix_len) { - /* a less specific entry, which needs to be replaced */ - set_entry_to_nh(ent, rt->rtr_nh); - ent->entry_prefix_len = rt->rtr_req.rtr_prefix_len; - ent->entry_label_flags = rt->rtr_req.rtr_label_flags; - ent->entry_label = rt->rtr_req.rtr_label; - ent->entry_bridge_index = rt->rtr_req.rtr_index; - } - if (fin) { - /* Repeat the loop 'fin' times only */ - fin--; - if (fin == 0) - break; - } - /* - * Bailout at the last index, - * the below check takes care of overflow - */ - if (i == (ip_bkt_info[level].bi_size-1)) - break; + add_to_tree(ent, level, rt); } break; @@ -512,6 +496,7 @@ __mtrie_delete(struct vr_route_req *rt, struct ip_bucket_entry *ent, for (i = index; i < fin; i++) { tmp_ent = index_to_entry(bkt, i); + if (tmp_ent->entry_prefix_len == rt->rtr_req.rtr_prefix_len) { tmp_ent->entry_label_flags = rt->rtr_req.rtr_label_flags; tmp_ent->entry_label = rt->rtr_req.rtr_label; @@ -529,15 +514,8 @@ __mtrie_delete(struct vr_route_req *rt, struct ip_bucket_entry *ent, /* check if current bucket neds to be deleted */ for (i = 1; i < ip_bkt_info[level].bi_size; i++) { - if ((bkt->bkt_data[i].entry_long_i == bkt->bkt_data[0].entry_long_i) && - (bkt->bkt_data[i].entry_label_flags == - bkt->bkt_data[0].entry_label_flags) && - (bkt->bkt_data[i].entry_label == - bkt->bkt_data[0].entry_label) && - (bkt->bkt_data[i].entry_prefix_len == - bkt->bkt_data[0].entry_prefix_len)) { - continue; - } else + if (memcmp(bkt->bkt_data + i, bkt->bkt_data, + sizeof(struct ip_bucket_entry))) return 0; } @@ -754,6 +732,12 @@ mtrie_delete(struct vr_rtable * _unused, struct vr_route_req *rt) rt->rtr_req.rtr_index = lreq.rtr_req.rtr_index; } + if (!(rt->rtr_req.rtr_label_flags & VR_RT_LABEL_VALID_FLAG)) { + rt->rtr_req.rtr_label = 0xFFFFF; + } else { + rt->rtr_req.rtr_label &= 0xFFFFF; + } + __mtrie_delete(rt, &rtable->root, 0); vrouter_put_nexthop(rt->rtr_nh); @@ -1003,6 +987,12 @@ mtrie_add(struct vr_rtable * _unused, struct vr_route_req *rt) rt->rtr_req.rtr_index = tmp_req.rtr_req.rtr_index; } + if (!(rt->rtr_req.rtr_label_flags & VR_RT_LABEL_VALID_FLAG)) { + rt->rtr_req.rtr_label = 0xFFFFF; + } else { + rt->rtr_req.rtr_label &= 0xFFFFF; + } + ret = __mtrie_add(mtrie, rt); vrouter_put_nexthop(rt->rtr_nh); return ret; @@ -1041,6 +1031,7 @@ mtrie_alloc_vrf(unsigned int vrf_id, unsigned int family) mtrie->root.entry_bridge_index = VR_BE_INVALID_INDEX; mtrie_table = vn_rtable[index]; mtrie_table[vrf_id] = mtrie; + mtrie->root.entry_label = 0xFFFFF; } return mtrie; diff --git a/utils/rt.c b/utils/rt.c index b29992de6..27b445df7 100644 --- a/utils/rt.c +++ b/utils/rt.c @@ -63,7 +63,7 @@ static int cmd_nh_id = -1; static uint8_t cmd_prefix[16], cmd_src[16]; static uint32_t cmd_plen = 0; static int32_t cmd_label; -static uint32_t cmd_replace_plen = 100; +static uint32_t cmd_replace_plen = 0xFFFFFFFF; static char cmd_dst_mac[6]; static bool response_pending = true; static void Usage(void); @@ -520,8 +520,11 @@ validate_options(void) case SANDESH_OP_DELETE: if ((cmd_family_id == AF_INET) || (cmd_family_id == AF_INET6)) { - if ((cmd_replace_plen < 0 || - ( cmd_replace_plen > (RT_IP_ADDR_SIZE(cmd_family_id)*4)))) { + + if (cmd_replace_plen == 0xFFFFFFFF) + goto usage_internal; + + if (cmd_replace_plen > (RT_IP_ADDR_SIZE(cmd_family_id) * 8)) { goto usage_internal; }