Date: Fri, 27 Jul 2012 16:45:34 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: Konstantin Belousov <kostikbel@gmail.com>, svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r238828 - head/sys/sys Message-ID: <20120727124534.GT14135@FreeBSD.org> In-Reply-To: <20120727221529.K7360@besplex.bde.org> References: <201207270916.q6R9Gm23086648@svn.freebsd.org> <20120727111237.GC2676@deviant.kiev.zoral.com.ua> <20120727111904.GQ14135@FreeBSD.org> <20120727221529.K7360@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jul 27, 2012 at 10:32:55PM +1000, Bruce Evans wrote:
B> > On Fri, Jul 27, 2012 at 02:12:37PM +0300, Konstantin Belousov wrote:
B> > K> On Fri, Jul 27, 2012 at 09:16:48AM +0000, Gleb Smirnoff wrote:
B> > K> > ...
B> > K> > Log:
B> > K> > Add assertion for refcount overflow.
B> > K> >
B> > K> > Submitted by: Andrey Zonov <andrey zonov.org>
B> > K> > Reviewed by: kib
B> > K> It was discussed rather then reviewed.
B> > K>
B> > K> I suggest that the assert may be expressed as a check after the increment,
B> > K> which verifies that counter is != 0. This allows to avoid namespace
B> > K> pollution due to limits.h.
B> >
B> > Hmm, overflowing unsigned is a defined behavior in C. If Bruce agrees,
B> > then I'm happy with KASSERT after increment.
B>
B> Comparing with (uint_t)-1 before is equivalent. You can even omit the
B> cast (depend on the default promotion).
B>
B> I just noticed that there is a technical problem -- the count is read
B> unlocked in the KASSERT. And since the comparision is for equality,
B> if you lose the race reading the count when it reaches the overflow
B> threshold, then you won't see it overflow unless it wraps again and
B> you win the race next time (or later). atomic_cmpset could be used
B> to clamp the value at the max, but that is too much for an assertion.
We have discussed that. As alternative I proposed:
@@ -50,8 +51,14 @@
static __inline void
refcount_acquire(volatile u_int *count)
{
+#ifdef INVARIANTS
+ u_int old;
+ old = atomic_fetchadd_int(count, 1);
+ KASSERT(old < UINT_MAX, ("refcount %p overflowed", count));
+#else
atomic_add_acq_int(count, 1);
+#endif
}
Konstantin didn't like that production code differs from INVARIANTS.
So we ended with what I committed, advocating to the fact that although
assertion is racy and bad panics still can occur, the "good panics"
would occur much more often, and a single "good panic" is enough to
show what's going on.
--
Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120727124534.GT14135>