Refactor taskmgr to run on top of netmgr

This commit changes the taskmgr to run the individual tasks on the
netmgr internal workers.  While an effort has been put into keeping the
taskmgr interface intact, couple of changes have been made:

 * The taskmgr has no concept of universal privileged mode - rather the
   tasks are either privileged or unprivileged (normal).  The privileged
   tasks are run as a first thing when the netmgr is unpaused.  There
   are now four different queues in in the netmgr:

   1. priority queue - netievent on the priority queue are run even when
      the taskmgr enter exclusive mode and netmgr is paused.  This is
      needed to properly start listening on the interfaces, free
      resources and resume.

   2. privileged task queue - only privileged tasks are queued here and
      this is the first queue that gets processed when network manager
      is unpaused using isc_nm_resume().  All netmgr workers need to
      clean the privileged task queue before they all proceed normal
      operation.  Both task queues are processed when the workers are
      finished.

   3. task queue - only (traditional) task are scheduled here and this
      queue along with privileged task queues are process when the
      netmgr workers are finishing.  This is needed to process the task
      shutdown events.

   4. normal queue - this is the queue with netmgr events, e.g. reading,
      sending, callbacks and pretty much everything is processed here.

 * The isc_taskmgr_create() now requires initialized netmgr (isc_nm_t)
   object.

 * The isc_nm_destroy() function now waits for indefinite time, but it
   will print out the active objects when in tracing mode
   (-DNETMGR_TRACE=1 and -DNETMGR_TRACE_VERBOSE=1), the netmgr has been
   made a little bit more asynchronous and it might take longer time to
   shutdown all the active networking connections.

 * Previously, the isc_nm_stoplistening() was a synchronous operation.
   This has been changed and the isc_nm_stoplistening() just schedules
   the child sockets to stop listening and exits.  This was needed to
   prevent a deadlock as the the (traditional) tasks are now executed on
   the netmgr threads.

 * The socket selection logic in isc__nm_udp_send() was flawed, but
   fortunatelly, it was broken, so we never hit the problem where we
   created uvreq_t on a socket from nmhandle_t, but then a different
   socket could be picked up and then we were trying to run the send
   callback on a socket that had different threadid than currently
   running.

lib/isc/tests/isctest.c

@@ -64,12 +64,12 @@ cleanup_managers(void) {
 	if (taskmgr != NULL) {
 		isc_taskmgr_destroy(&taskmgr);
 	}
+	if (netmgr != NULL) {
+		isc_nm_destroy(&netmgr);
+	}
 	if (timermgr != NULL) {
 		isc_timermgr_destroy(&timermgr);
 	}
-	if (netmgr != NULL) {
-		isc_nm_detach(&netmgr);
-	}
 }
 
 static isc_result_t
@@ -89,7 +89,7 @@ create_managers(unsigned int workers) {
 	isc_hp_init(6 * workers);
 
 	netmgr = isc_nm_start(test_mctx, workers);
-	CHECK(isc_taskmgr_create(test_mctx, workers, 0, netmgr, &taskmgr));
+	CHECK(isc_taskmgr_create(test_mctx, 0, netmgr, &taskmgr));
 	CHECK(isc_task_create(taskmgr, 0, &maintask));
 	isc_taskmgr_setexcltask(taskmgr, maintask);
 

lib/isc/tests/task_test.c

@@ -37,7 +37,6 @@
 #include <isc/timer.h>
 #include <isc/util.h>
 
-#include "../task_p.h"
 #include "isctest.h"
 
 /* Set to true (or use -v option) for verbose output */
@@ -120,6 +119,8 @@ set(isc_task_t *task, isc_event_t *event) {
 	atomic_store(value, atomic_fetch_add(&counter, 1));
 }
 
+#include <isc/thread.h>
+
 static void
 set_and_drop(isc_task_t *task, isc_event_t *event) {
 	atomic_int_fast32_t *value = (atomic_int_fast32_t *)event->ev_arg;
@@ -128,8 +129,7 @@ set_and_drop(isc_task_t *task, isc_event_t *event) {
 
 	isc_event_free(&event);
 	LOCK(&lock);
-	atomic_store(value, (int)isc_taskmgr_mode(taskmgr));
-	atomic_fetch_add(&counter, 1);
+	atomic_store(value, atomic_fetch_add(&counter, 1));
 	UNLOCK(&lock);
 }
 
@@ -204,17 +204,17 @@ privileged_events(void **state) {
 	UNUSED(state);
 
 	atomic_init(&counter, 1);
-	atomic_init(&a, 0);
-	atomic_init(&b, 0);
-	atomic_init(&c, 0);
-	atomic_init(&d, 0);
-	atomic_init(&e, 0);
+	atomic_init(&a, -1);
+	atomic_init(&b, -1);
+	atomic_init(&c, -1);
+	atomic_init(&d, -1);
+	atomic_init(&e, -1);
 
 	/*
-	 * Pause the task manager so we can fill up the work queue
-	 * without things happening while we do it.
+	 * Pause the net/task manager so we can fill up the work
+	 * queue without things happening while we do it.
 	 */
-	isc__taskmgr_pause(taskmgr);
+	isc_nm_pause(netmgr);
 
 	result = isc_task_create(taskmgr, 0, &task1);
 	assert_int_equal(result, ISC_R_SUCCESS);
@@ -233,7 +233,7 @@ privileged_events(void **state) {
 				   &a, sizeof(isc_event_t));
 	assert_non_null(event);
 
-	assert_int_equal(atomic_load(&a), 0);
+	assert_int_equal(atomic_load(&a), -1);
 	isc_task_send(task1, &event);
 
 	/* Second event: not privileged */
@@ -241,7 +241,7 @@ privileged_events(void **state) {
 				   &b, sizeof(isc_event_t));
 	assert_non_null(event);
 
-	assert_int_equal(atomic_load(&b), 0);
+	assert_int_equal(atomic_load(&b), -1);
 	isc_task_send(task2, &event);
 
 	/* Third event: privileged */
@@ -249,7 +249,7 @@ privileged_events(void **state) {
 				   &c, sizeof(isc_event_t));
 	assert_non_null(event);
 
-	assert_int_equal(atomic_load(&c), 0);
+	assert_int_equal(atomic_load(&c), -1);
 	isc_task_send(task1, &event);
 
 	/* Fourth event: privileged */
@@ -257,7 +257,7 @@ privileged_events(void **state) {
 				   &d, sizeof(isc_event_t));
 	assert_non_null(event);
 
-	assert_int_equal(atomic_load(&d), 0);
+	assert_int_equal(atomic_load(&d), -1);
 	isc_task_send(task1, &event);
 
 	/* Fifth event: not privileged */
@@ -265,19 +265,15 @@ privileged_events(void **state) {
 				   &e, sizeof(isc_event_t));
 	assert_non_null(event);
 
-	assert_int_equal(atomic_load(&e), 0);
+	assert_int_equal(atomic_load(&e), -1);
 	isc_task_send(task2, &event);
 
-	assert_int_equal(isc_taskmgr_mode(taskmgr), isc_taskmgrmode_normal);
-	isc_taskmgr_setprivilegedmode(taskmgr);
-	assert_int_equal(isc_taskmgr_mode(taskmgr), isc_taskmgrmode_privileged);
-
-	isc__taskmgr_resume(taskmgr);
+	isc_nm_resume(netmgr);
 
 	/* We're waiting for *all* variables to be set */
-	while ((atomic_load(&a) == 0 || atomic_load(&b) == 0 ||
-		atomic_load(&c) == 0 || atomic_load(&d) == 0 ||
-		atomic_load(&e) == 0) &&
+	while ((atomic_load(&a) < 0 || atomic_load(&b) < 0 ||
+		atomic_load(&c) < 0 || atomic_load(&d) < 0 ||
+		atomic_load(&e) < 0) &&
 	       i++ < 5000)
 	{
 		isc_test_nap(1000);
@@ -293,16 +289,14 @@ privileged_events(void **state) {
 	assert_true(atomic_load(&d) <= 3);
 
 	/* ...and the non-privileged tasks that set b and e, last */
-	assert_true(atomic_load(&b) >= 4);
-	assert_true(atomic_load(&e) >= 4);
+	assert_true(atomic_load(&b) > 3);
+	assert_true(atomic_load(&e) > 3);
 
 	assert_int_equal(atomic_load(&counter), 6);
 
 	isc_task_setprivilege(task1, false);
 	assert_false(isc_task_privilege(task1));
 
-	assert_int_equal(isc_taskmgr_mode(taskmgr), isc_taskmgrmode_normal);
-
 	isc_task_destroy(&task1);
 	assert_null(task1);
 	isc_task_destroy(&task2);
@@ -331,10 +325,10 @@ privilege_drop(void **state) {
 	atomic_init(&e, -1);
 
 	/*
-	 * Pause the task manager so we can fill up the work queue
+	 * Pause the net/task manager so we can fill up the work queue
 	 * without things happening while we do it.
 	 */
-	isc__taskmgr_pause(taskmgr);
+	isc_nm_pause(netmgr);
 
 	result = isc_task_create(taskmgr, 0, &task1);
 	assert_int_equal(result, ISC_R_SUCCESS);
@@ -388,11 +382,7 @@ privilege_drop(void **state) {
 	assert_int_equal(atomic_load(&e), -1);
 	isc_task_send(task2, &event);
 
-	assert_int_equal(isc_taskmgr_mode(taskmgr), isc_taskmgrmode_normal);
-	isc_taskmgr_setprivilegedmode(taskmgr);
-	assert_int_equal(isc_taskmgr_mode(taskmgr), isc_taskmgrmode_privileged);
-
-	isc__taskmgr_resume(taskmgr);
+	isc_nm_resume(netmgr);
 
 	/* We're waiting for all variables to be set. */
 	while ((atomic_load(&a) == -1 || atomic_load(&b) == -1 ||
@@ -407,19 +397,17 @@ privilege_drop(void **state) {
 	 * We need to check that all privilege mode events were fired
 	 * in privileged mode, and non privileged in non-privileged.
 	 */
-	assert_true(atomic_load(&a) == isc_taskmgrmode_privileged ||
-		    atomic_load(&c) == isc_taskmgrmode_privileged ||
-		    atomic_load(&d) == isc_taskmgrmode_privileged);
+	assert_true(atomic_load(&a) <= 3);
+	assert_true(atomic_load(&c) <= 3);
+	assert_true(atomic_load(&d) <= 3);
 
 	/* ...and neither of the non-privileged tasks did... */
-	assert_true(atomic_load(&b) == isc_taskmgrmode_normal ||
-		    atomic_load(&e) == isc_taskmgrmode_normal);
+	assert_true(atomic_load(&b) > 3);
+	assert_true(atomic_load(&e) > 3);
 
 	/* ...but all five of them did run. */
 	assert_int_equal(atomic_load(&counter), 6);
 
-	assert_int_equal(isc_taskmgr_mode(taskmgr), isc_taskmgrmode_normal);
-
 	isc_task_destroy(&task1);
 	assert_null(task1);
 	isc_task_destroy(&task2);
@@ -695,6 +683,7 @@ exclusive_cb(isc_task_t *task, isc_event_t *event) {
 	if (atomic_load(&done)) {
 		isc_mem_put(event->ev_destroy_arg, event->ev_arg, sizeof(int));
 		isc_event_free(&event);
+		atomic_fetch_sub(&counter, 1);
 	} else {
 		isc_task_send(task, &event);
 	}
@@ -708,6 +697,8 @@ task_exclusive(void **state) {
 
 	UNUSED(state);
 
+	atomic_init(&counter, 0);
+
 	for (i = 0; i < 10; i++) {
 		isc_event_t *event = NULL;
 		int *v;
@@ -732,11 +723,16 @@ task_exclusive(void **state) {
 		assert_non_null(event);
 
 		isc_task_send(tasks[i], &event);
+		atomic_fetch_add(&counter, 1);
 	}
 
 	for (i = 0; i < 10; i++) {
 		isc_task_detach(&tasks[i]);
 	}
+
+	while (atomic_load(&counter) > 0) {
+		isc_test_nap(1000);
+	}
 }
 
 /*
@@ -805,7 +801,8 @@ manytasks(void **state) {
 	isc_mem_debugging = ISC_MEM_DEBUGRECORD;
 	isc_mem_create(&mctx);
 
-	result = isc_taskmgr_create(mctx, 4, 0, NULL, &taskmgr);
+	netmgr = isc_nm_start(mctx, 4);
+	result = isc_taskmgr_create(mctx, 0, netmgr, &taskmgr);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
 	atomic_init(&done, false);
@@ -822,6 +819,7 @@ manytasks(void **state) {
 	UNLOCK(&lock);
 
 	isc_taskmgr_destroy(&taskmgr);
+	isc_nm_destroy(&netmgr);
 	isc_mem_destroy(&mctx);
 	isc_condition_destroy(&cv);
 	isc_mutex_destroy(&lock);

lib/isc/tests/taskpool_test.c

@@ -28,6 +28,9 @@
 
 #include "isctest.h"
 
+#define TASK_MAGIC    ISC_MAGIC('T', 'A', 'S', 'K')
+#define VALID_TASK(t) ISC_MAGIC_VALID(t, TASK_MAGIC)
+
 static int
 _setup(void **state) {
 	isc_result_t result;
@@ -57,7 +60,7 @@ create_pool(void **state) {
 
 	UNUSED(state);
 
-	result = isc_taskpool_create(taskmgr, test_mctx, 8, 2, &pool);
+	result = isc_taskpool_create(taskmgr, test_mctx, 8, 2, false, &pool);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool), 8);
 
@@ -73,13 +76,13 @@ expand_pool(void **state) {
 
 	UNUSED(state);
 
-	result = isc_taskpool_create(taskmgr, test_mctx, 10, 2, &pool1);
+	result = isc_taskpool_create(taskmgr, test_mctx, 10, 2, false, &pool1);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool1), 10);
 
 	/* resizing to a smaller size should have no effect */
 	hold = pool1;
-	result = isc_taskpool_expand(&pool1, 5, &pool2);
+	result = isc_taskpool_expand(&pool1, 5, false, &pool2);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool2), 10);
 	assert_ptr_equal(pool2, hold);
@@ -89,7 +92,7 @@ expand_pool(void **state) {
 
 	/* resizing to the same size should have no effect */
 	hold = pool1;
-	result = isc_taskpool_expand(&pool1, 10, &pool2);
+	result = isc_taskpool_expand(&pool1, 10, false, &pool2);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool2), 10);
 	assert_ptr_equal(pool2, hold);
@@ -99,7 +102,7 @@ expand_pool(void **state) {
 
 	/* resizing to larger size should make a new pool */
 	hold = pool1;
-	result = isc_taskpool_expand(&pool1, 20, &pool2);
+	result = isc_taskpool_expand(&pool1, 20, false, &pool2);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool2), 20);
 	assert_ptr_not_equal(pool2, hold);
@@ -118,19 +121,19 @@ get_tasks(void **state) {
 
 	UNUSED(state);
 
-	result = isc_taskpool_create(taskmgr, test_mctx, 2, 2, &pool);
+	result = isc_taskpool_create(taskmgr, test_mctx, 2, 2, false, &pool);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool), 2);
 
 	/* two tasks in pool; make sure we can access them more than twice */
 	isc_taskpool_gettask(pool, &task1);
-	assert_non_null(task1);
+	assert_true(VALID_TASK(task1));
 
 	isc_taskpool_gettask(pool, &task2);
-	assert_non_null(task2);
+	assert_true(VALID_TASK(task2));
 
 	isc_taskpool_gettask(pool, &task3);
-	assert_non_null(task3);
+	assert_true(VALID_TASK(task3));
 
 	isc_task_destroy(&task1);
 	isc_task_destroy(&task2);
@@ -149,30 +152,22 @@ set_privilege(void **state) {
 
 	UNUSED(state);
 
-	result = isc_taskpool_create(taskmgr, test_mctx, 2, 2, &pool);
+	result = isc_taskpool_create(taskmgr, test_mctx, 2, 2, true, &pool);
 	assert_int_equal(result, ISC_R_SUCCESS);
 	assert_int_equal(isc_taskpool_size(pool), 2);
 
-	isc_taskpool_setprivilege(pool, true);
-
 	isc_taskpool_gettask(pool, &task1);
 	isc_taskpool_gettask(pool, &task2);
 	isc_taskpool_gettask(pool, &task3);
 
-	assert_non_null(task1);
-	assert_non_null(task2);
-	assert_non_null(task3);
+	assert_true(VALID_TASK(task1));
+	assert_true(VALID_TASK(task2));
+	assert_true(VALID_TASK(task3));
 
 	assert_true(isc_task_privilege(task1));
 	assert_true(isc_task_privilege(task2));
 	assert_true(isc_task_privilege(task3));
 
-	isc_taskpool_setprivilege(pool, false);
-
-	assert_false(isc_task_privilege(task1));
-	assert_false(isc_task_privilege(task2));
-	assert_false(isc_task_privilege(task3));
-
 	isc_task_destroy(&task1);
 	isc_task_destroy(&task2);
 	isc_task_destroy(&task3);

lib/isc/tests/timer_test.c

@@ -435,8 +435,8 @@ reset(void **state) {
 	setup_test(isc_timertype_ticker, &expires, &interval, test_reset);
 }
 
-static int startflag;
-static int shutdownflag;
+static atomic_bool startflag;
+static atomic_bool shutdownflag;
 static isc_timer_t *tickertimer = NULL;
 static isc_timer_t *oncetimer = NULL;
 static isc_task_t *task1 = NULL;
@@ -447,23 +447,6 @@ static isc_task_t *task2 = NULL;
  * in its queue, until signaled by task2.
  */
 
-static void
-start_event(isc_task_t *task, isc_event_t *event) {
-	UNUSED(task);
-
-	if (verbose) {
-		print_message("# start_event\n");
-	}
-
-	LOCK(&mx);
-	while (!startflag) {
-		(void)isc_condition_wait(&cv, &mx);
-	}
-	UNLOCK(&mx);
-
-	isc_event_free(&event);
-}
-
 static void
 tick_event(isc_task_t *task, isc_event_t *event) {
 	isc_result_t result;
@@ -472,6 +455,14 @@ tick_event(isc_task_t *task, isc_event_t *event) {
 
 	UNUSED(task);
 
+	if (!atomic_load(&startflag)) {
+		if (verbose) {
+			print_message("# tick_event %d\n", -1);
+		}
+		isc_event_free(&event);
+		return;
+	}
+
 	int tick = atomic_fetch_add(&eventcnt, 1);
 	if (verbose) {
 		print_message("# tick_event %d\n", tick);
@@ -496,8 +487,6 @@ tick_event(isc_task_t *task, isc_event_t *event) {
 
 static void
 once_event(isc_task_t *task, isc_event_t *event) {
-	isc_result_t result;
-
 	if (verbose) {
 		print_message("# once_event\n");
 	}
@@ -505,12 +494,7 @@ once_event(isc_task_t *task, isc_event_t *event) {
 	/*
 	 * Allow task1 to start processing events.
 	 */
-	LOCK(&mx);
-	startflag = 1;
-
-	result = isc_condition_broadcast(&cv);
-	subthread_assert_result_equal(result, ISC_R_SUCCESS);
-	UNLOCK(&mx);
+	atomic_store(&startflag, true);
 
 	isc_event_free(&event);
 	isc_task_shutdown(task);
@@ -518,8 +502,6 @@ once_event(isc_task_t *task, isc_event_t *event) {
 
 static void
 shutdown_purge(isc_task_t *task, isc_event_t *event) {
-	isc_result_t result;
-
 	UNUSED(task);
 	UNUSED(event);
 
@@ -530,12 +512,7 @@ shutdown_purge(isc_task_t *task, isc_event_t *event) {
 	/*
 	 * Signal shutdown processing complete.
 	 */
-	LOCK(&mx);
-	shutdownflag = 1;
-
-	result = isc_condition_signal(&cv);
-	subthread_assert_result_equal(result, ISC_R_SUCCESS);
-	UNLOCK(&mx);
+	atomic_store(&shutdownflag, 1);
 
 	isc_event_free(&event);
 }
@@ -544,22 +521,17 @@ shutdown_purge(isc_task_t *task, isc_event_t *event) {
 static void
 purge(void **state) {
 	isc_result_t result;
-	isc_event_t *event = NULL;
 	isc_time_t expires;
 	isc_interval_t interval;
 
 	UNUSED(state);
 
-	startflag = 0;
-	shutdownflag = 0;
+	atomic_init(&startflag, 0);
+	atomic_init(&shutdownflag, 0);
 	atomic_init(&eventcnt, 0);
 	seconds = 1;
 	nanoseconds = 0;
 
-	isc_mutex_init(&mx);
-
-	isc_condition_init(&cv);
-
 	result = isc_task_create(taskmgr, 0, &task1);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
@@ -569,13 +541,6 @@ purge(void **state) {
 	result = isc_task_create(taskmgr, 0, &task2);
 	assert_int_equal(result, ISC_R_SUCCESS);
 
-	LOCK(&mx);
-
-	event = isc_event_allocate(test_mctx, (void *)1, (isc_eventtype_t)1,
-				   start_event, NULL, sizeof(*event));
-	assert_non_null(event);
-	isc_task_send(task1, &event);
-
 	isc_time_settoepoch(&expires);
 	isc_interval_set(&interval, seconds, 0);
 
@@ -600,13 +565,10 @@ purge(void **state) {
 	/*
 	 * Wait for shutdown processing to complete.
 	 */
-	while (!shutdownflag) {
-		result = isc_condition_wait(&cv, &mx);
-		assert_int_equal(result, ISC_R_SUCCESS);
+	while (!atomic_load(&shutdownflag)) {
+		isc_test_nap(1000);
 	}
 
-	UNLOCK(&mx);
-
 	assert_int_equal(atomic_load(&errcnt), ISC_R_SUCCESS);
 
 	assert_int_equal(atomic_load(&eventcnt), 1);
@@ -615,7 +577,6 @@ purge(void **state) {
 	isc_timer_detach(&oncetimer);
 	isc_task_destroy(&task1);
 	isc_task_destroy(&task2);
-	isc_mutex_destroy(&mx);
 }
 
 int