bgpd : route agg. with ecomm attribute is consuming lot of cycles.

While configuring aggregate route prepare the hash table first,
then prepare the aggregated ecomm value and then do the
unique sort once for ecommunity.

Signed-off-by: vishaldhingra<vdhingra@vmware.com>

bgpd/bgp_ecommunity.c

@@ -1042,15 +1042,13 @@ static void *bgp_aggr_ecommunty_hash_alloc(void *p)
 
 static void bgp_aggr_ecommunity_prepare(struct hash_backet *hb, void *arg)
 {
-	struct ecommunity *ecommerge = NULL;
 	struct ecommunity *hb_ecommunity = hb->data;
 	struct ecommunity **aggr_ecommunity = arg;
 
-	if (*aggr_ecommunity) {
+	if (*aggr_ecommunity)
-		ecommerge = ecommunity_merge(*aggr_ecommunity, hb_ecommunity);
+		*aggr_ecommunity = ecommunity_merge(*aggr_ecommunity,
-		*aggr_ecommunity = ecommunity_uniq_sort(ecommerge);
+						    hb_ecommunity);
-		ecommunity_free(&ecommerge);
+	else
-	} else
 		*aggr_ecommunity = ecommunity_dup(hb_ecommunity);
 }
 
@@ -1063,6 +1061,14 @@ void bgp_aggr_ecommunity_remove(void *arg)
 
 void bgp_compute_aggregate_ecommunity(struct bgp_aggregate *aggregate,
 				      struct ecommunity *ecommunity)
+{
+	bgp_compute_aggregate_ecommunity_hash(aggregate, ecommunity);
+	bgp_compute_aggregate_ecommunity_val(aggregate);
+}
+
+
+void bgp_compute_aggregate_ecommunity_hash(struct bgp_aggregate *aggregate,
+					   struct ecommunity *ecommunity)
 {
 	struct ecommunity *aggr_ecommunity = NULL;
 
@@ -1083,20 +1089,34 @@ void bgp_compute_aggregate_ecommunity(struct bgp_aggregate *aggregate,
 		aggr_ecommunity = hash_get(aggregate->ecommunity_hash,
 					   ecommunity,
 					   bgp_aggr_ecommunty_hash_alloc);
+	}
+
+	/* Increment reference counter.
+	 */
+	aggr_ecommunity->refcnt++;
+}
+
+void bgp_compute_aggregate_ecommunity_val(struct bgp_aggregate *aggregate)
+{
+	struct ecommunity *ecommerge = NULL;
+
+	if (aggregate == NULL)
+		return;
 
 	/* Re-compute aggregate's ecommunity.
 	 */
 	if (aggregate->ecommunity)
 		ecommunity_free(&aggregate->ecommunity);
-
+	if (aggregate->ecommunity_hash
+	    && aggregate->ecommunity_hash->count) {
 		hash_iterate(aggregate->ecommunity_hash,
 			     bgp_aggr_ecommunity_prepare,
 			     &aggregate->ecommunity);
+		ecommerge = aggregate->ecommunity;
+		aggregate->ecommunity = ecommunity_uniq_sort(ecommerge);
+		if (ecommerge)
+			ecommunity_free(&ecommerge);
 	}
-
-	/* Increment refernce counter.
-	 */
-	aggr_ecommunity->refcnt++;
 }
 
 void bgp_remove_ecommunity_from_aggregate(struct bgp_aggregate *aggregate,
@@ -1105,10 +1125,9 @@ void bgp_remove_ecommunity_from_aggregate(struct bgp_aggregate *aggregate,
 	struct ecommunity *aggr_ecommunity = NULL;
 	struct ecommunity *ret_ecomm = NULL;
 
-	if ((aggregate == NULL) || (ecommunity == NULL))
+	if ((!aggregate)
-		return;
+	    || (!aggregate->ecommunity_hash)
-
+	    || (!ecommunity))
-	if (aggregate->ecommunity_hash == NULL)
 		return;
 
 	/* Look-up the ecommunity in the hash.
@@ -1121,14 +1140,33 @@ void bgp_remove_ecommunity_from_aggregate(struct bgp_aggregate *aggregate,
 			ret_ecomm = hash_release(aggregate->ecommunity_hash,
 						 aggr_ecommunity);
 			ecommunity_free(&ret_ecomm);
+			bgp_compute_aggregate_ecommunity_val(aggregate);
+		}
+	}
+}
 
-			ecommunity_free(&aggregate->ecommunity);
+void bgp_remove_ecomm_from_aggregate_hash(struct bgp_aggregate *aggregate,
+					  struct ecommunity *ecommunity)
+{
 
-			/* Compute aggregate's ecommunity.
+	struct ecommunity *aggr_ecommunity = NULL;
+	struct ecommunity *ret_ecomm = NULL;
+
+	if ((!aggregate)
+	    || (!aggregate->ecommunity_hash)
+	    || (!ecommunity))
+		return;
+
+	/* Look-up the ecommunity in the hash.
 	 */
-			hash_iterate(aggregate->ecommunity_hash,
+	aggr_ecommunity = bgp_aggr_ecommunity_lookup(aggregate, ecommunity);
-				     bgp_aggr_ecommunity_prepare,
+	if (aggr_ecommunity) {
-				     &aggregate->ecommunity);
+		aggr_ecommunity->refcnt--;
+
+		if (aggr_ecommunity->refcnt == 0) {
+			ret_ecomm = hash_release(aggregate->ecommunity_hash,
+						 aggr_ecommunity);
+			ecommunity_free(&ret_ecomm);
 		}
 	}
 }

bgpd/bgp_ecommunity.h

@@ -190,9 +190,18 @@ extern int ecommunity_fill_pbr_action(struct ecommunity_val *ecom_eval,
 extern void bgp_compute_aggregate_ecommunity(
 					struct bgp_aggregate *aggregate,
 					struct ecommunity *ecommunity);
+
+extern void bgp_compute_aggregate_ecommunity_hash(
+					struct bgp_aggregate *aggregate,
+					struct ecommunity *ecommunity);
+extern void bgp_compute_aggregate_ecommunity_val(
+					struct bgp_aggregate *aggregate);
 extern void bgp_remove_ecommunity_from_aggregate(
 					struct bgp_aggregate *aggregate,
 					struct ecommunity *ecommunity);
+extern void bgp_remove_ecomm_from_aggregate_hash(
+					struct bgp_aggregate *aggregate,
+					struct ecommunity *ecommunity);
 extern void bgp_aggr_ecommunity_remove(void *arg);
 
 #endif /* _QUAGGA_BGP_ECOMMUNITY_H */

bgpd/bgp_route.c

@@ -5940,7 +5940,7 @@ void bgp_aggregate_route(struct bgp *bgp, struct prefix *p,
 			/* Compute aggregate route's extended community.
 			 */
 			if (pi->attr->ecommunity)
-				bgp_compute_aggregate_ecommunity(
+				bgp_compute_aggregate_ecommunity_hash(
 							aggregate,
 							pi->attr->ecommunity);
 
@@ -5956,6 +5956,7 @@ void bgp_aggregate_route(struct bgp *bgp, struct prefix *p,
 	}
 	if (aggregate->as_set) {
 		bgp_compute_aggregate_community_val(aggregate);
+		bgp_compute_aggregate_ecommunity_val(aggregate);
 		bgp_compute_aggregate_lcommunity_val(aggregate);
 	}
 
@@ -6054,7 +6055,7 @@ void bgp_aggregate_delete(struct bgp *bgp, struct prefix *p, afi_t afi,
 				if (pi->attr->ecommunity)
 					/* Remove ecommunity from aggregate.
 					 */
-					bgp_remove_ecommunity_from_aggregate(
+					bgp_remove_ecomm_from_aggregate_hash(
 							aggregate,
 							pi->attr->ecommunity);
 
@@ -6075,6 +6076,8 @@ void bgp_aggregate_delete(struct bgp *bgp, struct prefix *p, afi_t afi,
 	if (aggregate->as_set) {
 		if (aggregate->community)
 			community_free(&aggregate->community);
+		if (aggregate->ecommunity)
+			ecommunity_free(&aggregate->ecommunity);
 		if (aggregate->lcommunity)
 			lcommunity_free(&aggregate->lcommunity);
 	}