From: Ondřej Surý Date: Mon, 9 May 2022 10:58:34 +0000 (+0200) Subject: Remove isc_task_destroy() and isc_task_shutdown() X-Git-Tag: v9.19.2~39^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=0582478c96f4a83ac169fc3b6c90d33bf2aa1eb4;p=thirdparty%2Fbind9.git Remove isc_task_destroy() and isc_task_shutdown() After removing the isc_task_onshutdown(), the isc_task_shutdown() and isc_task_destroy() became obsolete. Remove calls to isc_task_shutdown() and replace the calls to isc_task_destroy() with isc_task_detach(). Simplify the internal logic to destroy the task when the last reference is removed. --- diff --git a/bin/tests/system/pipelined/pipequeries.c b/bin/tests/system/pipelined/pipequeries.c index 3291fcf0d3b..b8e3e3ffec8 100644 --- a/bin/tests/system/pipelined/pipequeries.c +++ b/bin/tests/system/pipelined/pipequeries.c @@ -286,7 +286,6 @@ main(int argc, char *argv[]) { dns_dispatch_detach(&dispatchv4); dns_dispatchmgr_detach(&dispatchmgr); - isc_task_shutdown(task); isc_task_detach(&task); isc_managers_destroy(&netmgr, &taskmgr, NULL); diff --git a/bin/tests/system/tkey/keycreate.c b/bin/tests/system/tkey/keycreate.c index b2c61e23e2f..d03dffc5d50 100644 --- a/bin/tests/system/tkey/keycreate.c +++ b/bin/tests/system/tkey/keycreate.c @@ -254,7 +254,6 @@ main(int argc, char *argv[]) { dns_requestmgr_detach(&requestmgr); dns_dispatch_detach(&dispatchv4); dns_dispatchmgr_detach(&dispatchmgr); - isc_task_shutdown(task); isc_task_detach(&task); isc_managers_destroy(&netmgr, &taskmgr, NULL); diff --git a/bin/tests/system/tkey/keydelete.c b/bin/tests/system/tkey/keydelete.c index b563ebcb44d..4d328655103 100644 --- a/bin/tests/system/tkey/keydelete.c +++ b/bin/tests/system/tkey/keydelete.c @@ -197,7 +197,6 @@ main(int argc, char **argv) { dns_requestmgr_detach(&requestmgr); dns_dispatch_detach(&dispatchv4); dns_dispatchmgr_detach(&dispatchmgr); - isc_task_shutdown(task); isc_task_detach(&task); isc_managers_destroy(&netmgr, &taskmgr, NULL); diff --git a/bin/tools/mdig.c b/bin/tools/mdig.c index e2adf43235e..17221700e7d 100644 --- a/bin/tools/mdig.c +++ b/bin/tools/mdig.c @@ -2217,7 +2217,6 @@ main(int argc, char *argv[]) { dns_dispatch_detach(&dispatchvx); dns_dispatchmgr_detach(&dispatchmgr); - isc_task_shutdown(task); isc_task_detach(&task); isc_managers_destroy(&netmgr, &taskmgr, NULL); diff --git a/doc/design/tasks b/doc/design/tasks index ef616dc7f16..d10787b2f17 100644 --- a/doc/design/tasks +++ b/doc/design/tasks @@ -14,23 +14,8 @@ information regarding copyright ownership. Changes I made last week to the task code simplified the task shutdown and termination model. Here's how things now work: -When a task is shutdown: - - The "shutting down" attribute of the task is set - -Task shutdown can be initiated explicitly, via a call to isc_task_shutdown(), -or implicitly, when the following conditions occur: - - The "shutting down" attribute of the task is not set - - The task has no references - - The task has an empty event queue - Task termination occurs when: - The "shutting down" attribute of the task is set - The task has no references The task has an empty event queue diff --git a/lib/dns/cache.c b/lib/dns/cache.c index 22b2c9deeb0..c7fa21d94f7 100644 --- a/lib/dns/cache.c +++ b/lib/dns/cache.c @@ -416,7 +416,6 @@ dns_cache_detach(dns_cache_t **cachep) { cleaner_shutdown_action, &cache->cleaner, sizeof(*event)); isc_task_send(cache->cleaner.task, &event); - isc_task_shutdown(cache->cleaner.task); } else { cache_free(cache); } diff --git a/lib/dns/catz.c b/lib/dns/catz.c index cd76eaa179b..e14880bfd03 100644 --- a/lib/dns/catz.c +++ b/lib/dns/catz.c @@ -965,7 +965,7 @@ dns_catz_catzs_detach(dns_catz_zones_t **catzsp) { if (isc_refcount_decrement(&catzs->refs) == 1) { catzs->magic = 0; - isc_task_destroy(&catzs->updater); + isc_task_detach(&catzs->updater); isc_mutex_destroy(&catzs->lock); if (catzs->zones != NULL) { isc_ht_iter_t *iter = NULL; diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 3514ad722b4..2baa32cf51d 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -10106,7 +10106,6 @@ destroy(dns_resolver_t *res) { isc_mutex_destroy(&res->lock); for (i = 0; i < res->nbuckets; i++) { INSIST(ISC_LIST_EMPTY(res->buckets[i].fctxs)); - isc_task_shutdown(res->buckets[i].task); isc_task_detach(&res->buckets[i].task); isc_mutex_destroy(&res->buckets[i].lock); } @@ -10346,7 +10345,6 @@ cleanup_primelock: cleanup_buckets: for (size_t i = 0; i < ntasks; i++) { isc_mutex_destroy(&res->buckets[i].lock); - isc_task_shutdown(res->buckets[i].task); isc_task_detach(&res->buckets[i].task); } isc_mem_put(view->mctx, res->buckets, diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index 4cdb2eb1ce3..d76bd53683d 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -2145,7 +2145,7 @@ rpz_destroy_rpzs(dns_rpz_zones_t *rpzs) { if (rpzs->rbt != NULL) { dns_rbt_destroy(&rpzs->rbt); } - isc_task_destroy(&rpzs->updater); + isc_task_detach(&rpzs->updater); isc_mutex_destroy(&rpzs->maint_lock); isc_rwlock_destroy(&rpzs->search_lock); isc_refcount_destroy(&rpzs->refs); diff --git a/lib/dns/tests/dnstest.c b/lib/dns/tests/dnstest.c index 7da8c3cc835..1ec9adfd1c1 100644 --- a/lib/dns/tests/dnstest.c +++ b/lib/dns/tests/dnstest.c @@ -95,8 +95,7 @@ static isc_logcategory_t categories[] = { { "", 0 }, static void cleanup_managers(void) { if (maintask != NULL) { - isc_task_shutdown(maintask); - isc_task_destroy(&maintask); + isc_task_detach(&maintask); } isc_managers_destroy(netmgr == NULL ? NULL : &netmgr, diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 2c011c0241a..4293516d0f9 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -19104,7 +19104,7 @@ dns_zonemgr_shutdown(dns_zonemgr_t *zmgr) { isc_ratelimiter_shutdown(zmgr->startuprefreshrl); if (zmgr->task != NULL) { - isc_task_destroy(&zmgr->task); + isc_task_detach(&zmgr->task); } for (size_t i = 0; i < zmgr->workers; i++) { diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index 22ce1a6a255..703e7bff7f2 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -266,53 +266,6 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event); * or was marked unpurgeable. */ -void -isc_task_shutdown(isc_task_t *task); -/*%< - * Shutdown 'task'. - * - * Notes: - * - *\li The task moves into a "shutting down" mode. - * - *\li Trying to shutdown a task that has already been shutdown has no - * effect. - * - * Requires: - * - *\li 'task' is a valid task. - * - */ - -void -isc_task_destroy(isc_task_t **taskp); -/*%< - * Destroy '*taskp'. - * - * Notes: - * - *\li This call is equivalent to: - * - *\code - * isc_task_shutdown(*taskp); - * isc_task_detach(taskp); - *\endcode - * - * Requires: - * - * '*taskp' is a valid task. - * - * Ensures: - * - *\li Any shutdown events requested with isc_task_onshutdown() have been - * posted (in LIFO order). - * - *\li *taskp == NULL - * - *\li If '*taskp' is the last reference to the task, - * all resources used by the task will be freed. - */ - void isc_task_setname(isc_task_t *task, const char *name, void *tag); /*%< @@ -404,16 +357,6 @@ isc_task_endexclusive(isc_task_t *task); * exclusive access by calling isc_task_spl(). */ -bool -isc_task_exiting(isc_task_t *t); -/*%< - * Returns true if the task is in the process of shutting down, - * false otherwise. - * - * Requires: - *\li 'task' is a valid task. - */ - /***** ***** Task Manager. *****/ diff --git a/lib/isc/task.c b/lib/isc/task.c index 8e5d88ce1c9..75d42198c51 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -111,7 +111,6 @@ struct isc_task { int tid; task_state_t state; isc_refcount_t references; - isc_refcount_t running; isc_eventlist_t events; unsigned int nevents; unsigned int quantum; @@ -133,8 +132,6 @@ struct isc_task { LINK(isc_task_t) link; }; -#define TASK_SHUTTINGDOWN(t) (atomic_load_acquire(&(t)->shuttingdown)) - #define TASK_MANAGER_MAGIC ISC_MAGIC('T', 'S', 'K', 'M') #define VALID_MANAGER(m) ISC_MAGIC_VALID(m, TASK_MANAGER_MAGIC) @@ -177,7 +174,7 @@ isc_taskmgr_excltask(isc_taskmgr_t *mgr, isc_task_t **taskp); ***/ static void -task_finished(isc_task_t *task) { +task_destroy(isc_task_t *task) { isc_taskmgr_t *manager = task->manager; isc_mem_t *mctx = manager->mctx; REQUIRE(EMPTY(task->events)); @@ -186,7 +183,6 @@ task_finished(isc_task_t *task) { XTRACE("task_finished"); - isc_refcount_destroy(&task->running); isc_refcount_destroy(&task->references); LOCK(&manager->lock); @@ -245,7 +241,6 @@ isc__task_create_bound(isc_taskmgr_t *manager, unsigned int quantum, task->state = task_state_idle; isc_refcount_init(&task->references, 1); - isc_refcount_init(&task->running, 0); INIT_LIST(task->events); task->nevents = 0; task->quantum = (quantum > 0) ? quantum : manager->default_quantum; @@ -266,7 +261,6 @@ isc__task_create_bound(isc_taskmgr_t *manager, unsigned int quantum, UNLOCK(&manager->lock); if (exiting) { - isc_refcount_destroy(&task->running); isc_refcount_decrement(&task->references); isc_refcount_destroy(&task->references); isc_mutex_destroy(&task->lock); @@ -296,31 +290,6 @@ isc_task_attach(isc_task_t *source, isc_task_t **targetp) { *targetp = source; } -static bool -task_shutdown(isc_task_t *task) { - bool was_idle = false; - - /* - * Caller must be holding the task's lock. - */ - - XTRACE("task_shutdown"); - - if (atomic_compare_exchange_strong(&task->shuttingdown, - &(bool){ false }, true)) { - XTRACE("shutting down"); - if (task->state == task_state_idle) { - INSIST(EMPTY(task->events)); - task->state = task_state_ready; - was_idle = true; - } - INSIST(task->state == task_state_ready || - task->state == task_state_running); - } - - return (was_idle); -} - /* * Moves a task onto the appropriate run queue. * @@ -333,7 +302,7 @@ task_ready(isc_task_t *task) { XTRACE("task_ready"); - isc_refcount_increment0(&task->running); + isc_task_attach(task, &(isc_task_t *){ NULL }); LOCK(&task->lock); if (task->tid < 0) { task->tid = (int)isc_random_uniform(manager->nworkers); @@ -347,57 +316,25 @@ isc_task_ready(isc_task_t *task) { task_ready(task); } -static bool -task_detach(isc_task_t *task) { - /* - * Caller must be holding the task lock. - */ - - XTRACE("detach"); - - if (isc_refcount_decrement(&task->references) == 1 && - task->state == task_state_idle) - { - INSIST(EMPTY(task->events)); - /* - * There are no references to this task, and no - * pending events. We could try to optimize and - * either initiate shutdown or clean up the task, - * depending on its state, but it's easier to just - * make the task ready and allow run() or the event - * loop to deal with shutting down and termination. - */ - task->state = task_state_ready; - return (true); - } - - return (false); -} - void isc_task_detach(isc_task_t **taskp) { isc_task_t *task; - bool was_idle; - - /* - * Detach *taskp from its task. - */ REQUIRE(taskp != NULL); + REQUIRE(VALID_TASK(*taskp)); + task = *taskp; - REQUIRE(VALID_TASK(task)); + *taskp = NULL; XTRACE("isc_task_detach"); - LOCK(&task->lock); - was_idle = task_detach(task); - UNLOCK(&task->lock); + if (isc_refcount_decrement(&task->references) == 1) { + LOCK(&task->lock); + task->state = task_state_done; + UNLOCK(&task->lock); - if (was_idle) { - task_ready(task); + task_destroy(task); } - - *taskp = NULL; } static bool @@ -478,36 +415,16 @@ isc_task_send(isc_task_t *task, isc_event_t **eventp) { void isc_task_sendanddetach(isc_task_t **taskp, isc_event_t **eventp) { - bool idle1, idle2; isc_task_t *task; - /* - * Send '*event' to '*taskp' and then detach '*taskp' from its - * task. - */ - REQUIRE(taskp != NULL); task = *taskp; + *taskp = NULL; REQUIRE(VALID_TASK(task)); XTRACE("isc_task_sendanddetach"); - LOCK(&task->lock); - idle1 = task_send(task, eventp); - idle2 = task_detach(task); - UNLOCK(&task->lock); - - /* - * If idle1, then idle2 shouldn't be true as well since we're holding - * the task lock, and thus the task cannot switch from ready back to - * idle. - */ - INSIST(!(idle1 && idle2)); - - if (idle1 || idle2) { - task_ready(task); - } - - *taskp = NULL; + isc_task_send(task, eventp); + isc_task_detach(&task); } bool @@ -547,37 +464,6 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { return (true); } -void -isc_task_shutdown(isc_task_t *task) { - bool was_idle; - - /* - * Shutdown 'task'. - */ - - REQUIRE(VALID_TASK(task)); - - LOCK(&task->lock); - was_idle = task_shutdown(task); - UNLOCK(&task->lock); - - if (was_idle) { - task_ready(task); - } -} - -void -isc_task_destroy(isc_task_t **taskp) { - /* - * Destroy '*taskp'. - */ - - REQUIRE(taskp != NULL); - - isc_task_shutdown(*taskp); - isc_task_detach(taskp); -} - void isc_task_setname(isc_task_t *task, const char *name, void *tag) { /* @@ -630,7 +516,6 @@ isc_task_setquantum(isc_task_t *task, unsigned int quantum) { static isc_result_t task_run(isc_task_t *task) { unsigned int dispatch_count = 0; - bool finished = false; isc_event_t *event = NULL; isc_result_t result = ISC_R_SUCCESS; uint32_t quantum; @@ -671,37 +556,12 @@ task_run(isc_task_t *task) { dispatch_count++; } - if (isc_refcount_current(&task->references) == 0 && - EMPTY(task->events) && !TASK_SHUTTINGDOWN(task)) - { - /* - * There are no references and no pending events for - * this task, which means it will not become runnable - * again via an external action (such as sending an - * event or detaching). - * - * We initiate shutdown to prevent it from becoming a - * zombie. - * - * We do this here instead of in the "if - * EMPTY(task->events)" block below because: - * - * If we post no shutdown events, we want the task - * to finish. - * - * If we did post shutdown events, will still want - * the task's quantum to be applied. - */ - INSIST(!task_shutdown(task)); - } - if (EMPTY(task->events)) { /* * Nothing else to do for this task right now. */ XTRACE("empty"); - if (isc_refcount_current(&task->references) == 0 && - TASK_SHUTTINGDOWN(task)) { + if (isc_refcount_current(&task->references) == 0) { /* * The task is done. */ @@ -728,16 +588,8 @@ task_run(isc_task_t *task) { } done: - if (isc_refcount_decrement(&task->running) == 1 && - task->state == task_state_done) - { - finished = true; - } UNLOCK(&task->lock); - - if (finished) { - task_finished(task); - } + isc_task_detach(&task); return (result); } @@ -820,7 +672,7 @@ isc__taskmgr_create(isc_mem_t *mctx, unsigned int default_quantum, isc_nm_t *nm, void isc__taskmgr_shutdown(isc_taskmgr_t *manager) { - isc_task_t *task; + isc_task_t *task = NULL; REQUIRE(VALID_MANAGER(manager)); @@ -832,18 +684,10 @@ isc__taskmgr_shutdown(isc_taskmgr_t *manager) { * isc_taskmgr_destroy(), e.g. by signalling a condition variable * that the startup thread is sleeping on. */ - - /* - * Unlike elsewhere, we're going to hold this lock a long time. - * We need to do so, because otherwise the list of tasks could - * change while we were traversing it. - * - * This is also the only function where we will hold both the - * task manager lock and a task lock at the same time. - */ LOCK(&manager->lock); if (manager->excl != NULL) { - isc_task_detach((isc_task_t **)&manager->excl); + task = manager->excl; + manager->excl = NULL; } /* @@ -852,24 +696,11 @@ isc__taskmgr_shutdown(isc_taskmgr_t *manager) { INSIST(manager->exiting == false); manager->exiting = true; - /* - * Post shutdown event(s) to every task (if they haven't already been - * posted). - */ - for (task = HEAD(manager->tasks); task != NULL; task = NEXT(task, link)) - { - bool was_idle; - - LOCK(&task->lock); - was_idle = task_shutdown(task); - UNLOCK(&task->lock); + UNLOCK(&manager->lock); - if (was_idle) { - task_ready(task); - } + if (task != NULL) { + isc_task_detach(&task); } - - UNLOCK(&manager->lock); } void @@ -999,13 +830,6 @@ isc_task_endexclusive(isc_task_t *task) { &(bool){ true }, false)); } -bool -isc_task_exiting(isc_task_t *task) { - REQUIRE(VALID_TASK(task)); - - return (TASK_SHUTTINGDOWN(task)); -} - #ifdef HAVE_LIBXML2 #define TRY0(a) \ do { \ diff --git a/lib/isc/tests/isctest.c b/lib/isc/tests/isctest.c index 18b95a9c8cd..ad8fa2ad60a 100644 --- a/lib/isc/tests/isctest.c +++ b/lib/isc/tests/isctest.c @@ -55,8 +55,7 @@ static isc_logcategory_t categories[] = { { "", 0 }, static void cleanup_managers(void) { if (maintask != NULL) { - isc_task_shutdown(maintask); - isc_task_destroy(&maintask); + isc_task_detach(&maintask); } isc_managers_destroy(netmgr == NULL ? NULL : &netmgr, taskmgr == NULL ? NULL : &taskmgr, diff --git a/lib/isc/tests/task_test.c b/lib/isc/tests/task_test.c index d32e063b553..b19fb9d9850 100644 --- a/lib/isc/tests/task_test.c +++ b/lib/isc/tests/task_test.c @@ -134,7 +134,7 @@ create_task(void **state) { result = isc_task_create(taskmgr, 0, &task); assert_int_equal(result, ISC_R_SUCCESS); - isc_task_destroy(&task); + isc_task_detach(&task); assert_null(task); } @@ -178,7 +178,7 @@ all_events(void **state) { assert_int_not_equal(atomic_load(&a), 0); assert_int_not_equal(atomic_load(&b), 0); - isc_task_destroy(&task); + isc_task_detach(&task); assert_null(task); } @@ -442,9 +442,6 @@ manytasks(void **state) { (unsigned long)ntasks); } - isc_mutex_init(&lock); - isc_condition_init(&cv); - atomic_init(&done, false); event = isc_event_allocate(test_mctx, NULL, 1, maxtask_cb, @@ -457,9 +454,6 @@ manytasks(void **state) { WAIT(&cv, &lock); } UNLOCK(&lock); - - isc_condition_destroy(&cv); - isc_mutex_destroy(&lock); } /* @@ -519,8 +513,6 @@ try_purgeevent(void) { atomic_init(&done, false); eventcnt = 0; - isc_condition_init(&cv); - result = isc_task_create(taskmgr, 0, &task); assert_int_equal(result, ISC_R_SUCCESS); @@ -551,8 +543,6 @@ try_purgeevent(void) { atomic_store(&started, true); SIGNAL(&cv); - isc_task_shutdown(task); - isc_interval_set(&interval, 5, 0); /* diff --git a/lib/isc/tests/timer_test.c b/lib/isc/tests/timer_test.c index 2ce80adc08e..685ce6973b3 100644 --- a/lib/isc/tests/timer_test.c +++ b/lib/isc/tests/timer_test.c @@ -529,8 +529,8 @@ purge(void **state) { isc_timer_destroy(&tickertimer); isc_timer_destroy(&oncetimer); - isc_task_destroy(&task1); - isc_task_destroy(&task2); + isc_task_detach(&task1); + isc_task_detach(&task2); } int diff --git a/lib/ns/tests/nstest.c b/lib/ns/tests/nstest.c index 0676009626c..b06bc027c8a 100644 --- a/lib/ns/tests/nstest.c +++ b/lib/ns/tests/nstest.c @@ -183,8 +183,7 @@ cleanup_managers(void) { mctx, NULL, ISC_TASKEVENT_TEST, shutdown_managers, NULL, sizeof(*event)); isc_task_send(maintask, &event); - isc_task_shutdown(maintask); - isc_task_destroy(&maintask); + isc_task_detach(&maintask); } while (atomic_load(&run_managers) && !atomic_load(&shutdown_done)) {