Merge pull request #1222 from opensourcerouting/isis-spf-improvements

isisd: spf improvements

isisd/isis_spf.c

@@ -36,6 +36,7 @@
 #include "table.h"
 #include "spf_backoff.h"
 #include "jhash.h"
+#include "skiplist.h"
 
 #include "isis_constants.h"
 #include "isis_common.h"
@@ -88,14 +89,18 @@ struct isis_vertex {
 	u_int16_t depth;       /* The depth in the imaginary tree */
 	struct list *Adj_N;    /* {Adj(N)} next hop or neighbor list */
 	struct list *parents;  /* list of parents for ECMP */
-	struct list *children; /* list of children used for tree dump */
+	uint64_t insert_counter;
 };
 
 /* Vertex Queue and associated functions */
 
 struct isis_vertex_queue {
+	union {
+		struct skiplist *slist;
 		struct list *list;
+	} l;
 	struct hash *hash;
+	uint64_t insert_counter;
 };
 
 static unsigned isis_vertex_queue_hash_key(void *vp)
@@ -121,9 +126,50 @@ static int isis_vertex_queue_hash_cmp(const void *a, const void *b)
 	return memcmp(va->N.id, vb->N.id, ISIS_SYS_ID_LEN + 1) == 0;
 }
 
-static void isis_vertex_queue_init(struct isis_vertex_queue *queue, const char *name)
+/*
+ * Compares vertizes for sorting in the TENT list. Returns true
+ * if candidate should be considered before current, false otherwise.
+ */
+static int isis_vertex_queue_tent_cmp(void *a, void *b)
 {
-	queue->list = list_new();
+	struct isis_vertex *va = a;
+	struct isis_vertex *vb = b;
+
+	if (va->d_N < vb->d_N)
+		return -1;
+
+	if (va->d_N > vb->d_N)
+		return 1;
+
+	if (va->type < vb->type)
+		return -1;
+
+	if (va->type > vb->type)
+		return 1;
+
+	if (va->insert_counter < vb->insert_counter)
+		return -1;
+
+	if (va->insert_counter > vb->insert_counter)
+		return 1;
+
+	assert(!"Vertizes should be strictly ordered");
+}
+
+static struct skiplist *isis_vertex_queue_skiplist(void)
+{
+	return skiplist_new(0, isis_vertex_queue_tent_cmp, NULL);
+}
+
+static void isis_vertex_queue_init(struct isis_vertex_queue *queue, const char *name, bool ordered)
+{
+	if (ordered) {
+		queue->insert_counter = 1;
+		queue->l.slist = isis_vertex_queue_skiplist();
+	} else {
+		queue->insert_counter = 0;
+		queue->l.list = list_new();
+	}
 	queue->hash = hash_create(isis_vertex_queue_hash_key,
 				  isis_vertex_queue_hash_cmp,
 				  name);
@@ -135,9 +181,18 @@ static void isis_vertex_queue_clear(struct isis_vertex_queue *queue)
 {
 	hash_clean(queue->hash, NULL);
 
-	queue->list->del = (void (*)(void *))isis_vertex_del;
+	if (queue->insert_counter) {
-	list_delete_all_node(queue->list);
+		struct isis_vertex *vertex;
-	queue->list->del = NULL;
+		while (0 == skiplist_first(queue->l.slist, NULL, (void**)&vertex)) {
+			isis_vertex_del(vertex);
+			skiplist_delete_first(queue->l.slist);
+		}
+		queue->insert_counter = 1;
+	} else {
+		queue->l.list->del = (void (*)(void *))isis_vertex_del;
+		list_delete_all_node(queue->l.list);
+		queue->l.list->del = NULL;
+	}
 }
 
 static void isis_vertex_queue_free(struct isis_vertex_queue *queue)
@@ -147,19 +202,26 @@ static void isis_vertex_queue_free(struct isis_vertex_queue *queue)
 	hash_free(queue->hash);
 	queue->hash = NULL;
 
-	list_delete(queue->list);
+	if (queue->insert_counter) {
-	queue->list = NULL;
+		skiplist_free(queue->l.slist);
+		queue->l.slist = NULL;
+	} else {
+		list_delete(queue->l.list);
+		queue->l.list = NULL;
+	}
 }
 
 static unsigned int isis_vertex_queue_count(struct isis_vertex_queue *queue)
 {
-	return listcount(queue->list);
+	return hashcount(queue->hash);
 }
 
-static void isis_vertex_queue_add(struct isis_vertex_queue *queue,
+static void isis_vertex_queue_append(struct isis_vertex_queue *queue,
 				     struct isis_vertex *vertex)
 {
-	listnode_add(queue->list, vertex);
+	assert(!queue->insert_counter);
+
+	listnode_add(queue->l.list, vertex);
 
 	struct isis_vertex *inserted;
 
@@ -167,17 +229,30 @@ static void isis_vertex_queue_add(struct isis_vertex_queue *queue,
 	assert(inserted == vertex);
 }
 
+static void isis_vertex_queue_insert(struct isis_vertex_queue *queue,
+				     struct isis_vertex *vertex)
+{
+	assert(queue->insert_counter);
+	vertex->insert_counter = queue->insert_counter++;
+	assert(queue->insert_counter != (uint64_t)-1);
+
+	skiplist_insert(queue->l.slist, vertex, vertex);
+
+	struct isis_vertex *inserted;
+	inserted = hash_get(queue->hash, vertex, hash_alloc_intern);
+	assert(inserted == vertex);
+}
+
 static struct isis_vertex *isis_vertex_queue_pop(struct isis_vertex_queue *queue)
 {
-	struct listnode *node;
+	assert(queue->insert_counter);
 
-	node = listhead(queue->list);
+	struct isis_vertex *rv;
-	if (!node)
+
+	if (skiplist_first(queue->l.slist, NULL, (void**)&rv))
 		return NULL;
 
-	struct isis_vertex *rv = listgetdata(node);
+	skiplist_delete_first(queue->l.slist);
-
-	list_delete_node(queue->list, node);
 	hash_release(queue->hash, rv);
 
 	return rv;
@@ -186,12 +261,14 @@ static struct isis_vertex *isis_vertex_queue_pop(struct isis_vertex_queue *queue
 static void isis_vertex_queue_delete(struct isis_vertex_queue *queue,
 				     struct isis_vertex *vertex)
 {
-	listnode_delete(queue->list, vertex);
+	assert(queue->insert_counter);
+
+	skiplist_delete(queue->l.slist, vertex, vertex);
 	hash_release(queue->hash, vertex);
 }
 
 #define ALL_QUEUE_ELEMENTS_RO(queue, node, data) \
-		ALL_LIST_ELEMENTS_RO((queue)->list, node, data)
+		ALL_LIST_ELEMENTS_RO((queue)->l.list, node, data)
 
 
 /* End of vertex queue definitions */
@@ -353,7 +430,6 @@ static struct isis_vertex *isis_vertex_new(void *id, enum vertextype vtype)
 
 	vertex->Adj_N = list_new();
 	vertex->parents = list_new();
-	vertex->children = list_new();
 
 	return vertex;
 }
@@ -364,8 +440,6 @@ static void isis_vertex_del(struct isis_vertex *vertex)
 	vertex->Adj_N = NULL;
 	list_delete(vertex->parents);
 	vertex->parents = NULL;
-	list_delete(vertex->children);
-	vertex->children = NULL;
 
 	memset(vertex, 0, sizeof(struct isis_vertex));
 	XFREE(MTYPE_ISIS_VERTEX, vertex);
@@ -397,8 +471,8 @@ struct isis_spftree *isis_spftree_new(struct isis_area *area)
 		return NULL;
 	}
 
-	isis_vertex_queue_init(&tree->tents, "IS-IS SPF tents");
+	isis_vertex_queue_init(&tree->tents, "IS-IS SPF tents", true);
-	isis_vertex_queue_init(&tree->paths, "IS-IS SPF paths");
+	isis_vertex_queue_init(&tree->paths, "IS-IS SPF paths", false);
 	tree->area = area;
 	tree->last_run_timestamp = 0;
 	tree->last_run_duration = 0;
@@ -422,8 +496,7 @@ static void isis_spftree_adj_del(struct isis_spftree *spftree,
 	struct isis_vertex *v;
 	if (!adj)
 		return;
-	for (ALL_QUEUE_ELEMENTS_RO(&spftree->tents, node, v))
+	assert(!isis_vertex_queue_count(&spftree->tents));
-		isis_vertex_adj_del(v, adj);
 	for (ALL_QUEUE_ELEMENTS_RO(&spftree->paths, node, v))
 		isis_vertex_adj_del(v, adj);
 	return;
@@ -538,7 +611,7 @@ static struct isis_vertex *isis_spf_add_root(struct isis_spftree *spftree,
 				 spftree->area->oldmetric
 					 ? VTYPE_NONPSEUDO_IS
 					 : VTYPE_NONPSEUDO_TE_IS);
-	isis_vertex_queue_add(&spftree->paths, vertex);
+	isis_vertex_queue_append(&spftree->paths, vertex);
 
 #ifdef EXTREME_DEBUG
 	zlog_debug("ISIS-Spf: added this IS  %s %s depth %d dist %d to PATHS",
@@ -559,45 +632,6 @@ static struct isis_vertex *isis_find_vertex(struct isis_vertex_queue *queue, voi
 	return hash_lookup(queue->hash, &querier);
 }
 
-/*
- * Compares vertizes for sorting in the TENT list. Returns true
- * if candidate should be considered before current, false otherwise.
- */
-static bool tent_cmp(struct isis_vertex *current, struct isis_vertex *candidate)
-{
-	if (current->d_N > candidate->d_N)
-		return true;
-
-	if (current->d_N == candidate->d_N && current->type > candidate->type)
-		return true;
-
-	return false;
-}
-
-static void isis_vertex_queue_insert(struct isis_vertex_queue *queue,
-				     struct isis_vertex *vertex)
-{
-	struct listnode *node;
-	struct isis_vertex *v;
-
-	/* XXX: This cant use the standard ALL_LIST_ELEMENTS macro */
-	for (node = listhead(queue->list); node; node = listnextnode(node)) {
-		v = listgetdata(node);
-		if (tent_cmp(v, vertex)) {
-			listnode_add_before(queue->list, node, vertex);
-			break;
-		}
-	}
-
-	if (node == NULL)
-		listnode_add(queue->list, vertex);
-
-	struct isis_vertex *inserted;
-
-	inserted = hash_get(queue->hash, vertex, hash_alloc_intern);
-	assert(inserted == vertex);
-}
-
 /*
  * Add a vertex to TENT sorted by cost and by vertextype on tie break situation
  */
@@ -622,8 +656,6 @@ static struct isis_vertex *isis_spf_add2tent(struct isis_spftree *spftree,
 
 	if (parent) {
 		listnode_add(vertex->parents, parent);
-		if (listnode_lookup(parent->children, vertex) == NULL)
-			listnode_add(parent->children, vertex);
 	}
 
 	if (parent && parent->Adj_N && listcount(parent->Adj_N) > 0) {
@@ -665,9 +697,6 @@ static void isis_spf_add_local(struct isis_spftree *spftree,
 			if (parent && (listnode_lookup(vertex->parents, parent)
 				       == NULL))
 				listnode_add(vertex->parents, parent);
-			if (parent && (listnode_lookup(parent->children, vertex)
-				       == NULL))
-				listnode_add(parent->children, vertex);
 			return;
 		} else if (vertex->d_N < cost) {
 			/*       e) do nothing */
@@ -677,10 +706,6 @@ static void isis_spf_add_local(struct isis_spftree *spftree,
 			struct listnode *pnode, *pnextnode;
 			struct isis_vertex *pvertex;
 			isis_vertex_queue_delete(&spftree->tents, vertex);
-			assert(listcount(vertex->children) == 0);
-			for (ALL_LIST_ELEMENTS(vertex->parents, pnode,
-					       pnextnode, pvertex))
-				listnode_delete(pvertex->children, vertex);
 			isis_vertex_del(vertex);
 		}
 	}
@@ -756,9 +781,6 @@ static void process_N(struct isis_spftree *spftree, enum vertextype vtype,
 				remove_excess_adjs(vertex->Adj_N);
 			if (listnode_lookup(vertex->parents, parent) == NULL)
 				listnode_add(vertex->parents, parent);
-			if (listnode_lookup(parent->children, vertex) == NULL)
-				listnode_add(parent->children, vertex);
-			/*      3) */
 			return;
 		} else if (vertex->d_N < dist) {
 			return;
@@ -767,10 +789,6 @@ static void process_N(struct isis_spftree *spftree, enum vertextype vtype,
 			struct listnode *pnode, *pnextnode;
 			struct isis_vertex *pvertex;
 			isis_vertex_queue_delete(&spftree->tents, vertex);
-			assert(listcount(vertex->children) == 0);
-			for (ALL_LIST_ELEMENTS(vertex->parents, pnode,
-					       pnextnode, pvertex))
-				listnode_delete(pvertex->children, vertex);
 			isis_vertex_del(vertex);
 		}
 	}
@@ -1198,7 +1216,7 @@ static void add_to_paths(struct isis_spftree *spftree,
 
 	if (isis_find_vertex(&spftree->paths, vertex->N.id, vertex->type))
 		return;
-	isis_vertex_queue_add(&spftree->paths, vertex);
+	isis_vertex_queue_append(&spftree->paths, vertex);
 
 #ifdef EXTREME_DEBUG
 	zlog_debug("ISIS-Spf: added %s %s %s depth %d dist %d to PATHS",

tests/.gitignore

@@ -26,6 +26,7 @@ __pycache__
 /bgpd/test_mpath
 /isisd/test_fuzz_isis_tlv
 /isisd/test_fuzz_isis_tlv_tests.h
+/isisd/test_isis_vertex_queue
 /lib/cli/test_cli
 /lib/cli/test_commands
 /lib/cli/test_commands_defun.c

tests/Makefile.am

@@ -29,7 +29,9 @@ if SOLARIS
 TESTS_ISISD =
 else
 TESTS_ISISD = \
-	isisd/test_fuzz_isis_tlv
+	isisd/test_fuzz_isis_tlv \
+	isisd/test_isis_vertex_queue \
+	# end
 endif
 else
 TESTS_ISISD =
@@ -135,6 +137,7 @@ bgpd_test_mp_attr_SOURCES = bgpd/test_mp_attr.c
 bgpd_test_mpath_SOURCES = bgpd/test_mpath.c
 isisd_test_fuzz_isis_tlv_SOURCES = isisd/test_fuzz_isis_tlv.c
 isisd_test_fuzz_isis_tlv_CPPFLAGS = $(AM_CPPFLAGS) -I$(top_builddir)/tests/isisd
+isisd_test_isis_vertex_queue_SOURCES = isisd/test_isis_vertex_queue.c
 
 ospf6d_test_lsdb_SOURCES = ospf6d/test_lsdb.c lib/cli/common_cli.c
 
@@ -168,6 +171,7 @@ bgpd_test_ecommunity_LDADD = $(BGP_TEST_LDADD)
 bgpd_test_mp_attr_LDADD = $(BGP_TEST_LDADD)
 bgpd_test_mpath_LDADD = $(BGP_TEST_LDADD)
 isisd_test_fuzz_isis_tlv_LDADD = $(ISISD_TEST_LDADD)
+isisd_test_isis_vertex_queue_LDADD = $(ISISD_TEST_LDADD)
 ospf6d_test_lsdb_LDADD = $(OSPF6_TEST_LDADD)
 
 EXTRA_DIST = \
@@ -181,6 +185,7 @@ EXTRA_DIST = \
     helpers/python/frrtest.py \
     isisd/test_fuzz_isis_tlv.py \
     isisd/test_fuzz_isis_tlv_tests.h.gz \
+    isisd/test_isis_vertex_queue.py \
     lib/cli/test_commands.in \
     lib/cli/test_commands.py \
     lib/cli/test_commands.refout \

tests/isisd/test_isis_vertex_queue.c

@@ -0,0 +1,108 @@
+#include <zebra.h>
+
+#include "isisd/isis_spf.c"
+
+struct thread_master *master;
+int isis_sock_init(struct isis_circuit *circuit);
+int isis_sock_init(struct isis_circuit *circuit)
+{
+	return 0;
+}
+
+static struct isis_vertex **vertices;
+static size_t vertex_count;
+
+static void setup_test_vertices(void)
+{
+	struct prefix p = {
+		.family = AF_UNSPEC
+	};
+	uint8_t node_id[7];
+
+	vertices = XMALLOC(MTYPE_TMP, sizeof(*vertices) * 16);
+
+	p.family = AF_INET;
+	p.prefixlen = 24;
+	inet_pton(AF_INET, "192.168.1.0", &p.u.prefix4);
+	vertices[vertex_count] = isis_vertex_new(&p, VTYPE_IPREACH_TE);
+	vertices[vertex_count]->d_N = 20;
+	vertex_count++;
+
+	p.family = AF_INET;
+	p.prefixlen = 24;
+	inet_pton(AF_INET, "192.168.2.0", &p.u.prefix4);
+	vertices[vertex_count] = isis_vertex_new(&p, VTYPE_IPREACH_TE);
+	vertices[vertex_count]->d_N = 20;
+	vertex_count++;
+
+	memset(node_id, 0, sizeof(node_id));
+	node_id[6] = 1;
+	vertices[vertex_count] = isis_vertex_new(node_id, VTYPE_PSEUDO_TE_IS);
+	vertices[vertex_count]->d_N = 15;
+	vertex_count++;
+
+	memset(node_id, 0, sizeof(node_id));
+	node_id[5] = 2;
+	vertices[vertex_count] = isis_vertex_new(node_id, VTYPE_NONPSEUDO_TE_IS);
+	vertices[vertex_count]->d_N = 15;
+	vertex_count++;
+
+	p.family = AF_INET;
+	p.prefixlen = 24;
+	inet_pton(AF_INET, "192.168.3.0", &p.u.prefix4);
+	vertices[vertex_count] = isis_vertex_new(&p, VTYPE_IPREACH_TE);
+	vertices[vertex_count]->d_N = 20;
+	vertex_count++;
+};
+
+static void cleanup_test_vertices(void)
+{
+	for (size_t i = 0; i < vertex_count; i++)
+		isis_vertex_del(vertices[i]);
+	XFREE(MTYPE_TMP, vertices);
+	vertex_count = 0;
+}
+
+static void test_ordered(void)
+{
+	struct isis_vertex_queue q;
+
+	isis_vertex_queue_init(&q, NULL, true);
+	for (size_t i = 0; i < vertex_count; i++)
+		isis_vertex_queue_insert(&q, vertices[i]);
+
+	assert(isis_vertex_queue_count(&q) == vertex_count);
+
+	for (size_t i = 0; i < vertex_count; i++) {
+		assert(isis_find_vertex(&q, vertices[i]->N.id, vertices[i]->type) == vertices[i]);
+	}
+
+	assert(isis_vertex_queue_pop(&q) == vertices[2]);
+	assert(isis_find_vertex(&q, vertices[2]->N.id, vertices[2]->type) == NULL);
+
+	assert(isis_vertex_queue_pop(&q) == vertices[3]);
+	assert(isis_find_vertex(&q, vertices[3]->N.id, vertices[3]->type) == NULL);
+
+	assert(isis_vertex_queue_pop(&q) == vertices[0]);
+	assert(isis_find_vertex(&q, vertices[0]->N.id, vertices[0]->type) == NULL);
+
+	assert(isis_vertex_queue_pop(&q) == vertices[1]);
+	assert(isis_find_vertex(&q, vertices[1]->N.id, vertices[1]->type) == NULL);
+
+	assert(isis_vertex_queue_pop(&q) == vertices[4]);
+	assert(isis_find_vertex(&q, vertices[4]->N.id, vertices[4]->type) == NULL);
+
+	assert(isis_vertex_queue_count(&q) == 0);
+	assert(isis_vertex_queue_pop(&q) == NULL);
+
+	isis_vertex_queue_free(&q);
+}
+
+int main(int argc, char **argv)
+{
+	setup_test_vertices();
+	test_ordered();
+	cleanup_test_vertices();
+
+	return 0;
+}

tests/isisd/test_isis_vertex_queue.py

@@ -0,0 +1,6 @@
+import frrtest
+
+class TestIsisVertexQueue(frrtest.TestMultiOut):
+    program = './test_isis_vertex_queue'
+
+TestIsisVertexQueue.exit_cleanly()