On Fri, Apr 22, 2022 at 12:07:44AM +0200, Alexander Bluhm wrote:
> I still think it is worth to have refcount debugging in generic
> kernel dt(4). Having tools is easier than first adding printf to
> hunt a bug. I see no downside.
I have talked with mpi@. We both think it is useful to have refcount
debugging tools available in GENERIC kernel. If there are no hard
objections, I will put this in tomorrow.
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 21 Apr 2022 21:06:03 -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 21 Apr 2022 21:06:03 -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(dt_tracing) && \
> + __predict_false(index > 0) && \
> + __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 21 Apr 2022 21:06:03 -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.265
> diff -u -p -r1.265 in_pcb.c
> --- netinet/in_pcb.c 14 Apr 2022 14:10:22 -0000 1.265
> +++ netinet/in_pcb.c 21 Apr 2022 21:06:03 -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 21 Apr 2022 21:06:03 -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 21 Apr 2022 21:06:03 -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 21 Apr 2022 21:06:03 -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 */
