Re: Debugging function of localcount

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

> Date: Mon, 7 Aug 2017 13:08:35 +0900
> From: Ryota Ozaki <ozaki-r%netbsd.org@localhost>
> 
> Anyway I've brought another (stupid) approach that uses an atomic
> counter:
>   http://www.netbsd.org/~ozaki-r/localcount_debug_atomic.diff
> 
> It counts up a counter with atomic instructions along with localcount
> if LOCKDEBUG enabled. Of course it blows the merit of localcount but
> I can provide an accurate view of the counter.
> 
> Do you think that it works and is useful?

That looks plausible.  I'd put it under DEBUG && LOCKDEBUG, and
require localcount_debug_refcnt to be used only in KDASSERT, to avoid
adding yet more contention to plain LOCKDEBUG, and to make it safe to
write, e.g.,

	KDASSERT(localcount_debug_refcnt(lc) >= 2).

Otherwise, under, e.g., DIAGNOSTIC && !LOCKDEBUG,

	KASSERT(localcount_debug_refcnt(lc) >= 2)

would always fail, since localcount_debug_refcnt always returns 0.

Tiny nit: I suggest you always use uint32_t for this, and use
atomic_dec_32_nv(&lc->lc_refcnt) == UINT_MAX to detect underflow,
rather than conversion to int32_t.  Maybe check-and-panic on overflow
too: if atomic_inc_32_nv(&lc->lc_refcnt) == 0, panic.

Problem: The ABI is different with and without LOCKDEBUG.  The
structure member can't be conditional.  I don't see a problem with
changing always adding another 32-bit (or even 64-bit) member to
struct localcount -- they already require O(nobj * ncpu) memory, so an
extra handful of bytes per object is inconsequential -- so I'd just
make it unconditional.

No need for #include <sys/atomic.h> in localcount.h.  lc_refcnt member
should be volatile-qualified.