From 17aed2f8951d75bfb4ea8ff5fcfdb7f13dd0ce2d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 21:55:15 +0200 Subject: [PATCH 1/8] Repair isc_task_purgeevent(), clean isc_task_unsend{,range}() The isc_task_purgerange() was walking through all events on the task to find a matching task. Instead use the ISC_LINK_LINKED to find whether the event is active. Cleanup the related isc_task_unsend() and isc_task_unsendrange() functions that were not used anywhere. --- lib/isc/include/isc/task.h | 71 -------------------------------------- lib/isc/task.c | 46 ++++-------------------- 2 files changed, 7 insertions(+), 110 deletions(-) diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index e5251aa4be..efc50992c9 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -54,18 +54,6 @@ * * Purging calls isc_event_free() on the matching events. * - * Unsending returns a list of events that matched the pattern. - * The caller is then responsible for them. - * - * Consumers of events should purge, not unsend. - * - * Producers of events often want to remove events when the caller indicates - * it is no longer interested in the object, e.g. by canceling a timer. - * Sometimes this can be done by purging, but for some event types, the - * calls to isc_event_free() cause deadlock because the event free routine - * wants to acquire a lock the caller is already holding. Unsending instead - * of purging solves this problem. As a general rule, producers should only - * unsend events which they have sent. */ /*** @@ -337,65 +325,6 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event); * or was marked unpurgeable. */ -unsigned int -isc_task_unsendrange(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag, isc_eventlist_t *events); -/*%< - * Remove events from a task's event queue. - * - * Requires: - * - *\li 'task' is a valid task. - * - *\li last >= first. - * - *\li *events is a valid list. - * - * Ensures: - * - *\li Events in the event queue of 'task' whose sender is 'sender', whose - * type is >= first and <= last, and whose tag is 'tag' will be dequeued - * and appended to *events. - * - *\li A sender of NULL will match any sender. A NULL tag matches any - * tag. - * - * Returns: - * - *\li The number of events unsent. - */ - -unsigned int -isc_task_unsend(isc_task_t *task, void *sender, isc_eventtype_t type, void *tag, - isc_eventlist_t *events); -/*%< - * Remove events from a task's event queue. - * - * Notes: - * - *\li This function is equivalent to - * - *\code - * isc_task_unsendrange(task, sender, type, type, tag, events); - *\endcode - * - * Requires: - * - *\li 'task' is a valid task. - * - *\li *events is a valid list. - * - * Ensures: - * - *\li Events in the event queue of 'task' whose sender is 'sender', whose - * type is 'type', and whose tag is 'tag' will be dequeued and appended - * to *events. - * - * Returns: - * - *\li The number of events unsent. - */ - isc_result_t isc_task_onshutdown(isc_task_t *task, isc_taskaction_t action, void *arg); /*%< diff --git a/lib/isc/task.c b/lib/isc/task.c index 5d86c9d4d2..df1f4d565a 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -612,12 +612,10 @@ isc_task_purge(isc_task_t *task, void *sender, isc_eventtype_t type, bool isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { - isc_event_t *curr_event, *next_event; + bool found = false; /* * Purge 'event' from a task's event queue. - * - * XXXRTH: WARNING: This method may be removed before beta. */ REQUIRE(VALID_TASK(task)); @@ -633,52 +631,22 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { */ LOCK(&task->lock); - for (curr_event = HEAD(task->events); curr_event != NULL; - curr_event = next_event) - { - next_event = NEXT(curr_event, ev_link); - if (curr_event == event && PURGE_OK(event)) { - DEQUEUE(task->events, curr_event, ev_link); - task->nevents--; - break; - } + if (ISC_LINK_LINKED(event, ev_link)) { + DEQUEUE(task->events, event, ev_link); + task->nevents--; + found = true; } UNLOCK(&task->lock); - if (curr_event == NULL) { + if (!found) { return (false); } - isc_event_free(&curr_event); + isc_event_free(&event); return (true); } -unsigned int -isc_task_unsendrange(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag, isc_eventlist_t *events) { - /* - * Remove events from a task's event queue. - */ - REQUIRE(VALID_TASK(task)); - - XTRACE("isc_task_unsendrange"); - - return (dequeue_events(task, sender, first, last, tag, events, false)); -} - -unsigned int -isc_task_unsend(isc_task_t *task, void *sender, isc_eventtype_t type, void *tag, - isc_eventlist_t *events) { - /* - * Remove events from a task's event queue. - */ - - XTRACE("isc_task_unsend"); - - return (dequeue_events(task, sender, type, type, tag, events, false)); -} - isc_result_t isc_task_onshutdown(isc_task_t *task, isc_taskaction_t action, void *arg) { bool disallowed = false; From 48b2a5df97b3aaa15d610dc546f8f51ecf7cf1dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 21:59:57 +0200 Subject: [PATCH 2/8] Keep the list of scheduled events on the timer Instead of searching for the events to purge, keep the list of scheduled events on the timer list and purge the events that we have scheduled. --- lib/dns/include/dns/events.h | 3 -- lib/isc/include/isc/app.h | 4 +- lib/isc/include/isc/event.h | 3 -- lib/isc/include/isc/task.h | 6 +-- lib/isc/include/isc/timer.h | 13 ++++--- lib/isc/timer.c | 66 +++++++++++++++++++++++--------- lib/isccc/include/isccc/events.h | 3 -- 7 files changed, 57 insertions(+), 41 deletions(-) diff --git a/lib/dns/include/dns/events.h b/lib/dns/include/dns/events.h index d3d08e088a..a38effdf03 100644 --- a/lib/dns/include/dns/events.h +++ b/lib/dns/include/dns/events.h @@ -82,6 +82,3 @@ #define DNS_EVENT_TRYSTALE (ISC_EVENTCLASS_DNS + 59) #define DNS_EVENT_ZONEFLUSH (ISC_EVENTCLASS_DNS + 60) #define DNS_EVENT_CHECKDSSENDTOADDR (ISC_EVENTCLASS_DNS + 61) - -#define DNS_EVENT_FIRSTEVENT (ISC_EVENTCLASS_DNS + 0) -#define DNS_EVENT_LASTEVENT (ISC_EVENTCLASS_DNS + 65535) diff --git a/lib/isc/include/isc/app.h b/lib/isc/include/isc/app.h index 1a42bd0e27..5f5f04b08f 100644 --- a/lib/isc/include/isc/app.h +++ b/lib/isc/include/isc/app.h @@ -91,9 +91,7 @@ typedef isc_event_t isc_appevent_t; -#define ISC_APPEVENT_FIRSTEVENT (ISC_EVENTCLASS_APP + 0) -#define ISC_APPEVENT_SHUTDOWN (ISC_EVENTCLASS_APP + 1) -#define ISC_APPEVENT_LASTEVENT (ISC_EVENTCLASS_APP + 65535) +#define ISC_APPEVENT_SHUTDOWN (ISC_EVENTCLASS_APP + 0) ISC_LANG_BEGINDECLS diff --git a/lib/isc/include/isc/event.h b/lib/isc/include/isc/event.h index 811bcd8862..75a528e4e9 100644 --- a/lib/isc/include/isc/event.h +++ b/lib/isc/include/isc/event.h @@ -76,9 +76,6 @@ struct isc_event { ISC_EVENT_COMMON(struct isc_event); }; -#define ISC_EVENTTYPE_FIRSTEVENT 0x00000000 -#define ISC_EVENTTYPE_LASTEVENT 0xffffffff - #define ISC_EVENT_PTR(p) ((isc_event_t **)(void *)(p)) ISC_LANG_BEGINDECLS diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index efc50992c9..1d995083c7 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -68,10 +68,8 @@ #include #include -#define ISC_TASKEVENT_FIRSTEVENT (ISC_EVENTCLASS_TASK + 0) -#define ISC_TASKEVENT_SHUTDOWN (ISC_EVENTCLASS_TASK + 1) -#define ISC_TASKEVENT_TEST (ISC_EVENTCLASS_TASK + 1) -#define ISC_TASKEVENT_LASTEVENT (ISC_EVENTCLASS_TASK + 65535) +#define ISC_TASKEVENT_SHUTDOWN (ISC_EVENTCLASS_TASK + 0) +#define ISC_TASKEVENT_TEST (ISC_EVENTCLASS_TASK + 1) /***** ***** Tasks. diff --git a/lib/isc/include/isc/timer.h b/lib/isc/include/isc/timer.h index 1457d796f9..cbe2372878 100644 --- a/lib/isc/include/isc/timer.h +++ b/lib/isc/include/isc/timer.h @@ -81,15 +81,16 @@ typedef enum { isc_timertype_inactive = 3 /*%< Inactive */ } isc_timertype_t; -typedef struct isc_timerevent { +typedef struct isc_timerevent isc_timerevent_t; + +struct isc_timerevent { struct isc_event common; isc_time_t due; -} isc_timerevent_t; + ISC_LINK(isc_timerevent_t) ev_timerlink; +}; -#define ISC_TIMEREVENT_FIRSTEVENT (ISC_EVENTCLASS_TIMER + 0) -#define ISC_TIMEREVENT_TICK (ISC_EVENTCLASS_TIMER + 1) -#define ISC_TIMEREVENT_ONCE (ISC_EVENTCLASS_TIMER + 2) -#define ISC_TIMEREVENT_LASTEVENT (ISC_EVENTCLASS_TIMER + 65535) +#define ISC_TIMEREVENT_TICK (ISC_EVENTCLASS_TIMER + 0) +#define ISC_TIMEREVENT_ONCE (ISC_EVENTCLASS_TIMER + 1) /*** *** Timer and Timer Manager Functions diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 5d54e3843a..136ed3276a 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -70,6 +70,7 @@ struct isc_timer { void *arg; unsigned int index; isc_time_t due; + ISC_LIST(isc_timerevent_t) active; LINK(isc_timer_t) link; }; @@ -189,18 +190,44 @@ deschedule(isc_timer_t *timer) { } } +static void +timerevent_unlink(isc_timer_t *timer, isc_timerevent_t *event) { + fprintf(stderr, "unlinking %p from %p\n", event, &timer->active); + + REQUIRE(ISC_LINK_LINKED(event, ev_timerlink)); + ISC_LIST_UNLINK(timer->active, event, ev_timerlink); +} + +static void +timerevent_destroy(isc_event_t *event0) { + isc_timer_t *timer = event0->ev_destroy_arg; + isc_timerevent_t *event = (isc_timerevent_t *)event0; + + if (ISC_LINK_LINKED(event, ev_timerlink)) { + timerevent_unlink(timer, event); + } + + isc_mem_put(timer->manager->mctx, event, event0->ev_size); + isc_timer_detach(&timer); +} + +static void +timer_purge(isc_timer_t *timer) { + isc_timerevent_t *event = NULL; + + while ((event = ISC_LIST_HEAD(timer->active)) != NULL) { + (void)isc_task_purgeevent(timer->task, (isc_event_t *)event); + timerevent_unlink(timer, event); + } +} + static void destroy(isc_timer_t *timer) { isc_timermgr_t *manager = timer->manager; - /* - * The caller must ensure it is safe to destroy the timer. - */ - LOCK(&manager->lock); - (void)isc_task_purgerange(timer->task, timer, ISC_TIMEREVENT_FIRSTEVENT, - ISC_TIMEREVENT_LASTEVENT, NULL); + timer_purge(timer); deschedule(timer); UNLINK(manager->timers, timer, link); @@ -248,6 +275,8 @@ isc_timer_create(isc_timermgr_t *manager, isc_task_t *task, isc_mutex_init(&timer->lock); ISC_LINK_INIT(timer, link); + ISC_LIST_INIT(timer->active); + timer->magic = TIMER_MAGIC; /* @@ -303,9 +332,7 @@ isc_timer_reset(isc_timer_t *timer, isc_timertype_t type, LOCK(&timer->lock); if (purge) { - (void)isc_task_purgerange(timer->task, timer, - ISC_TIMEREVENT_FIRSTEVENT, - ISC_TIMEREVENT_LASTEVENT, NULL); + timer_purge(timer); } timer->type = type; timer->interval = *interval; @@ -376,20 +403,21 @@ static void post_event(isc_timermgr_t *manager, isc_timer_t *timer, isc_eventtype_t type) { isc_timerevent_t *event; XTRACEID("posting", timer); - /* - * XXX We could preallocate this event. - */ + event = (isc_timerevent_t *)isc_event_allocate( manager->mctx, timer, type, timer->action, timer->arg, sizeof(*event)); - if (event != NULL) { - event->due = timer->due; - isc_task_send(timer->task, ISC_EVENT_PTR(&event)); - } else { - UNEXPECTED_ERROR(__FILE__, __LINE__, "%s", - "couldn't allocate event"); - } + ISC_LINK_INIT(event, ev_timerlink); + ((isc_event_t *)event)->ev_destroy = timerevent_destroy; + + isc_timer_attach(timer, &(isc_timer_t *){ NULL }); + ((isc_event_t *)event)->ev_destroy_arg = timer; + + event->due = timer->due; + ISC_LIST_APPEND(timer->active, event, ev_timerlink); + + isc_task_send(timer->task, ISC_EVENT_PTR(&event)); } static void diff --git a/lib/isccc/include/isccc/events.h b/lib/isccc/include/isccc/events.h index 0461a8a96f..ddbfbcd79d 100644 --- a/lib/isccc/include/isccc/events.h +++ b/lib/isccc/include/isccc/events.h @@ -38,6 +38,3 @@ */ #define ISCCC_EVENT_CCMSG (ISC_EVENTCLASS_ISCCC + 0) - -#define ISCCC_EVENT_FIRSTEVENT (ISC_EVENTCLASS_ISCCC + 0) -#define ISCCC_EVENT_LASTEVENT (ISC_EVENTCLASS_ISCCC + 65535) From 9f7ba679ace5bc15f78db1ea8284677c0bf1a110 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 22:06:22 +0200 Subject: [PATCH 3/8] Purge the .resched_event in dns_cache Instead of sweeping the cache cleaner tasks, purge the more specific cleaner.resched_event event. --- lib/dns/cache.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dns/cache.c b/lib/dns/cache.c index bb4bea3052..b2d188ffc6 100644 --- a/lib/dns/cache.c +++ b/lib/dns/cache.c @@ -963,7 +963,7 @@ cleaner_shutdown_action(isc_task_t *task, isc_event_t *event) { } /* Make sure we don't reschedule anymore. */ - (void)isc_task_purge(task, NULL, DNS_EVENT_CACHECLEAN, NULL); + (void)isc_task_purgeevent(task, cache->cleaner.resched_event); isc_refcount_decrementz(&cache->live_tasks); From c17eee034be9c21c4e49a4b4ab338754d1d9b1b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 22:14:14 +0200 Subject: [PATCH 4/8] Remove isc_task_purge() and isc_task_purgerange() The isc_task_purge() and isc_task_purgerange() were now unused, so sweep the task.c file. Additionally remove unused ISC_EVENTATTR_NOPURGE event attribute. --- lib/isc/include/isc/event.h | 1 - lib/isc/include/isc/task.h | 57 ------ lib/isc/task.c | 87 --------- lib/isc/tests/task_test.c | 374 +----------------------------------- lib/isc/timer.c | 16 +- 5 files changed, 15 insertions(+), 520 deletions(-) diff --git a/lib/isc/include/isc/event.h b/lib/isc/include/isc/event.h index 75a528e4e9..2349f3140c 100644 --- a/lib/isc/include/isc/event.h +++ b/lib/isc/include/isc/event.h @@ -42,7 +42,6 @@ typedef void (*isc_eventdestructor_t)(isc_event_t *); * definition. Attributes of 0xffffff00 may be used by the application * or non-ISC libraries. */ -#define ISC_EVENTATTR_NOPURGE 0x00000001 /*% * The ISC_EVENTATTR_CANCELED attribute is intended to indicate diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index 1d995083c7..9bff6f8b60 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -234,63 +234,6 @@ isc_task_sendanddetach(isc_task_t **taskp, isc_event_t **eventp); * all resources used by the task will be freed. */ -unsigned int -isc_task_purgerange(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag); -/*%< - * Purge events from a task's event queue. - * - * Requires: - * - *\li 'task' is a valid task. - * - *\li last >= first - * - * Ensures: - * - *\li Events in the event queue of 'task' whose sender is 'sender', whose - * type is >= first and <= last, and whose tag is 'tag' will be purged, - * unless they are marked as unpurgable. - * - *\li A sender of NULL will match any sender. A NULL tag matches any - * tag. - * - * Returns: - * - *\li The number of events purged. - */ - -unsigned int -isc_task_purge(isc_task_t *task, void *sender, isc_eventtype_t type, void *tag); -/*%< - * Purge events from a task's event queue. - * - * Notes: - * - *\li This function is equivalent to - * - *\code - * isc_task_purgerange(task, sender, type, type, tag); - *\endcode - * - * Requires: - * - *\li 'task' is a valid task. - * - * Ensures: - * - *\li Events in the event queue of 'task' whose sender is 'sender', whose - * type is 'type', and whose tag is 'tag' will be purged, unless they - * are marked as unpurgable. - * - *\li A sender of NULL will match any sender. A NULL tag matches any - * tag. - * - * Returns: - * - *\li The number of events purged. - */ - bool isc_task_purgeevent(isc_task_t *task, isc_event_t *event); /*%< diff --git a/lib/isc/task.c b/lib/isc/task.c index df1f4d565a..cc76a6541f 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -523,93 +523,6 @@ isc_task_sendtoanddetach(isc_task_t **taskp, isc_event_t **eventp, int c) { *taskp = NULL; } -#define PURGE_OK(event) (((event)->ev_attributes & ISC_EVENTATTR_NOPURGE) == 0) - -static unsigned int -dequeue_events(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag, isc_eventlist_t *events, - bool purging) { - isc_event_t *event, *next_event; - unsigned int count = 0; - - REQUIRE(VALID_TASK(task)); - REQUIRE(last >= first); - - XTRACE("dequeue_events"); - - /* - * Events matching 'sender', whose type is >= first and <= last, and - * whose tag is 'tag' will be dequeued. If 'purging', matching events - * which are marked as unpurgable will not be dequeued. - * - * sender == NULL means "any sender", and tag == NULL means "any tag". - */ - - LOCK(&task->lock); - - for (event = HEAD(task->events); event != NULL; event = next_event) { - next_event = NEXT(event, ev_link); - if (event->ev_type >= first && event->ev_type <= last && - (sender == NULL || event->ev_sender == sender) && - (tag == NULL || event->ev_tag == tag) && - (!purging || PURGE_OK(event))) - { - DEQUEUE(task->events, event, ev_link); - task->nevents--; - ENQUEUE(*events, event, ev_link); - count++; - } - } - - UNLOCK(&task->lock); - - return (count); -} - -unsigned int -isc_task_purgerange(isc_task_t *task, void *sender, isc_eventtype_t first, - isc_eventtype_t last, void *tag) { - unsigned int count; - isc_eventlist_t events; - isc_event_t *event, *next_event; - REQUIRE(VALID_TASK(task)); - - /* - * Purge events from a task's event queue. - */ - - XTRACE("isc_task_purgerange"); - - ISC_LIST_INIT(events); - - count = dequeue_events(task, sender, first, last, tag, &events, true); - - for (event = HEAD(events); event != NULL; event = next_event) { - next_event = NEXT(event, ev_link); - ISC_LIST_UNLINK(events, event, ev_link); - isc_event_free(&event); - } - - /* - * Note that purging never changes the state of the task. - */ - - return (count); -} - -unsigned int -isc_task_purge(isc_task_t *task, void *sender, isc_eventtype_t type, - void *tag) { - /* - * Purge events from a task's event queue. - */ - REQUIRE(VALID_TASK(task)); - - XTRACE("isc_task_purge"); - - return (isc_task_purgerange(task, sender, type, type, tag)); -} - bool isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { bool found = false; diff --git a/lib/isc/tests/task_test.c b/lib/isc/tests/task_test.c index 0a4d91e5e5..57a1bec477 100644 --- a/lib/isc/tests/task_test.c +++ b/lib/isc/tests/task_test.c @@ -534,8 +534,6 @@ basic(void **state) { isc_task_send(task1, &event); } - (void)isc_task_purge(task3, NULL, 0, 0); - isc_task_detach(&task1); isc_task_detach(&task2); isc_task_detach(&task3); @@ -959,349 +957,10 @@ post_shutdown(void **state) { #define TAGCNT 5 #define NEVENTS (SENDERCNT * TYPECNT * TAGCNT) -static bool testrange; -static void *purge_sender; -static isc_eventtype_t purge_type_first; -static isc_eventtype_t purge_type_last; -static void *purge_tag; static int eventcnt; atomic_bool started; -static void -pg_event1(isc_task_t *task, isc_event_t *event) { - UNUSED(task); - - LOCK(&lock); - while (!atomic_load(&started)) { - WAIT(&cv, &lock); - } - UNLOCK(&lock); - - isc_event_free(&event); -} - -static void -pg_event2(isc_task_t *task, isc_event_t *event) { - bool sender_match = false; - bool type_match = false; - bool tag_match = false; - - UNUSED(task); - - if ((purge_sender == NULL) || (purge_sender == event->ev_sender)) { - sender_match = true; - } - - if (testrange) { - if ((purge_type_first <= event->ev_type) && - (event->ev_type <= purge_type_last)) { - type_match = true; - } - } else { - if (purge_type_first == event->ev_type) { - type_match = true; - } - } - - if ((purge_tag == NULL) || (purge_tag == event->ev_tag)) { - tag_match = true; - } - - if (sender_match && type_match && tag_match) { - if ((event->ev_attributes & ISC_EVENTATTR_NOPURGE) != 0) { - if (verbose) { - print_message("# event %p,%d,%p " - "matched but was not " - "purgeable\n", - event->ev_sender, - (int)event->ev_type, - event->ev_tag); - } - ++eventcnt; - } else if (verbose) { - print_message("# event %p,%d,%p not purged\n", - event->ev_sender, (int)event->ev_type, - event->ev_tag); - } - } else { - ++eventcnt; - } - - isc_event_free(&event); -} - -static void -pg_sde(isc_task_t *task, isc_event_t *event) { - UNUSED(task); - - LOCK(&lock); - atomic_store(&done, true); - SIGNAL(&cv); - UNLOCK(&lock); - - isc_event_free(&event); -} - -static void -test_purge(int sender, int type, int tag, int exp_purged) { - isc_result_t result; - isc_task_t *task = NULL; - isc_event_t *eventtab[NEVENTS]; - isc_event_t *event = NULL; - isc_interval_t interval; - isc_time_t now; - int sender_cnt, type_cnt, tag_cnt, event_cnt, i; - int purged = 0; - - atomic_init(&started, false); - atomic_init(&done, false); - eventcnt = 0; - - isc_condition_init(&cv); - - result = isc_task_create(taskmgr, 0, &task); - assert_int_equal(result, ISC_R_SUCCESS); - - result = isc_task_onshutdown(task, pg_sde, NULL); - assert_int_equal(result, ISC_R_SUCCESS); - - /* - * Block the task on cv. - */ - event = isc_event_allocate(test_mctx, (void *)1, 9999, pg_event1, NULL, - sizeof(*event)); - - assert_non_null(event); - isc_task_send(task, &event); - - /* - * Fill the task's queue with some messages with varying - * sender, type, tag, and purgeable attribute values. - */ - event_cnt = 0; - for (sender_cnt = 0; sender_cnt < SENDERCNT; ++sender_cnt) { - for (type_cnt = 0; type_cnt < TYPECNT; ++type_cnt) { - for (tag_cnt = 0; tag_cnt < TAGCNT; ++tag_cnt) { - eventtab[event_cnt] = isc_event_allocate( - test_mctx, - &senders[sender + sender_cnt], - (isc_eventtype_t)(type + type_cnt), - pg_event2, NULL, sizeof(*event)); - - assert_non_null(eventtab[event_cnt]); - - eventtab[event_cnt]->ev_tag = - (void *)((uintptr_t)tag + tag_cnt); - - /* - * Mark events as non-purgeable if - * sender, type and tag are all - * odd-numbered. (There should be 4 - * of these out of 60 events total.) - */ - if (((sender_cnt % 2) != 0) && - ((type_cnt % 2) != 0) && - ((tag_cnt % 2) != 0)) { - eventtab[event_cnt]->ev_attributes |= - ISC_EVENTATTR_NOPURGE; - } - ++event_cnt; - } - } - } - - for (i = 0; i < event_cnt; ++i) { - isc_task_send(task, &eventtab[i]); - } - - if (testrange) { - /* - * We're testing isc_task_purgerange. - */ - purged = isc_task_purgerange( - task, purge_sender, (isc_eventtype_t)purge_type_first, - (isc_eventtype_t)purge_type_last, purge_tag); - assert_int_equal(purged, exp_purged); - } else { - /* - * We're testing isc_task_purge. - */ - if (verbose) { - print_message("# purge events %p,%u,%p\n", purge_sender, - purge_type_first, purge_tag); - } - purged = isc_task_purge(task, purge_sender, - (isc_eventtype_t)purge_type_first, - purge_tag); - if (verbose) { - print_message("# purged %d expected %d\n", purged, - exp_purged); - } - - assert_int_equal(purged, exp_purged); - } - - /* - * Unblock the task, allowing event processing. - */ - LOCK(&lock); - atomic_store(&started, true); - SIGNAL(&cv); - - isc_task_shutdown(task); - - isc_interval_set(&interval, 5, 0); - - /* - * Wait for shutdown processing to complete. - */ - while (!atomic_load(&done)) { - result = isc_time_nowplusinterval(&now, &interval); - assert_int_equal(result, ISC_R_SUCCESS); - - WAITUNTIL(&cv, &lock, &now); - } - - UNLOCK(&lock); - - isc_task_detach(&task); - - assert_int_equal(eventcnt, event_cnt - exp_purged); -} - -/* - * Purge test: - * A call to isc_task_purge(task, sender, type, tag) purges all events of - * type 'type' and with tag 'tag' not marked as unpurgeable from sender - * from the task's " queue and returns the number of events purged. - */ -static void -purge(void **state) { - UNUSED(state); - - /* Try purging on a specific sender. */ - if (verbose) { - print_message("# testing purge on 2,4,8 expecting 1\n"); - } - purge_sender = &senders[2]; - purge_type_first = 4; - purge_type_last = 4; - purge_tag = (void *)8; - testrange = false; - test_purge(1, 4, 7, 1); - - /* Try purging on all senders. */ - if (verbose) { - print_message("# testing purge on 0,4,8 expecting 3\n"); - } - purge_sender = NULL; - purge_type_first = 4; - purge_type_last = 4; - purge_tag = (void *)8; - testrange = false; - test_purge(1, 4, 7, 3); - - /* Try purging on all senders, specified type, all tags. */ - if (verbose) { - print_message("# testing purge on 0,4,0 expecting 15\n"); - } - purge_sender = NULL; - purge_type_first = 4; - purge_type_last = 4; - purge_tag = NULL; - testrange = false; - test_purge(1, 4, 7, 15); - - /* Try purging on a specified tag, no such type. */ - if (verbose) { - print_message("# testing purge on 0,99,8 expecting 0\n"); - } - purge_sender = NULL; - purge_type_first = 99; - purge_type_last = 99; - purge_tag = (void *)8; - testrange = false; - test_purge(1, 4, 7, 0); - - /* Try purging on specified sender, type, all tags. */ - if (verbose) { - print_message("# testing purge on 3,5,0 expecting 5\n"); - } - purge_sender = &senders[3]; - purge_type_first = 5; - purge_type_last = 5; - purge_tag = NULL; - testrange = false; - test_purge(1, 4, 7, 5); -} - -/* - * Purge range test: - * A call to isc_event_purgerange(task, sender, first, last, tag) purges - * all events not marked unpurgeable from sender 'sender' and of type within - * the range 'first' to 'last' inclusive from the task's event queue and - * returns the number of tasks purged. - */ - -static void -purgerange(void **state) { - UNUSED(state); - - /* Now let's try some ranges. */ - /* testing purgerange on 2,4-5,8 expecting 1 */ - purge_sender = &senders[2]; - purge_type_first = 4; - purge_type_last = 5; - purge_tag = (void *)8; - testrange = true; - test_purge(1, 4, 7, 1); - - /* Try purging on all senders. */ - if (verbose) { - print_message("# testing purge on 0,4-5,8 expecting 5\n"); - } - purge_sender = NULL; - purge_type_first = 4; - purge_type_last = 5; - purge_tag = (void *)8; - testrange = true; - test_purge(1, 4, 7, 5); - - /* Try purging on all senders, specified type, all tags. */ - if (verbose) { - print_message("# testing purge on 0,5-6,0 expecting 28\n"); - } - purge_sender = NULL; - purge_type_first = 5; - purge_type_last = 6; - purge_tag = NULL; - testrange = true; - test_purge(1, 4, 7, 28); - - /* Try purging on a specified tag, no such type. */ - if (verbose) { - print_message("# testing purge on 0,99-101,8 expecting 0\n"); - } - purge_sender = NULL; - purge_type_first = 99; - purge_type_last = 101; - purge_tag = (void *)8; - testrange = true; - test_purge(1, 4, 7, 0); - - /* Try purging on specified sender, type, all tags. */ - if (verbose) { - print_message("# testing purge on 3,5-6,0 expecting 10\n"); - } - purge_sender = &senders[3]; - purge_type_first = 5; - purge_type_last = 6; - purge_tag = NULL; - testrange = true; - test_purge(1, 4, 7, 10); -} - /* * Helpers for purge event tests */ @@ -1339,7 +998,7 @@ pge_sde(isc_task_t *task, isc_event_t *event) { } static void -try_purgeevent(bool purgeable) { +try_purgeevent(void) { isc_result_t result; isc_task_t *task = NULL; bool purged; @@ -1375,16 +1034,11 @@ try_purgeevent(bool purgeable) { event2_clone = event2; - if (purgeable) { - event2->ev_attributes &= ~ISC_EVENTATTR_NOPURGE; - } else { - event2->ev_attributes |= ISC_EVENTATTR_NOPURGE; - } - isc_task_send(task, &event2); purged = isc_task_purgeevent(task, event2_clone); - assert_int_equal(purgeable, purged); + + assert_true(purged); /* * Unblock the task, allowing event processing. @@ -1410,8 +1064,6 @@ try_purgeevent(bool purgeable) { UNLOCK(&lock); isc_task_detach(&task); - - assert_int_equal(eventcnt, (purgeable ? 0 : 1)); } /* @@ -1425,21 +1077,7 @@ static void purgeevent(void **state) { UNUSED(state); - try_purgeevent(true); -} - -/* - * Purge event not purgeable test: - * When the event is not marked as purgable, a call to - * isc_task_purgeevent(task, event) does not purge the event - * 'event' from the task's queue and returns false. - */ - -static void -purgeevent_notpurge(void **state) { - UNUSED(state); - - try_purgeevent(false); + try_purgeevent(); } int @@ -1455,11 +1093,7 @@ main(int argc, char **argv) { _teardown), cmocka_unit_test_setup_teardown(privileged_events, _setup, _teardown), - cmocka_unit_test_setup_teardown(purge, _setup2, _teardown), cmocka_unit_test_setup_teardown(purgeevent, _setup2, _teardown), - cmocka_unit_test_setup_teardown(purgeevent_notpurge, _setup, - _teardown), - cmocka_unit_test_setup_teardown(purgerange, _setup, _teardown), cmocka_unit_test_setup_teardown(task_shutdown, _setup4, _teardown), cmocka_unit_test_setup_teardown(task_exclusive, _setup4, diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 136ed3276a..6ecb1b4c1b 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -192,8 +192,6 @@ deschedule(isc_timer_t *timer) { static void timerevent_unlink(isc_timer_t *timer, isc_timerevent_t *event) { - fprintf(stderr, "unlinking %p from %p\n", event, &timer->active); - REQUIRE(ISC_LINK_LINKED(event, ev_timerlink)); ISC_LIST_UNLINK(timer->active, event, ev_timerlink); } @@ -204,6 +202,7 @@ timerevent_destroy(isc_event_t *event0) { isc_timerevent_t *event = (isc_timerevent_t *)event0; if (ISC_LINK_LINKED(event, ev_timerlink)) { + /* The event was unlinked via timer_purge() */ timerevent_unlink(timer, event); } @@ -216,8 +215,15 @@ timer_purge(isc_timer_t *timer) { isc_timerevent_t *event = NULL; while ((event = ISC_LIST_HEAD(timer->active)) != NULL) { - (void)isc_task_purgeevent(timer->task, (isc_event_t *)event); - timerevent_unlink(timer, event); + bool purged = isc_task_purgeevent(timer->task, + (isc_event_t *)event); + if (!purged) { + /* + * The event has already been executed, but not + * yet destroyed. + */ + timerevent_unlink(timer, event); + } } } @@ -383,7 +389,6 @@ isc_timer_attach(isc_timer_t *timer, isc_timer_t **timerp) { void isc_timer_detach(isc_timer_t **timerp) { isc_timer_t *timer; - /* * Detach *timerp from its timer. */ @@ -415,6 +420,7 @@ post_event(isc_timermgr_t *manager, isc_timer_t *timer, isc_eventtype_t type) { ((isc_event_t *)event)->ev_destroy_arg = timer; event->due = timer->due; + ISC_LIST_APPEND(timer->active, event, ev_timerlink); isc_task_send(timer->task, ISC_EVENT_PTR(&event)); From 15ea6f002ffbe1f3463639d3fe8c0bc07526a6d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 1 Apr 2022 10:40:37 +0200 Subject: [PATCH 5/8] Add isc_task_setquantum() and use it for post-init zone loading Add isc_task_setquantum() function that modifies quantum for the future isc_task_run() invocations. NOTE: The current isc_task_run() caches the task->quantum into a local variable and therefore the current event loop is not affected by any quantum change. --- lib/isc/include/isc/task.h | 9 +++++++++ lib/isc/task.c | 16 ++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/lib/isc/include/isc/task.h b/lib/isc/include/isc/task.h index 9bff6f8b60..923d39e18f 100644 --- a/lib/isc/include/isc/task.h +++ b/lib/isc/include/isc/task.h @@ -398,6 +398,15 @@ isc_task_gettag(isc_task_t *task); *\li 'task' is a valid task. */ +void +isc_task_setquantum(isc_task_t *task, unsigned int quantum); +/*%< + * Set future 'task' quantum to 'quantum'. The current 'task' quantum will be + * kept for the current isc_task_run() loop, and will be changed for the next + * run. Therefore, the function is save to use from the event callback as it + * will not affect the current event loop processing. + */ + isc_result_t isc_task_beginexclusive(isc_task_t *task); /*%< diff --git a/lib/isc/task.c b/lib/isc/task.c index cc76a6541f..f2c60b9b8a 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -660,6 +660,16 @@ isc_task_getnetmgr(isc_task_t *task) { return (task->manager->netmgr); } +void +isc_task_setquantum(isc_task_t *task, unsigned int quantum) { + REQUIRE(VALID_TASK(task)); + + LOCK(&task->lock); + task->quantum = (quantum > 0) ? quantum + : task->manager->default_quantum; + UNLOCK(&task->lock); +} + /*** *** Task Manager. ***/ @@ -670,11 +680,13 @@ task_run(isc_task_t *task) { bool finished = false; isc_event_t *event = NULL; isc_result_t result = ISC_R_SUCCESS; + uint32_t quantum; REQUIRE(VALID_TASK(task)); LOCK(&task->lock); - /* FIXME */ + quantum = task->quantum; + if (task->state != task_state_ready) { goto done; } @@ -747,7 +759,7 @@ task_run(isc_task_t *task) { task->state = task_state_idle; } break; - } else if (dispatch_count >= task->quantum) { + } else if (dispatch_count >= quantum) { /* * Our quantum has expired, but there is more work to be * done. We'll requeue it to the ready queue later. From 87c4c24cdeb858decaf11232bd413a8a815b4732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Thu, 31 Mar 2022 22:15:49 +0200 Subject: [PATCH 6/8] Set quantum to infinity for the zone loading task When we are loading the zones, set the quantum to UINT_MAX, which makes task_run process all tasks at once. After the zone loading is finished the quantum will be dropped to 1 to not block server when we are loading new zones after reconfiguration. --- lib/dns/zone.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 31443d6815..ceb915ed5c 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -2426,6 +2426,9 @@ zone_asyncload(isc_task_t *task, isc_event_t *event) { (asl->loaded)(asl->loaded_arg, zone, task); } + /* Reduce the quantum */ + isc_task_setquantum(zone->loadtask, 1); + isc_mem_put(zone->mctx, asl, sizeof(*asl)); dns_zone_idetach(&zone); } @@ -19167,7 +19170,7 @@ dns_zonemgr_setsize(dns_zonemgr_t *zmgr, int num_zones) { pool = NULL; if (zmgr->loadtasks == NULL) { result = isc_taskpool_create(zmgr->taskmgr, zmgr->mctx, ntasks, - 2, true, &pool); + UINT_MAX, true, &pool); } else { result = isc_taskpool_expand(&zmgr->loadtasks, ntasks, true, &pool); From 40971b22e7695966a898002efa0c0c18649568c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 1 Apr 2022 10:51:41 +0200 Subject: [PATCH 7/8] Stop the zone timer before detaching the timer Previously, the zone timer was not stopped before detaching the timer. This could lead to a data race where the timer post_event() could fire before the timer was detached, but then the event would be executed after the zone was already destroyed. This was not noticed before because the timing or the ordering of the actions were different, but it was causing assertion failures in the libns tests now. Properly stop the zone timer before detaching the timer object from the dns_zone. --- lib/dns/zone.c | 61 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/lib/dns/zone.c b/lib/dns/zone.c index ceb915ed5c..2b3189cc55 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -849,6 +849,11 @@ unsigned int dns_zone_mkey_month = MONTH; #define SEND_BUFFER_SIZE 2048 +static void +zone_timer_start(dns_zone_t *zone, isc_time_t *next, isc_time_t *now); +static void +zone_timer_stop(dns_zone_t *zone); + static void zone_settimer(dns_zone_t *, isc_time_t *); static void @@ -15077,6 +15082,7 @@ zone_shutdown(isc_task_t *task, isc_event_t *event) { forward_cancel(zone); if (zone->timer != NULL) { + zone_timer_stop(zone); isc_timer_detach(&zone->timer); isc_refcount_decrement(&zone->irefs); } @@ -15129,11 +15135,41 @@ zone_timer(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); } +static void +zone_timer_start(dns_zone_t *zone, isc_time_t *next, isc_time_t *now) { + isc_interval_t interval; + isc_result_t result; + + if (isc_time_compare(next, now) <= 0) { + isc_interval_set(&interval, 0, 1); + } else { + isc_time_subtract(next, now, &interval); + } + + result = isc_timer_reset(zone->timer, isc_timertype_once, &interval, + true); + if (result != ISC_R_SUCCESS) { + dns_zone_log(zone, ISC_LOG_ERROR, + "could not reset zone timer: %s", + isc_result_totext(result)); + } +} + +static void +zone_timer_stop(dns_zone_t *zone) { + isc_result_t result = isc_timer_reset( + zone->timer, isc_timertype_inactive, NULL, true); + if (result != ISC_R_SUCCESS) { + dns_zone_log(zone, ISC_LOG_ERROR, + "could not deactivate zone timer: %s", + isc_result_totext(result)); + } +} + static void zone_settimer(dns_zone_t *zone, isc_time_t *now) { const char me[] = "zone_settimer"; isc_time_t next; - isc_result_t result; REQUIRE(DNS_ZONE_VALID(zone)); REQUIRE(LOCKED_ZONE(zone)); @@ -15272,28 +15308,9 @@ zone_settimer(dns_zone_t *zone, isc_time_t *now) { if (isc_time_isepoch(&next)) { zone_debuglog(zone, me, 10, "settimer inactive"); - result = isc_timer_reset(zone->timer, isc_timertype_inactive, - NULL, true); - if (result != ISC_R_SUCCESS) { - dns_zone_log(zone, ISC_LOG_ERROR, - "could not deactivate zone timer: %s", - isc_result_totext(result)); - } + zone_timer_stop(zone); } else { - isc_interval_t interval; - if (isc_time_compare(&next, now) <= 0) { - isc_interval_set(&interval, 0, 1); - } else { - isc_time_subtract(&next, now, &interval); - } - - result = isc_timer_reset(zone->timer, isc_timertype_once, - &interval, true); - if (result != ISC_R_SUCCESS) { - dns_zone_log(zone, ISC_LOG_ERROR, - "could not reset zone timer: %s", - isc_result_totext(result)); - } + zone_timer_start(zone, &next, now); } } From a7cd0868a249d10918a225aee8a6c5e208d7187b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Fri, 1 Apr 2022 10:59:28 +0200 Subject: [PATCH 8/8] Add CHANGES note for [GL #3252] --- CHANGES | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES b/CHANGES index 8d6ddedf25..3b8025e8c7 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,10 @@ +5845. [bug] Refactor the timer to keep track of posted events + as to use isc_task_purgeevent() instead of using + isc_task_purgerange(). The isc_task_purgeevent() + has been refactored to purge a single event instead + of walking through the list of posted events. + [GL #3252] + 5844. [bug] dig +nssearch was hanging until manually interrupted. [GL #3145]