On Thu, Mar 17, 2022 at 06:16:51PM +0100, Alexander Bluhm wrote:
> On Thu, Mar 17, 2022 at 07:25:27AM +0000, Visa Hankala wrote:
> > On Thu, Mar 17, 2022 at 12:42:13AM +0100, Alexander Bluhm wrote:
> > > I would like to use btrace to debug refernce counting. The idea
> > > is to a a tracepoint for every type of refcnt we have. When it
> > > changes, print the actual object, the current counter and the change
> > > value.
> >
> > > Do we want that feature?
> >
> > I am against this in its current form. The code would become more
> > complex, and the trace points can affect timing. There is a risk that
> > the kernel behaves slightly differently when dt has been compiled in.
>
> Can we get in this part then?
>
> - Remove DIAGNOSTIC to keep similar in non DIAGNOSTIC case.
> - Rename refcnt to refs. refcnt is the struct, refs contains the
> r_refs value.
> - Add KASSERT(refs != ~0) in refcnt_finalize().
> - Always use u_int refs so I can insert my btrace diff easily.
I think this is fine.
OK visa@
> Index: kern/kern_synch.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_synch.c,v
> retrieving revision 1.184
> diff -u -p -r1.184 kern_synch.c
> --- kern/kern_synch.c 16 Mar 2022 14:13:01 -0000 1.184
> +++ kern/kern_synch.c 17 Mar 2022 16:12:50 -0000
> @@ -810,25 +810,21 @@ refcnt_init(struct refcnt *r)
> void
> refcnt_take(struct refcnt *r)
> {
> -#ifdef DIAGNOSTIC
> - u_int refcnt;
> + u_int refs;
>
> - refcnt = atomic_inc_int_nv(&r->r_refs);
> - KASSERT(refcnt != 0);
> -#else
> - atomic_inc_int(&r->r_refs);
> -#endif
> + refs = atomic_inc_int_nv(&r->r_refs);
> + KASSERT(refs != 0);
> + (void)refs;
> }
>
> int
> refcnt_rele(struct refcnt *r)
> {
> - u_int refcnt;
> + u_int refs;
>
> - refcnt = atomic_dec_int_nv(&r->r_refs);
> - KASSERT(refcnt != ~0);
> -
> - return (refcnt == 0);
> + refs = atomic_dec_int_nv(&r->r_refs);
> + KASSERT(refs != ~0);
> + return (refs == 0);
> }
>
> void
> @@ -842,26 +838,33 @@ void
> refcnt_finalize(struct refcnt *r, const char *wmesg)
> {
> struct sleep_state sls;
> - u_int refcnt;
> + u_int refs;
>
> - refcnt = atomic_dec_int_nv(&r->r_refs);
> - while (refcnt) {
> + refs = atomic_dec_int_nv(&r->r_refs);
> + KASSERT(refs != ~0);
> + while (refs) {
> sleep_setup(&sls, r, PWAIT, wmesg, 0);
> - refcnt = atomic_load_int(&r->r_refs);
> - sleep_finish(&sls, refcnt);
> + refs = atomic_load_int(&r->r_refs);
> + sleep_finish(&sls, refs);
> }
> }
>
> int
> refcnt_shared(struct refcnt *r)
> {
> - return (atomic_load_int(&r->r_refs) > 1);
> + u_int refs;
> +
> + refs = atomic_load_int(&r->r_refs);
> + return (refs > 1);
> }
>
> unsigned int
> refcnt_read(struct refcnt *r)
> {
> - return (atomic_load_int(&r->r_refs));
> + u_int refs;
> +
> + refs = atomic_load_int(&r->r_refs);
> + return (refs);
> }
>
> void
>
