Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 27 Jul 2012 22:32:55 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Gleb Smirnoff <glebius@FreeBSD.org>
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:  <20120727221529.K7360@besplex.bde.org>
In-Reply-To: <20120727111904.GQ14135@FreeBSD.org>
References:  <201207270916.q6R9Gm23086648@svn.freebsd.org> <20120727111237.GC2676@deviant.kiev.zoral.com.ua> <20120727111904.GQ14135@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 27 Jul 2012, Gleb Smirnoff wrote:

> On Fri, Jul 27, 2012 at 02:12:37PM +0300, Konstantin Belousov wrote:
> K> On Fri, Jul 27, 2012 at 09:16:48AM +0000, Gleb Smirnoff wrote:
> K> > ...
> K> > Log:
> K> >   Add assertion for refcount overflow.
> K> >
> K> >   Submitted by:	Andrey Zonov <andrey zonov.org>
> K> >   Reviewed by:	kib
> K> It was discussed rather then reviewed.
> K>
> K> I suggest that the assert may be expressed as a check after the increment,
> K> which verifies that counter is != 0. This allows to avoid namespace
> K> pollution due to limits.h.
>
> Hmm, overflowing unsigned is a defined behavior in C. If Bruce agrees,
> then I'm happy with KASSERT after increment.

Comparing with (uint_t)-1 before is equivalent.  You can even omit the
cast (depend on the default promotion).

I just noticed that there is a technical problem -- the count is read
unlocked in the KASSERT.  And since the comparision is for equality,
if you lose the race reading the count when it reaches the overflow
threshold, then you won't see it overflow unless it wraps again and
you win the race next time (or later).  atomic_cmpset could be used
to clamp the value at the max, but that is too much for an assertion.

Simple locked reads of the count also don't prevent it wrapping and
going a bit higher than 0 with increments by other CPUs before the
CPU that notices the overflow can panic.  So the patch in the PR may
have been better than the one committed (IIRC, it paniced some
time before wrapping, and people didn't like this).

I prefer to use signed types, even for, or especially for counters.
Then if the counter overflows you have a long time to notice this,
and may notice without explicit testing because negative counts are
printed somewhere.  Integer overflow gives undefined behaviour
immediately, and there is a compiler flag to generate tests for it.
No one ever uses this, and it wouldn't work for variables that need
atomic accesses anyway.

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120727221529.K7360>