FIB memory leaks (VPP-578)v17.01-rc2

1) vec_free the fe_srcs of a fib_entry_t when the fib_entry_t is itself reed
2) in the load-balance fixup if a drop path is required add this to a new vector of next-hops 'fixed_nhs'. This vector is managed by the load-balance function. The caller continues to manage its own set. The function is now const implying that the caller is safe to assume the next-hops do not change.

Change-Id: I0f29203ee16b9a270f40edf237488fa99ba65320
Signed-off-by: Neale Ranns <nranns@cisco.com>

6 files changed, 48 insertions, 24 deletions

diff --git a/vnet/vnet/dpo/load_balance.c b/vnet/vnet/dpo/load_balance.c
index a244776ffb8..e70a7a306e1 100644
--- a/vnet/vnet/dpo/load_balance.c
+++ b/vnet/vnet/dpo/load_balance.c
@@ -279,8 +279,8 @@ load_balance_get_bucket (index_t lbi,
 }
 
 static int
-next_hop_sort_by_weight (load_balance_path_t * n1,
-                         load_balance_path_t * n2)
+next_hop_sort_by_weight (const load_balance_path_t * n1,
+                         const load_balance_path_t * n2)
 {
     return ((int) n1->path_weight - (int) n2->path_weight);
 }
@@ -289,7 +289,7 @@ next_hop_sort_by_weight (load_balance_path_t * n1,
    with weights corresponding to the number of adjacencies for each next hop.
    Returns number of adjacencies in block. */
 u32
-ip_multipath_normalize_next_hops (load_balance_path_t * raw_next_hops,
+ip_multipath_normalize_next_hops (const load_balance_path_t * raw_next_hops,
                                   load_balance_path_t ** normalized_next_hops,
                                   u32 *sum_weight_in,
                                   f64 multipath_next_hop_error_tolerance)
@@ -409,23 +409,25 @@ done:
 }
 
 static load_balance_path_t *
-load_balance_multipath_next_hop_fixup (load_balance_path_t *nhs,
+load_balance_multipath_next_hop_fixup (const load_balance_path_t *nhs,
                                        dpo_proto_t drop_proto)
 {
     if (0 == vec_len(nhs))
     {
-        load_balance_path_t *nh;
+        load_balance_path_t *new_nhs = NULL, *nh;
 
         /*
          * we need something for the load-balance. so use the drop
          */
-        vec_add2(nhs, nh, 1);
+        vec_add2(new_nhs, nh, 1);
 
         nh->path_weight = 1;
         dpo_copy(&nh->path_dpo, drop_dpo_get(drop_proto));
+
+        return (new_nhs);
     }
 
-    return (nhs);
+    return (NULL);
 }
 
 /*
@@ -467,11 +469,11 @@ load_balance_set_n_buckets (load_balance_t *lb,
 
 void
 load_balance_multipath_update (const dpo_id_t *dpo,
-                               load_balance_path_t * raw_next_hops,
+                               const load_balance_path_t * raw_nhs,
                                load_balance_flags_t flags)
 {
-    u32 sum_of_weights,n_buckets, ii;
-    load_balance_path_t * nh, * nhs;
+    load_balance_path_t *nh, *nhs, *fixed_nhs;
+    u32 sum_of_weights, n_buckets, ii;
     index_t lbmi, old_lbmi;
     load_balance_t *lb;
     dpo_id_t *tmp_dpo;
@@ -480,16 +482,16 @@ load_balance_multipath_update (const dpo_id_t *dpo,
 
     ASSERT(DPO_LOAD_BALANCE == dpo->dpoi_type);
     lb = load_balance_get(dpo->dpoi_index);
-    raw_next_hops =
-        load_balance_multipath_next_hop_fixup(raw_next_hops,
-                                              lb->lb_proto);
+    fixed_nhs = load_balance_multipath_next_hop_fixup(raw_nhs, lb->lb_proto);
     n_buckets =
-        ip_multipath_normalize_next_hops(raw_next_hops,
+        ip_multipath_normalize_next_hops((NULL == fixed_nhs ?
+                                          raw_nhs :
+                                          fixed_nhs),
                                          &nhs,
                                          &sum_of_weights,
                                          multipath_next_hop_error_tolerance);
 
-    ASSERT (n_buckets >= vec_len (raw_next_hops));
+    ASSERT (n_buckets >= vec_len (raw_nhs));
 
     /*
      * Save the old load-balance map used, and get a new one if required.
@@ -694,6 +696,7 @@ load_balance_multipath_update (const dpo_id_t *dpo,
         dpo_reset(&nh->path_dpo);
     }
     vec_free(nhs);
+    vec_free(fixed_nhs);
 
     load_balance_map_unlock(old_lbmi);
 }
diff --git a/vnet/vnet/dpo/load_balance.h b/vnet/vnet/dpo/load_balance.h
index dc6485e688a..1799653628d 100644
--- a/vnet/vnet/dpo/load_balance.h
+++ b/vnet/vnet/dpo/load_balance.h
@@ -159,7 +159,7 @@ extern index_t load_balance_create(u32 num_buckets,
 				   flow_hash_config_t fhc);
 extern void load_balance_multipath_update(
     const dpo_id_t *dpo,
-    load_balance_path_t * raw_next_hops,
+    const load_balance_path_t * raw_next_hops,
     load_balance_flags_t flags);
 
 extern void load_balance_set_bucket(index_t lbi,
diff --git a/vnet/vnet/fib/fib_entry.c b/vnet/vnet/fib/fib_entry.c
index 24b506379ac..3aa3632c596 100644
--- a/vnet/vnet/fib/fib_entry.c
+++ b/vnet/vnet/fib/fib_entry.c
@@ -230,6 +230,7 @@ fib_entry_last_lock_gone (fib_node_t *node)
 
     ASSERT(0 == vec_len(fib_entry->fe_delegates));
     vec_free(fib_entry->fe_delegates);
+    vec_free(fib_entry->fe_srcs);
     pool_put(fib_entry_pool, fib_entry);
 }
 
diff --git a/vnet/vnet/fib/fib_entry_src.c b/vnet/vnet/fib/fib_entry_src.c
index 060fac941d2..1fb040608b4 100644
--- a/vnet/vnet/fib/fib_entry_src.c
+++ b/vnet/vnet/fib/fib_entry_src.c
@@ -382,6 +382,14 @@ fib_entry_src_mk_lb (fib_entry_t *fib_entry,
         .fct = fct,
     };
 
+    /*
+     * As an optimisation we allocate the vector of next-hops to be sized
+     * equal to the maximum nuber of paths we will need, which is also the
+     * most likely number we will need, since in most cases the paths are 'up'.
+     */
+    vec_validate(ctx.next_hops, fib_path_list_get_n_paths(esrc->fes_pl));
+    vec_reset_length(ctx.next_hops);
+
     lb_proto = fib_proto_to_dpo(fib_entry->fe_prefix.fp_proto);
 
     fib_path_list_walk(esrc->fes_pl,
diff --git a/vnet/vnet/fib/fib_path_list.c b/vnet/vnet/fib/fib_path_list.c
index 5b35e9b87e7..db9d1af9e3f 100644
--- a/vnet/vnet/fib/fib_path_list.c
+++ b/vnet/vnet/fib/fib_path_list.c
@@ -365,10 +365,10 @@ fib_path_list_mk_lb (fib_path_list_t *path_list,
 		     fib_forward_chain_type_t fct,
 		     dpo_id_t *dpo)
 {
-    load_balance_path_t *hash_key;
+    load_balance_path_t *nhs;
     fib_node_index_t *path_index;
 
-    hash_key  = NULL;
+    nhs  = NULL;
 
     if (!dpo_id_is_valid(dpo))
     {
@@ -388,21 +388,20 @@ fib_path_list_mk_lb (fib_path_list_t *path_list,
      */
     vec_foreach (path_index, path_list->fpl_paths)
     {
-	hash_key = fib_path_append_nh_for_multipath_hash(
-	               *path_index,
-		       fct,
-		       hash_key);
+	nhs = fib_path_append_nh_for_multipath_hash(*path_index,
+                                                    fct,
+                                                    nhs);
     }
 
     /*
      * Path-list load-balances, which if used, would be shared and hence
      * never need a load-balance map.
      */
-    load_balance_multipath_update(dpo, hash_key, LOAD_BALANCE_FLAG_NONE);
+    load_balance_multipath_update(dpo, nhs, LOAD_BALANCE_FLAG_NONE);
 
     FIB_PATH_LIST_DBG(path_list, "mk lb: %d", dpo->dpoi_index);
 
-    vec_free(hash_key);
+    vec_free(nhs);
 }
 
 /**
@@ -592,6 +591,17 @@ fib_path_list_resolve (fib_path_list_t *path_list)
 }
 
 u32
+fib_path_list_get_n_paths (fib_node_index_t path_list_index)
+{
+    fib_path_list_t *path_list;
+
+    path_list = fib_path_list_get(path_list_index);
+
+    return (vec_len(path_list->fpl_paths));
+}
+
+
+u32
 fib_path_list_get_resolving_interface (fib_node_index_t path_list_index)
 {
     fib_node_index_t *path_index;
diff --git a/vnet/vnet/fib/fib_path_list.h b/vnet/vnet/fib/fib_path_list.h
index 8bc1b20b6bf..f4f94a1b04a 100644
--- a/vnet/vnet/fib/fib_path_list.h
+++ b/vnet/vnet/fib/fib_path_list.h
@@ -104,6 +104,8 @@ extern fib_node_index_t fib_path_list_copy_and_path_remove(
     fib_node_index_t pl_index,
     fib_path_list_flags_t flags,
     const fib_route_path_t *path);
+extern u32 fib_path_list_get_n_paths(fib_node_index_t pl_index);
+
 extern void fib_path_list_contribute_forwarding(fib_node_index_t path_list_index,
 						fib_forward_chain_type_t type,
 						dpo_id_t *dpo);