On Wed, Mar 23, 2022 at 06:13:27PM +0100, Alexander Bluhm wrote: > In my opinion tracepoints give insight at minimal cost. It is worth > it to have it in GENERIC to make it easy to use.
After release I want to revive the btrace of refcounts discussion. As mpi@ mentioned the idea of dt(4) is to have these trace points in GENERIC kernel. If you want to hunt a bug, just turn it on. Refounting is a common place for bugs, leaks can be detected easily. The alternative are some defines that you can compile in and access from ddb. This is more work and you would have to implement it for every recount. https://marc.info/?l=openbsd-tech&m=163786435916039&w=2 There is no measuarable performance difference. dt(4) is written in a way that is is only one additional branch. At least my goal is to add trace points to useful places when we identify them. ok? bluhm Index: dev/dt/dt_prov_static.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/dev/dt/dt_prov_static.c,v retrieving revision 1.13 diff -u -p -r1.13 dt_prov_static.c --- dev/dt/dt_prov_static.c 17 Mar 2022 14:53:59 -0000 1.13 +++ dev/dt/dt_prov_static.c 7 Apr 2022 17:32:23 -0000 @@ -87,6 +87,12 @@ DT_STATIC_PROBE1(smr, barrier_exit, "int DT_STATIC_PROBE0(smr, wakeup); DT_STATIC_PROBE2(smr, thread, "uint64_t", "uint64_t"); +/* + * reference counting + */ +DT_STATIC_PROBE0(refcnt, none); +DT_STATIC_PROBE3(refcnt, inpcb, "void *", "int", "int"); +DT_STATIC_PROBE3(refcnt, tdb, "void *", "int", "int"); /* * List of all static probes @@ -127,15 +133,24 @@ struct dt_probe *const dtps_static[] = { &_DT_STATIC_P(smr, barrier_exit), &_DT_STATIC_P(smr, wakeup), &_DT_STATIC_P(smr, thread), + /* refcnt */ + &_DT_STATIC_P(refcnt, none), + &_DT_STATIC_P(refcnt, inpcb), + &_DT_STATIC_P(refcnt, tdb), }; +struct dt_probe *const *dtps_index_refcnt; + int dt_prov_static_init(void) { int i; - for (i = 0; i < nitems(dtps_static); i++) + for (i = 0; i < nitems(dtps_static); i++) { + if (dtps_static[i] == &_DT_STATIC_P(refcnt, none)) + dtps_index_refcnt = &dtps_static[i]; dt_dev_register_probe(dtps_static[i]); + } return i; } Index: dev/dt/dtvar.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/dev/dt/dtvar.h,v retrieving revision 1.13 diff -u -p -r1.13 dtvar.h --- dev/dt/dtvar.h 27 Feb 2022 10:14:01 -0000 1.13 +++ dev/dt/dtvar.h 7 Apr 2022 17:41:55 -0000 @@ -313,11 +313,30 @@ extern volatile uint32_t dt_tracing; /* #define DT_STATIC_ENTER(func, name, args...) do { \ extern struct dt_probe _DT_STATIC_P(func, name); \ struct dt_probe *dtp = &_DT_STATIC_P(func, name); \ - struct dt_provider *dtpv = dtp->dtp_prov; \ \ if (__predict_false(dt_tracing) && \ __predict_false(dtp->dtp_recording)) { \ + struct dt_provider *dtpv = dtp->dtp_prov; \ + \ dtpv->dtpv_enter(dtpv, dtp, args); \ + } \ +} while (0) + +#define _DT_INDEX_P(func) (dtps_index_##func) + +#define DT_INDEX_ENTER(func, index, args...) do { \ + extern struct dt_probe **_DT_INDEX_P(func); \ + \ + if (__predict_false(index > 0) && \ + __predict_false(dt_tracing) && \ + __predict_true(_DT_INDEX_P(func) != NULL)) { \ + struct dt_probe *dtp = _DT_INDEX_P(func)[index]; \ + \ + if(__predict_false(dtp->dtp_recording)) { \ + struct dt_provider *dtpv = dtp->dtp_prov; \ + \ + dtpv->dtpv_enter(dtpv, dtp, args); \ + } \ } \ } while (0) Index: kern/kern_synch.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_synch.c,v retrieving revision 1.185 diff -u -p -r1.185 kern_synch.c --- kern/kern_synch.c 18 Mar 2022 15:32:06 -0000 1.185 +++ kern/kern_synch.c 7 Apr 2022 16:25:45 -0000 @@ -804,7 +804,15 @@ sys___thrwakeup(struct proc *p, void *v, void refcnt_init(struct refcnt *r) { + refcnt_init_trace(r, 0); +} + +void +refcnt_init_trace(struct refcnt *r, int idx) +{ + r->r_traceidx = idx; atomic_store_int(&r->r_refs, 1); + TRACEINDEX(refcnt, r->r_traceidx, r, 0, +1); } void @@ -814,6 +822,7 @@ refcnt_take(struct refcnt *r) refs = atomic_inc_int_nv(&r->r_refs); KASSERT(refs != 0); + TRACEINDEX(refcnt, r->r_traceidx, r, refs - 1, +1); (void)refs; } @@ -824,6 +833,7 @@ refcnt_rele(struct refcnt *r) refs = atomic_dec_int_nv(&r->r_refs); KASSERT(refs != ~0); + TRACEINDEX(refcnt, r->r_traceidx, r, refs + 1, -1); return (refs == 0); } @@ -842,11 +852,13 @@ refcnt_finalize(struct refcnt *r, const refs = atomic_dec_int_nv(&r->r_refs); KASSERT(refs != ~0); + TRACEINDEX(refcnt, r->r_traceidx, r, refs + 1, -1); while (refs) { sleep_setup(&sls, r, PWAIT, wmesg, 0); refs = atomic_load_int(&r->r_refs); sleep_finish(&sls, refs); } + TRACEINDEX(refcnt, r->r_traceidx, r, refs, 0); } int @@ -855,6 +867,7 @@ refcnt_shared(struct refcnt *r) u_int refs; refs = atomic_load_int(&r->r_refs); + TRACEINDEX(refcnt, r->r_traceidx, r, refs, 0); return (refs > 1); } @@ -864,6 +877,7 @@ refcnt_read(struct refcnt *r) u_int refs; refs = atomic_load_int(&r->r_refs); + TRACEINDEX(refcnt, r->r_traceidx, r, refs, 0); return (refs); } Index: netinet/in_pcb.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/in_pcb.c,v retrieving revision 1.264 diff -u -p -r1.264 in_pcb.c --- netinet/in_pcb.c 22 Mar 2022 18:02:54 -0000 1.264 +++ netinet/in_pcb.c 7 Apr 2022 16:25:45 -0000 @@ -235,7 +235,7 @@ in_pcballoc(struct socket *so, struct in return (ENOBUFS); inp->inp_table = table; inp->inp_socket = so; - refcnt_init(&inp->inp_refcnt); + refcnt_init_trace(&inp->inp_refcnt, DT_REFCNT_IDX_INPCB); inp->inp_seclevel[SL_AUTH] = IPSEC_AUTH_LEVEL_DEFAULT; inp->inp_seclevel[SL_ESP_TRANS] = IPSEC_ESP_TRANS_LEVEL_DEFAULT; inp->inp_seclevel[SL_ESP_NETWORK] = IPSEC_ESP_NETWORK_LEVEL_DEFAULT; Index: netinet/ip_ipsp.c =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_ipsp.c,v retrieving revision 1.269 diff -u -p -r1.269 ip_ipsp.c --- netinet/ip_ipsp.c 10 Mar 2022 15:21:08 -0000 1.269 +++ netinet/ip_ipsp.c 7 Apr 2022 16:25:45 -0000 @@ -1048,7 +1048,7 @@ tdb_alloc(u_int rdomain) tdbp = pool_get(&tdb_pool, PR_WAITOK | PR_ZERO); - refcnt_init(&tdbp->tdb_refcnt); + refcnt_init_trace(&tdbp->tdb_refcnt, DT_REFCNT_IDX_TDB); mtx_init(&tdbp->tdb_mtx, IPL_SOFTNET); TAILQ_INIT(&tdbp->tdb_policy_head); Index: sys/refcnt.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/sys/refcnt.h,v retrieving revision 1.6 diff -u -p -r1.6 refcnt.h --- sys/refcnt.h 16 Mar 2022 14:13:01 -0000 1.6 +++ sys/refcnt.h 7 Apr 2022 16:25:45 -0000 @@ -21,24 +21,30 @@ /* * Locks used to protect struct members in this file: + * I immutable after creation * a atomic operations */ struct refcnt { unsigned int r_refs; /* [a] reference counter */ + int r_traceidx; /* [I] index for dt(4) tracing */ }; -#define REFCNT_INITIALIZER() { .r_refs = 1 } +#define REFCNT_INITIALIZER() { .r_refs = 1, .r_traceidx = 0 } #ifdef _KERNEL void refcnt_init(struct refcnt *); +void refcnt_init_trace(struct refcnt *, int id); void refcnt_take(struct refcnt *); int refcnt_rele(struct refcnt *); void refcnt_rele_wake(struct refcnt *); void refcnt_finalize(struct refcnt *, const char *); int refcnt_shared(struct refcnt *); unsigned int refcnt_read(struct refcnt *); + +#define DT_REFCNT_IDX_INPCB 1 +#define DT_REFCNT_IDX_TDB 2 #endif /* _KERNEL */ Index: sys/tracepoint.h =================================================================== RCS file: /data/mirror/openbsd/cvs/src/sys/sys/tracepoint.h,v retrieving revision 1.1 diff -u -p -r1.1 tracepoint.h --- sys/tracepoint.h 21 Jan 2020 16:16:23 -0000 1.1 +++ sys/tracepoint.h 7 Apr 2022 16:25:45 -0000 @@ -25,11 +25,13 @@ #if NDT > 0 #include <dev/dt/dtvar.h> -#define TRACEPOINT(func, name, args...) DT_STATIC_ENTER(func, name, args) +#define TRACEPOINT(func, name, args...) DT_STATIC_ENTER(func, name, args) +#define TRACEINDEX(func, index, args...) DT_INDEX_ENTER(func, index, args) #else /* NDT > 0 */ -#define TRACEPOINT(func, name, args...) +#define TRACEPOINT(func, name, args...) +#define TRACEINDEX(func, index, args...) #endif /* NDT > 0 */ #endif /* _KERNEL */
