From: Claudio Jeker Subject: Re: dt: Trace tracing program To: Christian Ludwig , "tech@openbsd.org" Date: Sat, 30 Mar 2024 13:15:29 +0100 On Sat, Mar 30, 2024 at 09:22:59AM +0100, Martin Pieuchot wrote: > On 16/02/24(Fri) 15:21, Christian Ludwig wrote: > > Hi, > > > > sorry for the late reply. > > > > On Mon, 2024-02-05 at 16:14 +0100, Martin Pieuchot wrote: > > > On 02/02/24(Fri) 16:24, Christian Ludwig wrote: > > > > For the sake of visibility, also trace the tracing program itself. > > > > If you really wanted to hide it, add a filter to your probes, like: > > > > > > I'm not sure about that.  Why do you want to see the tracing program? > > > What are you trying to monitor? > > > > I am trying to monitor interrupt events using the TRACEPOINT macro. But > > in the current form, the TRACEPOINT macro simply ignores the tracing > > program, which leads to seemingly "lost" interrupts. Hence the diff. > > > > I think it's not worth adding another macro, TRACEPOINT does everything > > I need, once it's adapted to be safe to call in all interrupt contexts. > > I am working on that. > > > > OTOH exposing the tracing program also makes it easier to better > > connect the dots concering side effects introduced by the tracer. And > > if you really want to hide it you can use a filter explicitly. > > I believe now is the right time to do this change. I'd like commit the > more complete diff below which completely remove kernel filtering. > Kernel filtering was used in the early days of dt(4) but now all > filtering is done is userland. > > ok? But this adds a lot of extra code to drop unwanted messages and as a result you may drop messages because the ring overflowed. So I'm not sure if this is the right move. > Index: dev/dt/dt_dev.c > =================================================================== > RCS file: /cvs/src/sys/dev/dt/dt_dev.c,v > diff -u -p -r1.32 dt_dev.c > --- dev/dt/dt_dev.c 29 Feb 2024 00:18:48 -0000 1.32 > +++ dev/dt/dt_dev.c 30 Mar 2024 08:13:08 -0000 > @@ -348,30 +348,6 @@ dtlookup(int unit) > } > > int > -dtioc_req_isvalid(struct dtioc_req *dtrq) > -{ > - switch (dtrq->dtrq_filter.dtf_operand) { > - case DT_OP_NONE: > - case DT_OP_EQ: > - case DT_OP_NE: > - break; > - default: > - return 0; > - } > - > - switch (dtrq->dtrq_filter.dtf_variable) { > - case DT_FV_NONE: > - case DT_FV_PID: > - case DT_FV_TID: > - break; > - default: > - return 0; > - } > - > - return 1; > -} > - > -int > dt_ioctl_list_probes(struct dt_softc *sc, struct dtioc_probe *dtpr) > { > struct dtioc_probe_info info, *dtpi; > @@ -552,9 +528,6 @@ dt_ioctl_probe_enable(struct dt_softc *s > struct dt_probe *dtp; > int error; > > - if (!dtioc_req_isvalid(dtrq)) > - return EINVAL; > - > SIMPLEQ_FOREACH(dtp, &dt_probe_list, dtp_next) { > if (dtp->dtp_pbn == dtrq->dtrq_pbn) > break; > @@ -582,9 +555,6 @@ dt_ioctl_probe_disable(struct dt_softc * > struct dt_probe *dtp; > int error; > > - if (!dtioc_req_isvalid(dtrq)) > - return EINVAL; > - > SIMPLEQ_FOREACH(dtp, &dt_probe_list, dtp_next) { > if (dtp->dtp_pbn == dtrq->dtrq_pbn) > break; > @@ -711,48 +681,6 @@ dt_pcb_purge(struct dt_pcb_list *plist) > } > } > > -int > -dt_pcb_filter(struct dt_pcb *dp) > -{ > - struct dt_filter *dtf = &dp->dp_filter; > - struct proc *p = curproc; > - unsigned int var = 0; > - int match = 1; > - > - /* Filter out tracing program. */ > - if (dp->dp_sc->ds_pid == p->p_p->ps_pid) > - return 1; > - > - switch (dtf->dtf_variable) { > - case DT_FV_PID: > - var = p->p_p->ps_pid; > - break; > - case DT_FV_TID: > - var = p->p_tid + THREAD_PID_OFFSET; > - break; > - case DT_FV_NONE: > - break; > - default: > - KASSERT(0); > - } > - > - switch (dtf->dtf_operand) { > - case DT_OP_EQ: > - match = !!(var == dtf->dtf_value); > - break; > - case DT_OP_NE: > - match = !!(var != dtf->dtf_value); > - break; > - case DT_OP_NONE: > - break; > - default: > - KASSERT(0); > - } > - > - return !match; > -} > - > - > /* > * Get a reference to the next free event state from the ring. > */ > @@ -762,9 +690,6 @@ dt_pcb_ring_get(struct dt_pcb *dp, int p > struct proc *p = curproc; > struct dt_evt *dtev; > int distance; > - > - if (dt_pcb_filter(dp)) > - return NULL; > > mtx_enter(&dp->dp_mtx); > distance = dp->dp_prod - dp->dp_cons; > Index: dev/dt/dt_prov_profile.c > =================================================================== > RCS file: /cvs/src/sys/dev/dt/dt_prov_profile.c,v > diff -u -p -r1.7 dt_prov_profile.c > --- dev/dt/dt_prov_profile.c 13 Mar 2024 13:13:57 -0000 1.7 > +++ dev/dt/dt_prov_profile.c 30 Mar 2024 08:19:15 -0000 > @@ -72,7 +72,6 @@ dt_prov_profile_alloc(struct dt_probe *d > CPU_INFO_ITERATOR cii; > extern int hz; > > - KASSERT(dtioc_req_isvalid(dtrq)); > KASSERT(TAILQ_EMPTY(plist)); > KASSERT(dtp == dtpp_profile || dtp == dtpp_interval); > > @@ -92,7 +91,6 @@ dt_prov_profile_alloc(struct dt_probe *d > dp->dp_nsecs = SEC_TO_NSEC(1) / dtrq->dtrq_rate; > dp->dp_cpu = ci; > > - dp->dp_filter = dtrq->dtrq_filter; > dp->dp_evtflags = dtrq->dtrq_evtflags & DTEVT_PROV_PROFILE; > TAILQ_INSERT_HEAD(plist, dp, dp_snext); > } > Index: dev/dt/dt_prov_static.c > =================================================================== > RCS file: /cvs/src/sys/dev/dt/dt_prov_static.c,v > diff -u -p -r1.22 dt_prov_static.c > --- dev/dt/dt_prov_static.c 28 Aug 2023 14:50:01 -0000 1.22 > +++ dev/dt/dt_prov_static.c 30 Mar 2024 08:19:00 -0000 > @@ -179,14 +179,12 @@ dt_prov_static_alloc(struct dt_probe *dt > { > struct dt_pcb *dp; > > - KASSERT(dtioc_req_isvalid(dtrq)); > KASSERT(TAILQ_EMPTY(plist)); > > dp = dt_pcb_alloc(dtp, sc); > if (dp == NULL) > return ENOMEM; > > - dp->dp_filter = dtrq->dtrq_filter; > dp->dp_evtflags = dtrq->dtrq_evtflags; > TAILQ_INSERT_HEAD(plist, dp, dp_snext); > > Index: dev/dt/dt_prov_syscall.c > =================================================================== > RCS file: /cvs/src/sys/dev/dt/dt_prov_syscall.c,v > diff -u -p -r1.8 dt_prov_syscall.c > --- dev/dt/dt_prov_syscall.c 13 Apr 2023 02:19:05 -0000 1.8 > +++ dev/dt/dt_prov_syscall.c 30 Mar 2024 08:19:29 -0000 > @@ -101,7 +101,6 @@ dt_prov_syscall_alloc(struct dt_probe *d > { > struct dt_pcb *dp; > > - KASSERT(dtioc_req_isvalid(dtrq)); > KASSERT(TAILQ_EMPTY(plist)); > KASSERT(dtp->dtp_prov == &dt_prov_syscall); > KASSERT((dtp->dtp_sysnum >= 0) && (dtp->dtp_sysnum < dtps_nsysent)); > @@ -110,7 +109,6 @@ dt_prov_syscall_alloc(struct dt_probe *d > if (dp == NULL) > return ENOMEM; > > - dp->dp_filter = dtrq->dtrq_filter; > dp->dp_evtflags = dtrq->dtrq_evtflags & DTEVT_PROV_SYSCALL; > TAILQ_INSERT_HEAD(plist, dp, dp_snext); > > Index: dev/dt/dtvar.h > =================================================================== > RCS file: /cvs/src/sys/dev/dt/dtvar.h,v > diff -u -p -r1.18 dtvar.h > --- dev/dt/dtvar.h 9 Feb 2024 17:42:18 -0000 1.18 > +++ dev/dt/dtvar.h 30 Mar 2024 08:15:56 -0000 > @@ -82,26 +82,6 @@ struct dt_evt { > "\003KSTACK" \ > "\004FUNCARGS" \ > > -/* > - * Each PCB can have a filter attached to itself. A filter do not > - * prevent an enabled probe to fire, but when that happens, event > - * states are only recorded if it is matched. > - */ > -struct dt_filter { > - enum dt_operand { > - DT_OP_NONE = 0, > - DT_OP_EQ, > - DT_OP_NE, > - } dtf_operand; > - enum dt_filtervar { > - DT_FV_NONE = 0, > - DT_FV_PID, > - DT_FV_TID, > - } dtf_variable /* what should be filtered */; > - unsigned int dtf_value; /* PID or TID to filter */ > -}; > - > - > struct dtioc_probe_info { > uint32_t dtpi_pbn; /* probe number */ > uint8_t dtpi_nargs; /* # of arguments */ > @@ -129,7 +109,6 @@ struct dtioc_arg { > > struct dtioc_req { > uint32_t dtrq_pbn; /* probe number */ > - struct dt_filter dtrq_filter; /* probe filter */ > uint32_t dtrq_rate; /* number of ticks */ > uint64_t dtrq_evtflags; /* states to record */ > }; > @@ -165,8 +144,6 @@ struct dtioc_getaux { > > struct dt_softc; > > -int dtioc_req_isvalid(struct dtioc_req *); > - > /* > * Probe control block, possibly per-CPU. > * > @@ -195,7 +172,6 @@ struct dt_pcb { > struct dt_softc *dp_sc; /* [I] related softc */ > struct dt_probe *dp_dtp; /* [I] related probe */ > uint64_t dp_evtflags; /* [I] event states to record */ > - struct dt_filter dp_filter; /* [I] filter to match */ > > /* Provider specific fields. */ > struct clockintr dp_clockintr; /* [D] profiling handle */ > @@ -211,7 +187,6 @@ 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 *); > void dt_pcb_purge(struct dt_pcb_list *); > -int dt_pcb_filter(struct dt_pcb *); > > struct dt_evt *dt_pcb_ring_get(struct dt_pcb *, int); > void dt_pcb_ring_consume(struct dt_pcb *, struct dt_evt *); > -- :wq Claudio