From d6ebf485be5a1284c17a3e858921ad44e557063b Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 29 Dec 2021 22:25:14 +0000 Subject: [PATCH 1/2] sysmon(9): New sysmon_task_queue_barrier() function. This waits for all tasks currently queued at the time of the call to complete. XXX Doesn't do anything about priorities at the moment. --- share/man/man9/sysmon_taskq.9 | 9 +++++++- sys/dev/sysmon/sysmon_taskq.c | 42 +++++++++++++++++++++++++++++------ sys/dev/sysmon/sysmon_taskq.h | 1 + 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/share/man/man9/sysmon_taskq.9 b/share/man/man9/sysmon_taskq.9 index baa3c86ee79e..85b01bac52d8 100644 --- a/share/man/man9/sysmon_taskq.9 +++ b/share/man/man9/sysmon_taskq.9 @@ -43,6 +43,8 @@ .Fn sysmon_task_queue_fini "void" .Ft int .Fn sysmon_task_queue_sched "u_int pri" "void (*func)(void *)" "void *arg" +.Ft void +.Fn sysmon_task_queue_barrier .Sh DESCRIPTION The machine-independent .Nm @@ -67,7 +69,7 @@ All scheduled tasks are executed before the queue is halted. .Pp The .Fn sysmon_task_queue_sched -enqueues +function enqueues .Fa func to be executed at the priority .Fa pri . @@ -78,6 +80,11 @@ The single argument passed to .Fa func is specified by .Fa arg . +.Pp +The +.Fn sysmon_task_queue_barrier +function waits for all tasks currently queued at the time of the call +to complete. .Sh RETURN VALUES Upon successful completion, .Fn sysmon_task_queue_sched diff --git a/sys/dev/sysmon/sysmon_taskq.c b/sys/dev/sysmon/sysmon_taskq.c index e027d009b925..f92c71b3d942 100644 --- a/sys/dev/sysmon/sysmon_taskq.c +++ b/sys/dev/sysmon/sysmon_taskq.c @@ -67,6 +67,8 @@ static TAILQ_HEAD(, sysmon_task) sysmon_task_queue = static kmutex_t sysmon_task_queue_mtx; static kmutex_t sysmon_task_queue_init_mtx; static kcondvar_t sysmon_task_queue_cv; +static uint64_t sysmon_task_queue_gen; +static bool sysmon_task_queue_running; static int sysmon_task_queue_initialized; static int sysmon_task_queue_cleanup_sem; @@ -152,7 +154,7 @@ sysmon_task_queue_fini(void) mutex_enter(&sysmon_task_queue_mtx); sysmon_task_queue_cleanup_sem = 1; - cv_signal(&sysmon_task_queue_cv); + cv_broadcast(&sysmon_task_queue_cv); while (sysmon_task_queue_cleanup_sem != 0) cv_wait(&sysmon_task_queue_cv, @@ -172,7 +174,8 @@ sysmon_task_queue_fini(void) static void sysmon_task_queue_thread(void *arg) { - struct sysmon_task *st; + struct sysmon_task *st, *nextst; + TAILQ_HEAD(, sysmon_task) batch = TAILQ_HEAD_INITIALIZER(batch); /* * Run through all the tasks before we check for the exit @@ -181,13 +184,18 @@ sysmon_task_queue_thread(void *arg) */ mutex_enter(&sysmon_task_queue_mtx); for (;;) { - st = TAILQ_FIRST(&sysmon_task_queue); - if (st != NULL) { - TAILQ_REMOVE(&sysmon_task_queue, st, st_list); + if (!TAILQ_EMPTY(&sysmon_task_queue)) { + TAILQ_CONCAT(&batch, &sysmon_task_queue, st_list); + sysmon_task_queue_running = true; mutex_exit(&sysmon_task_queue_mtx); - (*st->st_func)(st->st_arg); - free(st, M_TEMP); + TAILQ_FOREACH_SAFE(st, &batch, st_list, nextst) { + (*st->st_func)(st->st_arg); + free(st, M_TEMP); + } mutex_enter(&sysmon_task_queue_mtx); + sysmon_task_queue_running = false; + sysmon_task_queue_gen++; + cv_broadcast(&sysmon_task_queue_cv); } else { /* Check for the exit condition. */ if (sysmon_task_queue_cleanup_sem != 0) @@ -246,6 +254,26 @@ sysmon_task_queue_sched(u_int pri, void (*func)(void *), void *arg) return 0; } +void +sysmon_task_queue_barrier(void) +{ + uint64_t gen; + + (void)RUN_ONCE(&once_tq, tq_preinit); + + KASSERT(curlwp != sysmon_task_queue_lwp); + + mutex_enter(&sysmon_task_queue_mtx); + gen = sysmon_task_queue_gen; + if (sysmon_task_queue_running) + gen++; + if (!TAILQ_EMPTY(&sysmon_task_queue)) + gen++; + while (sysmon_task_queue_gen != gen) + cv_wait(&sysmon_task_queue_cv, &sysmon_task_queue_mtx); + mutex_exit(&sysmon_task_queue_mtx); +} + static int sysmon_taskq_modcmd(modcmd_t cmd, void *arg) diff --git a/sys/dev/sysmon/sysmon_taskq.h b/sys/dev/sysmon/sysmon_taskq.h index 32a4fcd7a2f8..eb5cfcbd9d3a 100644 --- a/sys/dev/sysmon/sysmon_taskq.h +++ b/sys/dev/sysmon/sysmon_taskq.h @@ -41,5 +41,6 @@ void sysmon_task_queue_init(void); int sysmon_task_queue_fini(void); int sysmon_task_queue_sched(u_int, void (*)(void *), void *); +void sysmon_task_queue_barrier(void); #endif /* _DEV_SYSMON_SYSMON_TASKQ_H_ */ From 7fc18f4415a44c1fbb834c2409492bfa0fb543cc Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Wed, 29 Dec 2021 22:54:11 +0000 Subject: [PATCH 2/2] acpi(9): Fix memory ordering and completion bugs in notifiers. 1. Guarantee everything which happened before acpi_register_notify has also happened before the notifier is actually called. 2. On acpi_deregister_notify, don't return until the notifier is definitely not running any more on any CPU, using the new sysmon_task_queue_barrier. --- sys/dev/acpi/acpi.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/sys/dev/acpi/acpi.c b/sys/dev/acpi/acpi.c index 76bab345c5d3..da372c688382 100644 --- a/sys/dev/acpi/acpi.c +++ b/sys/dev/acpi/acpi.c @@ -107,6 +107,7 @@ __KERNEL_RCSID(0, "$NetBSD: acpi.c,v 1.294 2021/12/20 11:17:40 skrll Exp $"); #include "opt_pcifixup.h" #include +#include #include #include #include @@ -126,6 +127,8 @@ __KERNEL_RCSID(0, "$NetBSD: acpi.c,v 1.294 2021/12/20 11:17:40 skrll Exp $"); #include #include +#include + #include #include "ioconf.h" @@ -1146,6 +1149,7 @@ acpi_notify_handler(ACPI_HANDLE handle, uint32_t event, void *aux) { struct acpi_softc *sc = acpi_softc; struct acpi_devnode *ad; + ACPI_NOTIFY_HANDLER notify; KASSERT(sc != NULL); KASSERT(aux == NULL); @@ -1189,13 +1193,13 @@ acpi_notify_handler(ACPI_HANDLE handle, uint32_t event, void *aux) if (ad->ad_device == NULL) continue; - if (ad->ad_notify == NULL) + if ((notify = atomic_load_acquire(&ad->ad_notify)) == NULL) continue; if (ad->ad_handle != handle) continue; - (*ad->ad_notify)(ad->ad_handle, event, ad->ad_device); + (*notify)(ad->ad_handle, event, ad->ad_device); return; } @@ -1218,7 +1222,7 @@ acpi_register_notify(struct acpi_devnode *ad, ACPI_NOTIFY_HANDLER notify) if (ad == NULL || notify == NULL) goto fail; - ad->ad_notify = notify; + atomic_store_release(&ad->ad_notify, notify); return true; @@ -1233,7 +1237,15 @@ void acpi_deregister_notify(struct acpi_devnode *ad) { - ad->ad_notify = NULL; + atomic_store_relaxed(&ad->ad_notify, NULL); + + /* + * Wait for any in-flight calls to the notifier to complete. + * + * XXX This assumes that sys/dev/acpi/acpica/OsdSchedule.c uses + * the sysmon task queue to call acpi_notify_handler. + */ + sysmon_task_queue_barrier(); } /*