Merge pull request #4763 from opensourcerouting/ds-work

lib: get rid of pqueue_*, use DECLARE_HEAP in thread.c

doc/developer/lists.rst

@@ -611,6 +611,38 @@ Head removal (pop) and deallocation:
     * note nothing between wrlock() and unlock() */
    XFREE(MTYPE_ITEM, i);
 
+FAQ
+---
+
+Why is the list head not ``const`` in the list APIs?
+   The semantics that a ``const`` list head would imply are not obvious.  It
+   could mean any of the following:
+
+    * the list just shouldn't be allocated/deallocated, but may be modified.
+      This doesn't actually work since the list head needs to be modified for
+      inserting or deleting items.
+
+    * the list shouldn't be modified, but items can.  This may make sense for
+      iterating, but it's not exactly consistent - an item might be on more
+      than one list, does it apply to all of them?  If not, which one?
+
+    * neither the list nor the items should be modified.  This is consistent,
+      but hard to do without creating a ``const`` copy of every single list
+      function.  Ease of use trumps this.
+
+Why is there no "is this item on a/the list" test?
+   It's slow for several of the data structures, and the work of adding it
+   just hasn't been done.  It can certainly be added if it's needed.
+
+Why is it ``PREDECL`` + ``DECLARE`` instead of ``DECLARE`` + ``DEFINE``?
+   The rule is that a ``DEFINE`` must be in a ``.c`` file, and linked exactly
+   once because it defines some kind of global symbol.  This is not the case
+   for the data structure macros;  they only define ``static`` symbols and it
+   is perfectly fine to include both ``PREDECL`` and ``DECLARE`` in a header
+   file.  It is also perfectly fine to have the same ``DECLARE`` statement in
+   2 ``.c`` files, but only **if the macro arguments are identical.**  Maybe
+   don't do that unless you really need it.
+
 FRR lists
 ---------
 

lib/pqueue.c

@@ -1,185 +0,0 @@
-/* Priority queue functions.
- * Copyright (C) 2003 Yasuhiro Ohara
- *
- * This file is part of GNU Zebra.
- *
- * GNU Zebra is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published
- * by the Free Software Foundation; either version 2, or (at your
- * option) any later version.
- *
- * GNU Zebra is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; see the file COPYING; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
-
-#include <zebra.h>
-
-#include "memory.h"
-#include "pqueue.h"
-
-DEFINE_MTYPE_STATIC(LIB, PQUEUE, "Priority queue")
-DEFINE_MTYPE_STATIC(LIB, PQUEUE_DATA, "Priority queue data")
-
-/* priority queue using heap sort */
-
-/* pqueue->cmp() controls the order of sorting (i.e, ascending or
-   descending). If you want the left node to move upper of the heap
-   binary tree, make cmp() to return less than 0.  for example, if cmp
-   (10, 20) returns -1, the sorting is ascending order. if cmp (10,
-   20) returns 1, the sorting is descending order. if cmp (10, 20)
-   returns 0, this library does not do sorting (which will not be what
-   you want).  To be brief, if the contents of cmp_func (left, right)
-   is left - right, dequeue () returns the smallest node.  Otherwise
-   (if the contents is right - left), dequeue () returns the largest
-   node.  */
-
-#define DATA_SIZE (sizeof (void *))
-#define PARENT_OF(x) ((x - 1) / 2)
-#define LEFT_OF(x)  (2 * x + 1)
-#define RIGHT_OF(x) (2 * x + 2)
-#define HAVE_CHILD(x,q) (x < (q)->size / 2)
-
-void trickle_up(int index, struct pqueue *queue)
-{
-	void *tmp;
-
-	/* Save current node as tmp node.  */
-	tmp = queue->array[index];
-
-	/* Continue until the node reaches top or the place where the parent
-	   node should be upper than the tmp node.  */
-	while (index > 0
-	       && (*queue->cmp)(tmp, queue->array[PARENT_OF(index)]) < 0) {
-		/* actually trickle up */
-		queue->array[index] = queue->array[PARENT_OF(index)];
-		if (queue->update != NULL)
-			(*queue->update)(queue->array[index], index);
-		index = PARENT_OF(index);
-	}
-
-	/* Restore the tmp node to appropriate place.  */
-	queue->array[index] = tmp;
-	if (queue->update != NULL)
-		(*queue->update)(tmp, index);
-}
-
-void trickle_down(int index, struct pqueue *queue)
-{
-	void *tmp;
-	int which;
-
-	/* Save current node as tmp node.  */
-	tmp = queue->array[index];
-
-	/* Continue until the node have at least one (left) child.  */
-	while (HAVE_CHILD(index, queue)) {
-		/* If right child exists, and if the right child is more proper
-		   to be moved upper.  */
-		if (RIGHT_OF(index) < queue->size
-		    && (*queue->cmp)(queue->array[LEFT_OF(index)],
-				     queue->array[RIGHT_OF(index)])
-			       > 0)
-			which = RIGHT_OF(index);
-		else
-			which = LEFT_OF(index);
-
-		/* If the tmp node should be upper than the child, break.  */
-		if ((*queue->cmp)(queue->array[which], tmp) > 0)
-			break;
-
-		/* Actually trickle down the tmp node.  */
-		queue->array[index] = queue->array[which];
-		if (queue->update != NULL)
-			(*queue->update)(queue->array[index], index);
-		index = which;
-	}
-
-	/* Restore the tmp node to appropriate place.  */
-	queue->array[index] = tmp;
-	if (queue->update != NULL)
-		(*queue->update)(tmp, index);
-}
-
-struct pqueue *pqueue_create(void)
-{
-	struct pqueue *queue;
-
-	queue = XCALLOC(MTYPE_PQUEUE, sizeof(struct pqueue));
-
-	queue->array =
-		XCALLOC(MTYPE_PQUEUE_DATA, DATA_SIZE * PQUEUE_INIT_ARRAYSIZE);
-	queue->array_size = PQUEUE_INIT_ARRAYSIZE;
-
-	/* By default we want nothing to happen when a node changes. */
-	queue->update = NULL;
-	return queue;
-}
-
-void pqueue_delete(struct pqueue *queue)
-{
-	XFREE(MTYPE_PQUEUE_DATA, queue->array);
-	XFREE(MTYPE_PQUEUE, queue);
-}
-
-static int pqueue_expand(struct pqueue *queue)
-{
-	void **newarray;
-
-	newarray =
-		XCALLOC(MTYPE_PQUEUE_DATA, queue->array_size * DATA_SIZE * 2);
-
-	memcpy(newarray, queue->array, queue->array_size * DATA_SIZE);
-
-	XFREE(MTYPE_PQUEUE_DATA, queue->array);
-	queue->array = newarray;
-	queue->array_size *= 2;
-
-	return 1;
-}
-
-void pqueue_enqueue(void *data, struct pqueue *queue)
-{
-	if (queue->size + 2 >= queue->array_size && !pqueue_expand(queue))
-		return;
-
-	queue->array[queue->size] = data;
-	if (queue->update != NULL)
-		(*queue->update)(data, queue->size);
-	trickle_up(queue->size, queue);
-	queue->size++;
-}
-
-void *pqueue_dequeue(struct pqueue *queue)
-{
-	void *data = queue->array[0];
-	queue->array[0] = queue->array[--queue->size];
-	trickle_down(0, queue);
-	return data;
-}
-
-void pqueue_remove_at(int index, struct pqueue *queue)
-{
-	queue->array[index] = queue->array[--queue->size];
-
-	if (index > 0
-	    && (*queue->cmp)(queue->array[index],
-			     queue->array[PARENT_OF(index)])
-		       < 0) {
-		trickle_up(index, queue);
-	} else {
-		trickle_down(index, queue);
-	}
-}
-
-void pqueue_remove(void *data, struct pqueue *queue)
-{
-	for (int i = 0; i < queue->size; i++)
-		if (queue->array[i] == data)
-			pqueue_remove_at(i, queue);
-}

lib/pqueue.h

@@ -1,54 +0,0 @@
-/* Priority queue functions.
- * Copyright (C) 2003 Yasuhiro Ohara
- *
- * This file is part of GNU Zebra.
- *
- * GNU Zebra is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published
- * by the Free Software Foundation; either version 2, or (at your
- * option) any later version.
- *
- * GNU Zebra is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; see the file COPYING; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
- */
-
-#ifndef _ZEBRA_PQUEUE_H
-#define _ZEBRA_PQUEUE_H
-
-#ifdef __cplusplus
-extern "C" {
-#endif
-
-struct pqueue {
-	void **array;
-	int array_size;
-	int size;
-
-	int (*cmp)(void *, void *);
-	void (*update)(void *node, int actual_position);
-};
-
-#define PQUEUE_INIT_ARRAYSIZE  32
-
-extern struct pqueue *pqueue_create(void);
-extern void pqueue_delete(struct pqueue *queue);
-
-extern void pqueue_enqueue(void *data, struct pqueue *queue);
-extern void *pqueue_dequeue(struct pqueue *queue);
-extern void pqueue_remove_at(int index, struct pqueue *queue);
-extern void pqueue_remove(void *data, struct pqueue *queue);
-
-extern void trickle_down(int index, struct pqueue *queue);
-extern void trickle_up(int index, struct pqueue *queue);
-
-#ifdef __cplusplus
-}
-#endif
-
-#endif /* _ZEBRA_PQUEUE_H */

lib/subdir.am

@@ -61,7 +61,6 @@ lib_libfrr_la_SOURCES = \
 	lib/openbsd-tree.c \
 	lib/pid_output.c \
 	lib/plist.c \
-	lib/pqueue.c \
 	lib/prefix.c \
 	lib/privs.c \
 	lib/ptm_lib.c \
@@ -198,7 +197,6 @@ pkginclude_HEADERS += \
 	lib/openbsd-queue.h \
 	lib/openbsd-tree.h \
 	lib/plist.h \
-	lib/pqueue.h \
 	lib/prefix.h \
 	lib/printfrr.h \
 	lib/privs.h \

lib/thread.c

@@ -27,7 +27,6 @@
 #include "memory.h"
 #include "log.h"
 #include "hash.h"
-#include "pqueue.h"
 #include "command.h"
 #include "sigevent.h"
 #include "network.h"
@@ -42,6 +41,22 @@ DEFINE_MTYPE_STATIC(LIB, THREAD_STATS, "Thread stats")
 
 DECLARE_LIST(thread_list, struct thread, threaditem)
 
+static int thread_timer_cmp(const struct thread *a, const struct thread *b)
+{
+	if (a->u.sands.tv_sec < b->u.sands.tv_sec)
+		return -1;
+	if (a->u.sands.tv_sec > b->u.sands.tv_sec)
+		return 1;
+	if (a->u.sands.tv_usec < b->u.sands.tv_usec)
+		return -1;
+	if (a->u.sands.tv_usec > b->u.sands.tv_usec)
+		return 1;
+	return 0;
+}
+
+DECLARE_HEAP(thread_timer_list, struct thread, timeritem,
+		thread_timer_cmp)
+
 #if defined(__APPLE__)
 #include <mach/mach.h>
 #include <mach/mach_time.h>
@@ -401,25 +416,6 @@ void thread_cmd_init(void)
 /* CLI end ------------------------------------------------------------------ */
 
 
-static int thread_timer_cmp(void *a, void *b)
-{
-	struct thread *thread_a = a;
-	struct thread *thread_b = b;
-
-	if (timercmp(&thread_a->u.sands, &thread_b->u.sands, <))
-		return -1;
-	if (timercmp(&thread_a->u.sands, &thread_b->u.sands, >))
-		return 1;
-	return 0;
-}
-
-static void thread_timer_update(void *node, int actual_position)
-{
-	struct thread *thread = node;
-
-	thread->index = actual_position;
-}
-
 static void cancelreq_del(void *cr)
 {
 	XFREE(MTYPE_TMP, cr);
@@ -464,11 +460,7 @@ struct thread_master *thread_master_create(const char *name)
 	thread_list_init(&rv->event);
 	thread_list_init(&rv->ready);
 	thread_list_init(&rv->unuse);
-
+	thread_timer_list_init(&rv->timer);
-	/* Initialize the timer queues */
-	rv->timer = pqueue_create();
-	rv->timer->cmp = thread_timer_cmp;
-	rv->timer->update = thread_timer_update;
 
 	/* Initialize thread_fetch() settings */
 	rv->spin = true;
@@ -566,16 +558,6 @@ static void thread_array_free(struct thread_master *m,
 	XFREE(MTYPE_THREAD_POLL, thread_array);
 }
 
-static void thread_queue_free(struct thread_master *m, struct pqueue *queue)
-{
-	int i;
-
-	for (i = 0; i < queue->size; i++)
-		thread_free(m, queue->array[i]);
-
-	pqueue_delete(queue);
-}
-
 /*
  * thread_master_free_unused
  *
@@ -598,6 +580,8 @@ void thread_master_free_unused(struct thread_master *m)
 /* Stop thread scheduler. */
 void thread_master_free(struct thread_master *m)
 {
+	struct thread *t;
+
 	pthread_mutex_lock(&masters_mtx);
 	{
 		listnode_delete(masters, m);
@@ -609,7 +593,8 @@ void thread_master_free(struct thread_master *m)
 
 	thread_array_free(m, m->read);
 	thread_array_free(m, m->write);
-	thread_queue_free(m, m->timer);
+	while ((t = thread_timer_list_pop(&m->timer)))
+		thread_free(m, t);
 	thread_list_free(m, &m->event);
 	thread_list_free(m, &m->ready);
 	thread_list_free(m, &m->unuse);
@@ -683,7 +668,6 @@ static struct thread *thread_get(struct thread_master *m, uint8_t type,
 	thread->add_type = type;
 	thread->master = m;
 	thread->arg = arg;
-	thread->index = -1;
 	thread->yield = THREAD_YIELD_TIME_SLOT; /* default */
 	thread->ref = NULL;
 
@@ -854,7 +838,6 @@ funcname_thread_add_timer_timeval(struct thread_master *m,
 				  struct thread **t_ptr, debugargdef)
 {
 	struct thread *thread;
-	struct pqueue *queue;
 
 	assert(m != NULL);
 
@@ -870,7 +853,6 @@ funcname_thread_add_timer_timeval(struct thread_master *m,
 			return NULL;
 		}
 
-		queue = m->timer;
 		thread = thread_get(m, type, func, arg, debugargpass);
 
 		pthread_mutex_lock(&thread->mtx);
@@ -878,7 +860,7 @@ funcname_thread_add_timer_timeval(struct thread_master *m,
 			monotime(&thread->u.sands);
 			timeradd(&thread->u.sands, time_relative,
 				 &thread->u.sands);
-			pqueue_enqueue(thread, queue);
+			thread_timer_list_add(&m->timer, thread);
 			if (t_ptr) {
 				*t_ptr = thread;
 				thread->ref = t_ptr;
@@ -1055,7 +1037,6 @@ static void thread_cancel_rw(struct thread_master *master, int fd, short state)
 static void do_thread_cancel(struct thread_master *master)
 {
 	struct thread_list_head *list = NULL;
-	struct pqueue *queue = NULL;
 	struct thread **thread_array = NULL;
 	struct thread *thread;
 
@@ -1111,7 +1092,7 @@ static void do_thread_cancel(struct thread_master *master)
 			thread_array = master->write;
 			break;
 		case THREAD_TIMER:
-			queue = master->timer;
+			thread_timer_list_del(&master->timer, thread);
 			break;
 		case THREAD_EVENT:
 			list = &master->event;
@@ -1124,16 +1105,10 @@ static void do_thread_cancel(struct thread_master *master)
 			break;
 		}
 
-		if (queue) {
+		if (list) {
-			assert(thread->index >= 0);
-			assert(thread == queue->array[thread->index]);
-			pqueue_remove_at(thread->index, queue);
-		} else if (list) {
 			thread_list_del(list, thread);
 		} else if (thread_array) {
 			thread_array[thread->u.fd] = NULL;
-		} else {
-			assert(!"Thread should be either in queue or list or array!");
 		}
 
 		if (thread->ref)
@@ -1251,15 +1226,15 @@ void thread_cancel_async(struct thread_master *master, struct thread **thread,
 }
 /* ------------------------------------------------------------------------- */
 
-static struct timeval *thread_timer_wait(struct pqueue *queue,
+static struct timeval *thread_timer_wait(struct thread_timer_list_head *timers,
 					 struct timeval *timer_val)
 {
-	if (queue->size) {
+	if (!thread_timer_list_count(timers))
-		struct thread *next_timer = queue->array[0];
+		return NULL;
-		monotime_until(&next_timer->u.sands, timer_val);
+
-		return timer_val;
+	struct thread *next_timer = thread_timer_list_first(timers);
-	}
+	monotime_until(&next_timer->u.sands, timer_val);
-	return NULL;
+	return timer_val;
 }
 
 static struct thread *thread_run(struct thread_master *m, struct thread *thread,
@@ -1369,17 +1344,16 @@ static void thread_process_io(struct thread_master *m, unsigned int num)
 }
 
 /* Add all timers that have popped to the ready list. */
-static unsigned int thread_process_timers(struct pqueue *queue,
+static unsigned int thread_process_timers(struct thread_timer_list_head *timers,
 					  struct timeval *timenow)
 {
 	struct thread *thread;
 	unsigned int ready = 0;
 
-	while (queue->size) {
+	while ((thread = thread_timer_list_first(timers))) {
-		thread = queue->array[0];
 		if (timercmp(timenow, &thread->u.sands, <))
 			return ready;
-		pqueue_dequeue(queue);
+		thread_timer_list_pop(timers);
 		thread->type = THREAD_READY;
 		thread_list_add_tail(&thread->master->ready, thread);
 		ready++;
@@ -1461,7 +1435,7 @@ struct thread *thread_fetch(struct thread_master *m, struct thread *fetch)
 		 * once per loop to avoid starvation by events
 		 */
 		if (!thread_list_count(&m->ready))
-			tw = thread_timer_wait(m->timer, &tv);
+			tw = thread_timer_wait(&m->timer, &tv);
 
 		if (thread_list_count(&m->ready) ||
 				(tw && !timercmp(tw, &zerotime, >)))
@@ -1506,7 +1480,7 @@ struct thread *thread_fetch(struct thread_master *m, struct thread *fetch)
 
 		/* Post timers to ready queue. */
 		monotime(&now);
-		thread_process_timers(m->timer, &now);
+		thread_process_timers(&m->timer, &now);
 
 		/* Post I/O to ready queue. */
 		if (num > 0)

lib/thread.h

@@ -41,8 +41,7 @@ struct rusage_t {
 #define GETRUSAGE(X) thread_getrusage(X)
 
 PREDECL_LIST(thread_list)
-
+PREDECL_HEAP(thread_timer_list)
-struct pqueue;
 
 struct fd_handler {
 	/* number of pfd that fit in the allocated space of pfds. This is a
@@ -73,7 +72,7 @@ struct thread_master {
 
 	struct thread **read;
 	struct thread **write;
-	struct pqueue *timer;
+	struct thread_timer_list_head timer;
 	struct thread_list_head event, ready, unuse;
 	struct list *cancel_req;
 	bool canceled;
@@ -95,6 +94,7 @@ struct thread {
 	uint8_t type;		  /* thread type */
 	uint8_t add_type;	  /* thread type */
 	struct thread_list_item threaditem;
+	struct thread_timer_list_item timeritem;
 	struct thread **ref;	  /* external reference (if given) */
 	struct thread_master *master; /* pointer to the struct thread_master */
 	int (*func)(struct thread *); /* event function */
@@ -104,7 +104,6 @@ struct thread {
 		int fd;		      /* file descriptor in case of r/w */
 		struct timeval sands; /* rest of time sands value. */
 	} u;
-	int index; /* queue position for timers */
 	struct timeval real;
 	struct cpu_thread_history *hist; /* cache pointer to cpu_history */
 	unsigned long yield;		 /* yield time in microseconds */

tests/lib/cxxcompat.c

@@ -72,7 +72,6 @@
 #include "lib/openbsd-tree.h"
 #include "lib/pbr.h"
 #include "lib/plist.h"
-#include "lib/pqueue.h"
 #include "lib/prefix.h"
 #include "lib/privs.h"
 #include "lib/ptm_lib.h"

tests/lib/test_timer_correctness.c

@@ -28,7 +28,6 @@
 #include <unistd.h>
 
 #include "memory.h"
-#include "pqueue.h"
 #include "prng.h"
 #include "thread.h"
 

tests/lib/test_timer_performance.c

@@ -28,7 +28,6 @@
 #include <unistd.h>
 
 #include "thread.h"
-#include "pqueue.h"
 #include "prng.h"
 
 #define SCHEDULE_TIMERS 1000000