This is the next patch in the clock interrupt reorganization series. This patch moves the entry points for the interval and profile dt(4) providers from the hardclock(9) to a dedicated clock interrupt callback, dt_prov_profile_intr(), in dev/dt/dt_prov_profile.c.
- To preserve current behavior, (1) both provider entrypoints have been moved into a single callback function, (2) the interrupt runs at the same frequency as the hardclock, and (3) the interrupt is staggered to co-occur with the hardclock on a given CPU. All of this can be changed later. This patch is strictly logistical: moving these parts out of the hardclock. - Each dt_pcb gets a provider-specific "dp_clockintr" pointer. If the PCB's implementing provider needs a clock interrupt to do its work, it stores the handle in dp_clockintr. The handle, if any, is established during dtpv_alloc() and disestablished during dtpv_dealloc(). Only the interval and profile providers use it at present. - In order to implement dt_prov_profile_dealloc() I needed a complete definition of struct dt_softc, so I hoisted it up out of dt_dev.c into dtvar.h. - A PCB's periodic clock interrupt, if any, is started during dt_ioctl_record_start() stopped during dt_ioctl_record_stop(). This is not a provider-agnostic approach, but I don't see where else to start/stop the clock interrupt. One alternative is to start running the clock interrupts when they are allocated in dtpv_alloc() and stop them when they are freed in dtpv_dealloc(). This is wasteful, though: the PCBs are not recording yet, so the interrupts won't perform any useful work until the controlling process enables recording. An additional pair of provider hooks, e.g. "dtpv_record_start" and "dtpv_record_stop", might resolve this. - We haven't needed to destroy clock interrupts yet, so the clockintr_disestablish() function used in this patch is new. The implementation is extremely similar to that of clockintr_cancel(). However, for sake of simplicity, a callback may not disestablish its clockintr while the callback is running. One tricky part: if a clockintr is disestablished while it is running, the dispatch thread takes care not to use-after-free when it re-enters the clockintr_queue mutex. I have tested this on amd64. It seems to work: the clock interrupts fire, stack traces are recorded, and the handles are deallocated when the process closes the file descriptor. I will be testing it on other platforms over the next few days. Thoughts? Test results? Index: kern/kern_clockintr.c =================================================================== RCS file: /cvs/src/sys/kern/kern_clockintr.c,v retrieving revision 1.32 diff -u -p -r1.32 kern_clockintr.c --- kern/kern_clockintr.c 21 Aug 2023 17:22:04 -0000 1.32 +++ kern/kern_clockintr.c 23 Aug 2023 23:35:11 -0000 @@ -218,7 +218,7 @@ clockintr_dispatch(void *frame) { uint64_t lateness, run = 0, start; struct cpu_info *ci = curcpu(); - struct clockintr *cl; + struct clockintr *cl, *shadow; struct clockintr_queue *cq = &ci->ci_queue; u_int ogen; @@ -257,22 +257,26 @@ clockintr_dispatch(void *frame) break; } clockintr_cancel_locked(cl); - cq->cq_shadow.cl_expiration = cl->cl_expiration; + shadow = &cq->cq_shadow; + shadow->cl_expiration = cl->cl_expiration; + shadow->cl_func = cl->cl_func; cq->cq_running = cl; mtx_leave(&cq->cq_mtx); - cl->cl_func(&cq->cq_shadow, frame); + shadow->cl_func(shadow, frame); mtx_enter(&cq->cq_mtx); - cq->cq_running = NULL; - if (ISSET(cl->cl_flags, CLST_IGNORE_SHADOW)) { - CLR(cl->cl_flags, CLST_IGNORE_SHADOW); - CLR(cq->cq_shadow.cl_flags, CLST_SHADOW_PENDING); - } - if (ISSET(cq->cq_shadow.cl_flags, CLST_SHADOW_PENDING)) { - CLR(cq->cq_shadow.cl_flags, CLST_SHADOW_PENDING); - clockintr_schedule_locked(cl, - cq->cq_shadow.cl_expiration); + if (cq->cq_running != NULL) { + cq->cq_running = NULL; + if (ISSET(cl->cl_flags, CLST_IGNORE_SHADOW)) { + CLR(cl->cl_flags, CLST_IGNORE_SHADOW); + CLR(shadow->cl_flags, CLST_SHADOW_PENDING); + } + if (ISSET(shadow->cl_flags, CLST_SHADOW_PENDING)) { + CLR(shadow->cl_flags, CLST_SHADOW_PENDING); + clockintr_schedule_locked(cl, + shadow->cl_expiration); + } } run++; } @@ -382,6 +386,34 @@ clockintr_cancel_locked(struct clockintr TAILQ_REMOVE(&cq->cq_pend, cl, cl_plink); CLR(cl->cl_flags, CLST_PENDING); +} + +void +clockintr_disestablish(struct clockintr *cl) +{ + struct clockintr_queue *cq = cl->cl_queue; + int was_next; + + if (cl == &cq->cq_shadow) + panic("%s: called during dispatch\n", __func__); + + mtx_enter(&cq->cq_mtx); + if (ISSET(cl->cl_flags, CLST_PENDING)) { + was_next = cl == TAILQ_FIRST(&cq->cq_pend); + clockintr_cancel_locked(cl); + if (ISSET(cq->cq_flags, CQ_INTRCLOCK)) { + if (was_next && !TAILQ_EMPTY(&cq->cq_pend)) { + if (cq == &curcpu()->ci_queue) + clockqueue_reset_intrclock(cq); + } + } + } + if (cl == cq->cq_running) + cq->cq_running = NULL; + TAILQ_REMOVE(&cq->cq_est, cl, cl_elink); + mtx_leave(&cq->cq_mtx); + + free(cl, M_DEVBUF, sizeof *cl); } struct clockintr * Index: kern/kern_clock.c =================================================================== RCS file: /cvs/src/sys/kern/kern_clock.c,v retrieving revision 1.115 diff -u -p -r1.115 kern_clock.c --- kern/kern_clock.c 23 Aug 2023 01:55:45 -0000 1.115 +++ kern/kern_clock.c 23 Aug 2023 23:35:11 -0000 @@ -49,11 +49,6 @@ #include <sys/sched.h> #include <sys/timetc.h> -#include "dt.h" -#if NDT > 0 -#include <dev/dt/dtvar.h> -#endif - /* * Clock handling routines. * @@ -114,12 +109,6 @@ initclocks(void) void hardclock(struct clockframe *frame) { -#if NDT > 0 - DT_ENTER(profile, NULL); - if (CPU_IS_PRIMARY(curcpu())) - DT_ENTER(interval, NULL); -#endif - /* * If we are not the primary CPU, we're not allowed to do * any more work. Index: sys/clockintr.h =================================================================== RCS file: /cvs/src/sys/sys/clockintr.h,v retrieving revision 1.10 diff -u -p -r1.10 clockintr.h --- sys/clockintr.h 21 Aug 2023 17:22:04 -0000 1.10 +++ sys/clockintr.h 23 Aug 2023 23:35:11 -0000 @@ -128,6 +128,7 @@ void clockintr_trigger(void); uint64_t clockintr_advance(struct clockintr *, uint64_t); void clockintr_cancel(struct clockintr *); +void clockintr_disestablish(struct clockintr *); struct clockintr *clockintr_establish(struct clockintr_queue *, void (*)(struct clockintr *, void *)); void clockintr_stagger(struct clockintr *, uint64_t, u_int, u_int); Index: dev/dt/dt_prov_profile.c =================================================================== RCS file: /cvs/src/sys/dev/dt/dt_prov_profile.c,v retrieving revision 1.5 diff -u -p -r1.5 dt_prov_profile.c --- dev/dt/dt_prov_profile.c 26 Apr 2023 16:53:59 -0000 1.5 +++ dev/dt/dt_prov_profile.c 23 Aug 2023 23:35:13 -0000 @@ -20,6 +20,7 @@ #include <sys/systm.h> #include <sys/param.h> #include <sys/atomic.h> +#include <sys/clockintr.h> #include <dev/dt/dtvar.h> @@ -31,6 +32,8 @@ struct dt_probe *dtpp_interval; /* glob int dt_prov_profile_alloc(struct dt_probe *, struct dt_softc *, struct dt_pcb_list *, struct dtioc_req *); +int dt_prov_profile_dealloc(struct dt_probe *, struct dt_softc *, + struct dtioc_req *); int dt_prov_profile_enter(struct dt_provider *, ...); int dt_prov_interval_enter(struct dt_provider *, ...); @@ -39,7 +42,7 @@ struct dt_provider dt_prov_profile = { .dtpv_alloc = dt_prov_profile_alloc, .dtpv_enter = dt_prov_profile_enter, .dtpv_leave = NULL, - .dtpv_dealloc = NULL, + .dtpv_dealloc = dt_prov_profile_dealloc, }; struct dt_provider dt_prov_interval = { @@ -47,9 +50,11 @@ struct dt_provider dt_prov_interval = { .dtpv_alloc = dt_prov_profile_alloc, .dtpv_enter = dt_prov_interval_enter, .dtpv_leave = NULL, - .dtpv_dealloc = NULL, + .dtpv_dealloc = dt_prov_profile_dealloc, }; +void dt_prov_profile_intr(struct clockintr *, void *); + int dt_prov_profile_init(void) { @@ -85,10 +90,17 @@ dt_prov_profile_alloc(struct dt_probe *d continue; dp = dt_pcb_alloc(dtp, sc); - if (dp == NULL) { - dt_pcb_purge(plist); - return ENOMEM; + if (dp == NULL) + goto purge; + + dp->dp_clockintr = clockintr_establish(&ci->ci_queue, + dt_prov_profile_intr); + if (dp->dp_clockintr == NULL) { + dt_pcb_free(dp); + goto purge; } + clockintr_stagger(dp->dp_clockintr, hardclock_period, + CPU_INFO_UNIT(ci), MAXCPUS); dp->dp_maxtick = hz / dtrq->dtrq_rate; dp->dp_cpuid = ci->ci_cpuid; @@ -99,6 +111,31 @@ dt_prov_profile_alloc(struct dt_probe *d } return 0; +purge: + TAILQ_FOREACH(dp, plist, dp_snext) + clockintr_disestablish(dp->dp_clockintr); + dt_pcb_purge(plist); + return ENOMEM; +} + +int +dt_prov_profile_dealloc(struct dt_probe *dtp, struct dt_softc *sc, + struct dtioc_req *dtrq) +{ + struct dt_pcb *dp; + + KASSERT(dtioc_req_isvalid(dtrq)); + KASSERT(dtp == dtpp_profile || dtp == dtpp_interval); + + TAILQ_FOREACH(dp, &sc->ds_pcbs, dp_snext) { + if (dp->dp_dtp == dtp) { + if (dp->dp_clockintr != NULL) { + clockintr_disestablish(dp->dp_clockintr); + dp->dp_clockintr = NULL; + } + } + } + return 0; } static inline void @@ -148,4 +185,18 @@ dt_prov_interval_enter(struct dt_provide } smr_read_leave(); return 0; +} + +void +dt_prov_profile_intr(struct clockintr *cl, void *cf) +{ + uint64_t count, i; + struct cpu_info *ci = curcpu(); + + count = clockintr_advance(cl, hardclock_period); + for (i = 0; i < count; i++) { + DT_ENTER(profile, NULL); + if (CPU_IS_PRIMARY(ci)) + DT_ENTER(interval, NULL); + } } Index: dev/dt/dt_dev.c =================================================================== RCS file: /cvs/src/sys/dev/dt/dt_dev.c,v retrieving revision 1.28 diff -u -p -r1.28 dt_dev.c --- dev/dt/dt_dev.c 14 Jul 2023 07:07:08 -0000 1.28 +++ dev/dt/dt_dev.c 23 Aug 2023 23:35:14 -0000 @@ -19,6 +19,7 @@ #include <sys/types.h> #include <sys/systm.h> #include <sys/param.h> +#include <sys/clockintr.h> #include <sys/device.h> #include <sys/exec_elf.h> #include <sys/malloc.h> @@ -82,32 +83,6 @@ #define DPRINTF(x...) /* nothing */ -/* - * Descriptor associated with each program opening /dev/dt. It is used - * to keep track of enabled PCBs. - * - * Locks used to protect struct members in this file: - * m per-softc mutex - * K kernel lock - */ -struct dt_softc { - SLIST_ENTRY(dt_softc) ds_next; /* [K] descriptor list */ - int ds_unit; /* [I] D_CLONE unique unit */ - pid_t ds_pid; /* [I] PID of tracing program */ - - struct mutex ds_mtx; - - struct dt_pcb_list ds_pcbs; /* [K] list of enabled PCBs */ - struct dt_evt *ds_bufqueue; /* [K] copy evts to userland */ - size_t ds_bufqlen; /* [K] length of the queue */ - int ds_recording; /* [K] currently recording? */ - int ds_evtcnt; /* [m] # of readable evts */ - - /* Counters */ - uint64_t ds_readevt; /* [m] # of events read */ - uint64_t ds_dropevt; /* [m] # of events dropped */ -}; - SLIST_HEAD(, dt_softc) dtdev_list; /* [K] list of open /dev/dt nodes */ /* @@ -486,6 +461,14 @@ dt_ioctl_record_start(struct dt_softc *s SMR_SLIST_INSERT_HEAD_LOCKED(&dtp->dtp_pcbs, dp, dp_pnext); dtp->dtp_recording++; dtp->dtp_prov->dtpv_recording++; + + /* + * XXX Not provider-agnostic. Is there a better place + * to do this? + */ + if (dp->dp_clockintr != NULL) + clockintr_advance(dp->dp_clockintr, hardclock_period); + } rw_exit_write(&dt_lock); @@ -514,6 +497,11 @@ dt_ioctl_record_stop(struct dt_softc *sc dtp->dtp_recording--; dtp->dtp_prov->dtpv_recording--; + + /* XXX This is not provider-agnostic. */ + if (dp->dp_clockintr != NULL) + clockintr_cancel(dp->dp_clockintr); + SMR_SLIST_REMOVE_LOCKED(&dtp->dtp_pcbs, dp, dt_pcb, dp_pnext); } rw_exit_write(&dt_lock); Index: dev/dt/dtvar.h =================================================================== RCS file: /cvs/src/sys/dev/dt/dtvar.h,v retrieving revision 1.17 diff -u -p -r1.17 dtvar.h --- dev/dt/dtvar.h 26 Apr 2023 16:53:59 -0000 1.17 +++ dev/dt/dtvar.h 23 Aug 2023 23:35:14 -0000 @@ -158,12 +158,39 @@ struct dtioc_getaux { #include <sys/queue.h> #include <sys/smr.h> +struct dt_pcb; +TAILQ_HEAD(dt_pcb_list, dt_pcb); + /* Flags that make sense for all providers. */ #define DTEVT_COMMON (DTEVT_EXECNAME|DTEVT_KSTACK|DTEVT_USTACK) #define M_DT M_DEVBUF /* XXX FIXME */ -struct dt_softc; +/* + * Descriptor associated with each program opening /dev/dt. It is used + * to keep track of enabled PCBs. + * + * Locks used to protect struct members in this file: + * m per-softc mutex + * K kernel lock + */ +struct dt_softc { + SLIST_ENTRY(dt_softc) ds_next; /* [K] descriptor list */ + int ds_unit; /* [I] D_CLONE unique unit */ + pid_t ds_pid; /* [I] PID of tracing program */ + + struct mutex ds_mtx; + + struct dt_pcb_list ds_pcbs; /* [K] list of enabled PCBs */ + struct dt_evt *ds_bufqueue; /* [K] copy evts to userland */ + size_t ds_bufqlen; /* [K] length of the queue */ + int ds_recording; /* [K] currently recording? */ + int ds_evtcnt; /* [m] # of readable evts */ + + /* Counters */ + uint64_t ds_readevt; /* [m] # of events read */ + uint64_t ds_dropevt; /* [m] # of events dropped */ +}; int dtioc_req_isvalid(struct dtioc_req *); @@ -197,6 +224,7 @@ struct dt_pcb { struct dt_filter dp_filter; /* [I] filter to match */ /* Provider specific fields. */ + struct clockintr *dp_clockintr; /* [?] clock interrupt handle */ unsigned int dp_cpuid; /* [I] on which CPU */ unsigned int dp_maxtick; /* [I] freq. of profiling */ unsigned int dp_nticks; /* [c] current tick count */ @@ -204,8 +232,6 @@ struct dt_pcb { /* Counters */ uint64_t dp_dropevt; /* [m] # dropped event */ }; - -TAILQ_HEAD(dt_pcb_list, dt_pcb); struct dt_pcb *dt_pcb_alloc(struct dt_probe *, struct dt_softc *); void dt_pcb_free(struct dt_pcb *);