From 820b3b6596b3aa973378dda64a838e338279ee06 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 30 Apr 2019 17:56:05 -0400 Subject: [PATCH 1/6] doc: Some minor doc cleanup for new data structures Noticed during attempts at usage that the documentation needed a couple small updates: 1) Tell the user which header to include 2) Some functions want the address of the data structure Signed-off-by: Donald Sharp --- doc/developer/lists.rst | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/doc/developer/lists.rst b/doc/developer/lists.rst index 6d60420b2f..987b3b7f4f 100644 --- a/doc/developer/lists.rst +++ b/doc/developer/lists.rst @@ -119,6 +119,8 @@ The common setup pattern will look like this: .. code-block:: c + #include + PREDECL_XXX(Z) struct item { int otherdata; @@ -159,26 +161,26 @@ Common iteration macros The following iteration macros work across all data structures: -.. c:function:: for_each(Z, head, item) +.. c:function:: for_each(Z, &head, item) Equivalent to: .. code-block:: c - for (item = Z_first(head); item; item = Z_next(head, item)) + for (item = Z_first(&head); item; item = Z_next(&head, item)) Note that this will fail if the list is modified while being iterated over. -.. c:function:: for_each_safe(Z, head, item) +.. c:function:: for_each_safe(Z, &head, item) Same as the previous, but the next element is pre-loaded into a "hidden" variable (named ``Z_safe``.) Equivalent to: .. code-block:: c - for (item = Z_first(head); item; item = next) { - next = Z_next_safe(head, item); + for (item = Z_first(&head); item; item = next) { + next = Z_next_safe(&head, item); ... } @@ -189,7 +191,7 @@ The following iteration macros work across all data structures: tables is resized while iterating. This will cause items to be skipped or iterated over twice. -.. c:function:: for_each_from(Z, head, item, from) +.. c:function:: for_each_from(Z, &head, item, from) Iterates over the list, starting at item ``from``. This variant is "safe" as in the previous macro. Equivalent to: @@ -197,7 +199,7 @@ The following iteration macros work across all data structures: .. code-block:: c for (item = from; item; item = from) { - from = Z_next_safe(head, item); + from = Z_next_safe(&head, item); ... } From 0a45d974729e1495626783212b1bbaa829eb6b1e Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 30 Apr 2019 18:04:57 -0400 Subject: [PATCH 2/6] zebra: Remove linked list and replace with new LIST The `struct rib_dest_t` was being used to store the linked list of rnh's associated with the node. This was taking up a bunch of memory. Replace with new data structure supplied by David and see the memory reductions associated with 1 million routes in the zebra rib: Old: Memory statistics for zebra: System allocator statistics: Total heap allocated: 675 MiB Holding block headers: 0 bytes Used small blocks: 0 bytes Used ordinary blocks: 567 MiB Free small blocks: 39 MiB Free ordinary blocks: 69 MiB Ordinary blocks: 0 Small blocks: 0 Holding blocks: 0 New: Memory statistics for zebra: System allocator statistics: Total heap allocated: 574 MiB Holding block headers: 0 bytes Used small blocks: 0 bytes Used ordinary blocks: 536 MiB Free small blocks: 33 MiB Free ordinary blocks: 4600 KiB Ordinary blocks: 0 Small blocks: 0 Holding blocks: 0 `struct rnh` was moved to rib.h because of the tangled web of structure dependancies. This data structure is used in numerous places so it should be ok for the moment. Future work might be needed to do a better job of splitting up data structures and function definitions. Signed-off-by: Donald Sharp --- zebra/rib.h | 43 ++++++++++++++++++++++++++++++++++++++++++- zebra/zebra_rib.c | 7 +++---- zebra/zebra_rnh.c | 6 +++--- zebra/zebra_rnh.h | 34 ---------------------------------- zebra/zebra_vrf.c | 2 +- 5 files changed, 49 insertions(+), 43 deletions(-) diff --git a/zebra/rib.h b/zebra/rib.h index e26831e1a6..fd2aeabf57 100644 --- a/zebra/rib.h +++ b/zebra/rib.h @@ -24,6 +24,7 @@ #include "zebra.h" #include "hook.h" +#include "typesafe.h" #include "linklist.h" #include "prefix.h" #include "table.h" @@ -39,6 +40,44 @@ extern "C" { #endif +typedef enum { RNH_NEXTHOP_TYPE, RNH_IMPORT_CHECK_TYPE } rnh_type_t; + +PREDECL_LIST(rnh_list) + +/* Nexthop structure. */ +struct rnh { + uint8_t flags; + +#define ZEBRA_NHT_CONNECTED 0x1 +#define ZEBRA_NHT_DELETED 0x2 +#define ZEBRA_NHT_EXACT_MATCH 0x4 + + /* VRF identifier. */ + vrf_id_t vrf_id; + + afi_t afi; + + rnh_type_t type; + + uint32_t seqno; + + struct route_entry *state; + struct prefix resolved_route; + struct list *client_list; + + /* pseudowires dependent on this nh */ + struct list *zebra_pseudowire_list; + + struct route_node *node; + + /* + * if this has been filtered for the client + */ + int filtered[ZEBRA_ROUTE_MAX]; + + struct rnh_list_item rnh_list_item; +}; + #define DISTANCE_INFINITY 255 #define ZEBRA_KERNEL_TABLE_MAX 252 /* support for no more than this rt tables */ @@ -151,7 +190,7 @@ typedef struct rib_dest_t_ { * the data plane we will run evaluate_rnh * on these prefixes. */ - struct list *nht; + struct rnh_list_head nht; /* * Linkage to put dest on the FPM processing queue. @@ -160,6 +199,8 @@ typedef struct rib_dest_t_ { } rib_dest_t; +DECLARE_LIST(rnh_list, struct rnh, rnh_list_item); + #define RIB_ROUTE_QUEUED(x) (1 << (x)) // If MQ_SIZE is modified this value needs to be updated. #define RIB_ROUTE_ANY_QUEUED 0x1F diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c index 2994911165..f5ba619afa 100644 --- a/zebra/zebra_rib.c +++ b/zebra/zebra_rib.c @@ -1204,7 +1204,6 @@ static int rib_can_delete_dest(rib_dest_t *dest) void zebra_rib_evaluate_rn_nexthops(struct route_node *rn, uint32_t seq) { rib_dest_t *dest = rib_dest_from_rnode(rn); - struct listnode *node, *nnode; struct rnh *rnh; /* @@ -1236,7 +1235,7 @@ void zebra_rib_evaluate_rn_nexthops(struct route_node *rn, uint32_t seq) * nht resolution and as such we need to call the * nexthop tracking evaluation code */ - for (ALL_LIST_ELEMENTS(dest->nht, node, nnode, rnh)) { + for_each (rnh_list, &dest->nht, rnh) { struct zebra_vrf *zvrf = zebra_vrf_lookup_by_id(rnh->vrf_id); struct prefix *p = &rnh->node->p; @@ -1312,7 +1311,7 @@ int rib_gc_dest(struct route_node *rn) zebra_rib_evaluate_rn_nexthops(rn, zebra_router_get_next_sequence()); dest->rnode = NULL; - list_delete(&dest->nht); + rnh_list_fini(&dest->nht); XFREE(MTYPE_RIB_DEST, dest); rn->info = NULL; @@ -2357,7 +2356,7 @@ rib_dest_t *zebra_rib_create_dest(struct route_node *rn) rib_dest_t *dest; dest = XCALLOC(MTYPE_RIB_DEST, sizeof(rib_dest_t)); - dest->nht = list_new(); + rnh_list_init(&dest->nht); route_lock_node(rn); /* rn route table reference */ rn->info = dest; dest->rnode = rn; diff --git a/zebra/zebra_rnh.c b/zebra/zebra_rnh.c index 220a8006d0..2917d0e7a8 100644 --- a/zebra/zebra_rnh.c +++ b/zebra/zebra_rnh.c @@ -119,7 +119,7 @@ static void zebra_rnh_remove_from_routing_table(struct rnh *rnh) } dest = rib_dest_from_rnode(rn); - listnode_delete(dest->nht, rnh); + rnh_list_del(&dest->nht, rnh); route_unlock_node(rn); } @@ -145,7 +145,7 @@ static void zebra_rnh_store_in_routing_table(struct rnh *rnh) } dest = rib_dest_from_rnode(rn); - listnode_add(dest->nht, rnh); + rnh_list_add_tail(&dest->nht, rnh); route_unlock_node(rn); } @@ -251,7 +251,7 @@ void zebra_free_rnh(struct rnh *rnh) route_unlock_node(rern); dest = rib_dest_from_rnode(rern); - listnode_delete(dest->nht, rnh); + rnh_list_del(&dest->nht, rnh); } } free_state(rnh->vrf_id, rnh->state, rnh->node); diff --git a/zebra/zebra_rnh.h b/zebra/zebra_rnh.h index 9cd9116eed..95a3941181 100644 --- a/zebra/zebra_rnh.h +++ b/zebra/zebra_rnh.h @@ -29,40 +29,6 @@ extern "C" { #endif -typedef enum { RNH_NEXTHOP_TYPE, RNH_IMPORT_CHECK_TYPE } rnh_type_t; - -/* Nexthop structure. */ -struct rnh { - uint8_t flags; - -#define ZEBRA_NHT_CONNECTED 0x1 -#define ZEBRA_NHT_DELETED 0x2 -#define ZEBRA_NHT_EXACT_MATCH 0x4 - - /* VRF identifier. */ - vrf_id_t vrf_id; - - afi_t afi; - - rnh_type_t type; - - uint32_t seqno; - - struct route_entry *state; - struct prefix resolved_route; - struct list *client_list; - - /* pseudowires dependent on this nh */ - struct list *zebra_pseudowire_list; - - struct route_node *node; - - /* - * if this has been filtered for the client - */ - int filtered[ZEBRA_ROUTE_MAX]; -}; - extern int zebra_rnh_ip_default_route; extern int zebra_rnh_ipv6_default_route; diff --git a/zebra/zebra_vrf.c b/zebra/zebra_vrf.c index 2d721ec8a1..f0b99f41fc 100644 --- a/zebra/zebra_vrf.c +++ b/zebra/zebra_vrf.c @@ -357,7 +357,7 @@ void zebra_rtable_node_cleanup(struct route_table *table, if (node->info) { rib_dest_t *dest = node->info; - list_delete(&dest->nht); + rnh_list_fini(&dest->nht); XFREE(MTYPE_RIB_DEST, node->info); } } From a07f484614e0e9cc92afaf4bf58e6f6507fdbed4 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 30 Apr 2019 19:38:15 -0400 Subject: [PATCH 3/6] lib: Make _find functions treat the head as const The head of a list should not change for find functions. Probably are others that should be considered but these changes can come in as needed I believe. Signed-off-by: Donald Sharp --- lib/typesafe.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/typesafe.h b/lib/typesafe.h index bbf3ce8f1c..94002da599 100644 --- a/lib/typesafe.h +++ b/lib/typesafe.h @@ -275,7 +275,7 @@ macro_pure size_t prefix ## _count(struct prefix##_head *h) \ #define DECLARE_SORTLIST_UNIQ(prefix, type, field, cmpfn) \ _DECLARE_SORTLIST(prefix, type, field, cmpfn, cmpfn) \ \ -macro_inline type *prefix ## _find(struct prefix##_head *h, const type *item) \ +macro_inline type *prefix ## _find(const struct prefix##_head *h, const type *item) \ { \ struct ssort_item *sitem = h->sh.first; \ int cmpval = 0; \ @@ -383,7 +383,7 @@ macro_inline type *prefix ## _add(struct prefix##_head *h, type *item) \ *np = &item->field.hi; \ return NULL; \ } \ -macro_inline type *prefix ## _find(struct prefix##_head *h, const type *item) \ +macro_inline type *prefix ## _find(const struct prefix##_head *h, const type *item) \ { \ if (!h->hh.tabshift) \ return NULL; \ @@ -576,7 +576,7 @@ macro_inline int prefix ## __cmp(const struct sskip_item *a, \ return cmpfn(container_of(a, type, field.si), \ container_of(b, type, field.si)); \ } \ -macro_inline type *prefix ## _find(struct prefix##_head *h, const type *item) \ +macro_inline type *prefix ## _find(const struct prefix##_head *h, const type *item) \ { \ struct sskip_item *sitem = typesafe_skiplist_find(&h->sh, \ &item->field.si, &prefix ## __cmp); \ From 6701b1b7e749af853f9b9363ef507189d10ef6d2 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 30 Apr 2019 19:40:11 -0400 Subject: [PATCH 4/6] lib: Make prefix_hash_key accept a const We should not be modifying the pointer for the prefix_hash_key function, make it a const so that we can use it elsewhere. Signed-off-by: Donald Sharp --- lib/prefix.c | 2 +- lib/prefix.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/prefix.c b/lib/prefix.c index 6b91969218..d2a4c3a432 100644 --- a/lib/prefix.c +++ b/lib/prefix.c @@ -1543,7 +1543,7 @@ char *prefix_mac2str(const struct ethaddr *mac, char *buf, int size) return ptr; } -unsigned prefix_hash_key(void *pp) +unsigned prefix_hash_key(const void *pp) { struct prefix copy; diff --git a/lib/prefix.h b/lib/prefix.h index d3c387e102..d57b43dac6 100644 --- a/lib/prefix.h +++ b/lib/prefix.h @@ -466,7 +466,7 @@ extern int is_zero_mac(struct ethaddr *mac); extern int prefix_str2mac(const char *str, struct ethaddr *mac); extern char *prefix_mac2str(const struct ethaddr *mac, char *buf, int size); -extern unsigned prefix_hash_key(void *pp); +extern unsigned prefix_hash_key(const void *pp); extern int str_to_esi(const char *str, esi_t *esi); extern char *esi_to_str(const esi_t *esi, char *buf, int size); From 0103534f9c48fb2c00a3833a5acb7405af12b55a Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 30 Apr 2019 20:23:52 -0400 Subject: [PATCH 5/6] lib: Convert table code to use new hash type This converts the new table code to use the new hash type provided by David. The following test is 1 million routes installed and how much memory we are using: Old mem usage: Memory statistics for zebra: System allocator statistics: Total heap allocated: 574 MiB Holding block headers: 0 bytes Used small blocks: 0 bytes Used ordinary blocks: 536 MiB Free small blocks: 33 MiB Free ordinary blocks: 4600 KiB Ordinary blocks: 0 Small blocks: 0 Holding blocks: 0 New Memory usage: Memory statistics for zebra: System allocator statistics: Total heap allocated: 542 MiB Holding block headers: 0 bytes Used small blocks: 0 bytes Used ordinary blocks: 506 MiB Free small blocks: 3374 KiB Free ordinary blocks: 33 MiB Ordinary blocks: 0 Small blocks: 0 Holding blocks: 0 Signed-off-by: Donald Sharp --- lib/table.c | 31 ++++++++++++++----------------- lib/table.h | 6 +++++- 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/table.c b/lib/table.c index edba7f1932..2d42e2d55c 100644 --- a/lib/table.c +++ b/lib/table.c @@ -33,12 +33,14 @@ DEFINE_MTYPE(LIB, ROUTE_NODE, "Route node") static void route_table_free(struct route_table *); -static bool route_table_hash_cmp(const void *a, const void *b) +static int route_table_hash_cmp(const void *a, const void *b) { const struct prefix *pa = a, *pb = b; - return prefix_cmp(pa, pb) == 0; + return prefix_cmp(pa, pb); } +DECLARE_HASH(rn_hash_node, struct route_node, nodehash, route_table_hash_cmp, + prefix_hash_key) /* * route_table_init_with_delegate */ @@ -49,8 +51,7 @@ route_table_init_with_delegate(route_table_delegate_t *delegate) rt = XCALLOC(MTYPE_ROUTE_TABLE, sizeof(struct route_table)); rt->delegate = delegate; - rt->hash = hash_create(prefix_hash_key, route_table_hash_cmp, - "route table hash"); + rn_hash_node_init(&rt->hash); return rt; } @@ -69,15 +70,14 @@ static struct route_node *route_node_new(struct route_table *table) static struct route_node *route_node_set(struct route_table *table, const struct prefix *prefix) { - struct route_node *node, *inserted; + struct route_node *node; node = route_node_new(table); prefix_copy(&node->p, prefix); node->table = table; - inserted = hash_get(node->table->hash, node, hash_alloc_intern); - assert(inserted == node); + rn_hash_node_add(&node->table->hash, node); return node; } @@ -99,9 +99,6 @@ static void route_table_free(struct route_table *rt) if (rt == NULL) return; - hash_clean(rt->hash, NULL); - hash_free(rt->hash); - node = rt->top; /* Bulk deletion of nodes remaining in this table. This function is not @@ -123,6 +120,7 @@ static void route_table_free(struct route_table *rt) tmp_node->table->count--; tmp_node->lock = 0; /* to cause assert if unlocked after this */ + rn_hash_node_del(&rt->hash, tmp_node); route_node_free(rt, tmp_node); if (node != NULL) { @@ -137,6 +135,7 @@ static void route_table_free(struct route_table *rt) assert(rt->count == 0); + rn_hash_node_fini(&rt->hash); XFREE(MTYPE_ROUTE_TABLE, rt); return; } @@ -257,7 +256,7 @@ struct route_node *route_node_lookup(const struct route_table *table, prefix_copy(&p, pu.p); apply_mask(&p); - node = hash_get(table->hash, (void *)&p, NULL); + node = rn_hash_node_find(&table->hash, (void *)&p); return (node && node->info) ? route_lock_node(node) : NULL; } @@ -270,7 +269,7 @@ struct route_node *route_node_lookup_maynull(const struct route_table *table, prefix_copy(&p, pu.p); apply_mask(&p); - node = hash_get(table->hash, (void *)&p, NULL); + node = rn_hash_node_find(&table->hash, (void *)&p); return node ? route_lock_node(node) : NULL; } @@ -282,12 +281,11 @@ struct route_node *route_node_get(struct route_table *const table, struct route_node *new; struct route_node *node; struct route_node *match; - struct route_node *inserted; uint16_t prefixlen = p->prefixlen; const uint8_t *prefix = &p->u.prefix; apply_mask((struct prefix *)p); - node = hash_get(table->hash, (void *)p, NULL); + node = rn_hash_node_find(&table->hash, (void *)p); if (node && node->info) return route_lock_node(node); @@ -314,8 +312,7 @@ struct route_node *route_node_get(struct route_table *const table, new->p.family = p->family; new->table = table; set_link(new, node); - inserted = hash_get(node->table->hash, new, hash_alloc_intern); - assert(inserted == new); + rn_hash_node_add(&table->hash, new); if (match) set_link(match, new); @@ -367,7 +364,7 @@ void route_node_delete(struct route_node *node) node->table->count--; - hash_release(node->table->hash, node); + rn_hash_node_del(&node->table->hash, node); /* WARNING: FRAGILE CODE! * route_node_free may have the side effect of free'ing the entire diff --git a/lib/table.h b/lib/table.h index ce578e795c..3e3fb658ae 100644 --- a/lib/table.h +++ b/lib/table.h @@ -25,6 +25,7 @@ #include "memory.h" #include "hash.h" #include "prefix.h" +#include "typesafe.h" #ifdef __cplusplus extern "C" { @@ -59,10 +60,12 @@ struct route_table_delegate_t_ { route_table_destroy_node_func_t destroy_node; }; +PREDECL_HASH(rn_hash_node) + /* Routing table top structure. */ struct route_table { struct route_node *top; - struct hash *hash; + struct rn_hash_node_head hash; /* * Delegate that performs certain functions for this table. @@ -129,6 +132,7 @@ struct route_table { /* Lock of this radix */ \ unsigned int table_rdonly(lock); \ \ + struct rn_hash_node_item nodehash; \ /* Each node of route. */ \ void *info; \ From be6f84af73d0f9cc28c485512cba200aecded424 Mon Sep 17 00:00:00 2001 From: Lou Berger Date: Wed, 1 May 2019 21:16:14 +0000 Subject: [PATCH 6/6] topotests/bgp_rfapi_basic_sanity: cleanup rfapi using non-debug command Signed-off-by: Lou Berger --- tests/topotests/bgp_rfapi_basic_sanity/scripts/cleanup_all.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/topotests/bgp_rfapi_basic_sanity/scripts/cleanup_all.py b/tests/topotests/bgp_rfapi_basic_sanity/scripts/cleanup_all.py index 096e97fa94..e9c1916f75 100644 --- a/tests/topotests/bgp_rfapi_basic_sanity/scripts/cleanup_all.py +++ b/tests/topotests/bgp_rfapi_basic_sanity/scripts/cleanup_all.py @@ -10,7 +10,8 @@ luCommand('r3','vtysh -c "debug rfapi-dev close vn 10.0.0.2 un 2.2.2.2"','status luCommand('r4','vtysh -c "debug rfapi-dev unregister vn 10.0.0.3 un 3.3.3.3 prefix 33.33.33.0/24"','', 'none', 'Prefix removed') luCommand('r4','vtysh -c "debug rfapi-dev unregister vn 10.0.0.3 un 3.3.3.3 prefix 11.11.11.0/24"','', 'none', 'MP prefix removed') luCommand('r4','vtysh -c "show vnc registrations"','Locally: *Active: 0 ','wait','Local registration removed') -luCommand('r4','vtysh -c "debug rfapi-dev close vn 10.0.0.3 un 3.3.3.3"','status 0', 'pass', 'Closed RFAPI') +#luCommand('r4','vtysh -c "debug rfapi-dev close vn 10.0.0.3 un 3.3.3.3"','status 0', 'pass', 'Closed RFAPI') +luCommand('r4','vtysh -c "clear vnc nve *"','.', 'pass', 'Cleared NVEs') luCommand('r1','vtysh -c "show vnc registrations"','Locally: *Active: 0 .* Remotely: *Active: 0','wait','All registrations cleared') luCommand('r3','vtysh -c "show vnc registrations"','Locally: *Active: 0 .* Remotely: *Active: 0','wait','All registrations cleared')