From: Mark Kettenis Subject: Re: disestablish dohooks To: "Ted Unangst" Cc: cjeker@diehard.n-r-g.com, tech@openbsd.org Date: Wed, 11 Jun 2025 00:08:35 +0200 > From: "Ted Unangst" > Date: Tue, 10 Jun 2025 16:15:50 -0400 > > On 2025-06-10, Claudio Jeker wrote: > > On Tue, Jun 10, 2025 at 04:48:04AM -0400, Ted Unangst wrote: > > > Unless I've missed something, the only in use remnant of the hooks > > > code is for startup hooks. And never disestablished. > > > > > > So we can burn through one layer of abstraction, and also eliminate > > > one unused function entirely. > > > > > > There's also a collection of man.9 pages. > > > > We have various hooks all over the place reimplementing the same code. > > Should we try to consolidate those instead. e.g. kthread_create_deferred > > is a prime example for reimplementing the wheel for no good reason. To repeat what I said to Claudio on icb: the startuphook stuff appears to be an interface that was never widely adopted. Apart from some crufty hppa code, only acpithinkpad(4) seems to use it. That said, kthread_create_deferred() is a weird interface. Because you still have to create your own kthread when your function gets called... > This turns kthread_create_deferred into a startuphook. Independent of > previous diff. The functions appear to be used almost interchangably. > The only difference is that kthread will run immediately post boot, > while startuphook_establish called late will never run. > > I think this is a first step, then delete the kthread interface and > add the "just run now" logic to startuphook_establish. > > The only ordering consideration is in acpi. I think we should move the > thread creation there earlier, so it's running before any devices > do stuff. (I think we should do this anyway, the acpi thread should be > running before child threads, no?) The startuphook_establish(9) man page clearly states that startup hooks run immediately before the root and swap devices are configured. Your diff makes it do something different. And since we have that config_pending loop right before that matters. And that config_pending loop is there to wait for work done in kthreads created through kthread_create_deferred(). So that's a pretty hard NACK to this diff. Maybe Claudio meant to use the hooks infrastructure to create a new hook for kthread_create_deferred() instead of it creating its own list? > Index: kern/init_main.c > =================================================================== > RCS file: /home/cvs/src/sys/kern/init_main.c,v > diff -u -p -r1.329 init_main.c > --- kern/init_main.c 9 Jun 2025 10:57:46 -0000 1.329 > +++ kern/init_main.c 10 Jun 2025 19:56:10 -0000 > @@ -165,6 +165,7 @@ main(void *framep) > struct pdevinit *pdev; > extern struct pdevinit pdevinit[]; > extern void disk_init(void); > + extern int kthread_create_now; > > /* > * Initialize the current process pointer (curproc) before > @@ -429,10 +430,12 @@ main(void *framep) > } > > /* > + * Run startup hooks. > * Create any kernel threads whose creation was deferred because > * initprocess had not yet been created. > */ > - kthread_run_deferred_queue(); > + kthread_create_now = 1; > + dostartuphooks(); > > /* > * Now that device driver threads have been created, wait for > @@ -442,8 +445,6 @@ main(void *framep) > */ > while (config_pending) > tsleep_nsec(&config_pending, PWAIT, "cfpend", INFSLP); > - > - dostartuphooks(); > > #if NVSCSI > 0 > config_rootfound("vscsi", NULL); > Index: kern/kern_kthread.c > =================================================================== > RCS file: /home/cvs/src/sys/kern/kern_kthread.c,v > diff -u -p -r1.46 kern_kthread.c > --- kern/kern_kthread.c 26 Nov 2021 04:42:13 -0000 1.46 > +++ kern/kern_kthread.c 10 Jun 2025 19:53:30 -0000 > @@ -97,14 +97,6 @@ kthread_exit(int ecode) > /* NOTREACHED */ > } > > -struct kthread_q { > - SIMPLEQ_ENTRY(kthread_q) kq_q; > - void (*kq_func)(void *); > - void *kq_arg; > -}; > - > -SIMPLEQ_HEAD(, kthread_q) kthread_q = SIMPLEQ_HEAD_INITIALIZER(kthread_q); > - > /* > * Defer the creation of a kernel thread. Once the standard kernel threads > * and processes have been created, this queue will be run to callback to > @@ -113,34 +105,9 @@ SIMPLEQ_HEAD(, kthread_q) kthread_q = SI > void > kthread_create_deferred(void (*func)(void *), void *arg) > { > - struct kthread_q *kq; > - > if (kthread_create_now) { > (*func)(arg); > return; > } > - > - kq = malloc(sizeof *kq, M_TEMP, M_NOWAIT|M_ZERO); > - if (kq == NULL) > - panic("unable to allocate kthread_q"); > - > - kq->kq_func = func; > - kq->kq_arg = arg; > - > - SIMPLEQ_INSERT_TAIL(&kthread_q, kq, kq_q); > -} > - > -void > -kthread_run_deferred_queue(void) > -{ > - struct kthread_q *kq; > - > - /* No longer need to defer kthread creation. */ > - kthread_create_now = 1; > - > - while ((kq = SIMPLEQ_FIRST(&kthread_q)) != NULL) { > - SIMPLEQ_REMOVE_HEAD(&kthread_q, kq_q); > - (*kq->kq_func)(kq->kq_arg); > - free(kq, M_TEMP, sizeof(*kq)); > - } > + startuphook_establish(func, arg); > } > Index: dev/acpi/acpi.c > =================================================================== > RCS file: /home/cvs/src/sys/dev/acpi/acpi.c,v > diff -u -p -r1.448 acpi.c > --- dev/acpi/acpi.c 2 Jun 2025 02:20:56 -0000 1.448 > +++ dev/acpi/acpi.c 10 Jun 2025 20:10:20 -0000 > @@ -1031,6 +1031,8 @@ acpi_attach_common(struct acpi_softc *sc > "acpiwqpl", NULL); > pool_setlowat(&acpiwqpool, 16); > > + kthread_create_deferred(acpi_create_thread, sc); > + > SIMPLEQ_INIT(&sc->sc_tables); > SIMPLEQ_INIT(&sc->sc_wakedevs); > #if NACPIPWRRES > 0 > @@ -1309,8 +1311,6 @@ acpi_attach_common(struct acpi_softc *sc > pci_dopm = 1; > > acpi_attach_machdep(sc); > - > - kthread_create_deferred(acpi_create_thread, sc); > } > > int > >