Date: Sat, 19 Sep 2015 21:55:44 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: Bruce Evans <brde@optusnet.com.au>, freebsd-arch@freebsd.org, Konstantin Belousov <kostikbel@gmail.com>, jhb@freebsd.org Subject: Re: new api for reference counters (was: atomic v_usecount and v_holdcnt) Message-ID: <20150919205230.F3483@besplex.bde.org> In-Reply-To: <20150919092355.GA14906@dft-labs.eu> References: <20141122211147.GA23623@dft-labs.eu> <20141124095251.GH17068@kib.kiev.ua> <20150314225226.GA15302@dft-labs.eu> <20150316094643.GZ2379@kib.kiev.ua> <20150317014412.GA10819@dft-labs.eu> <20150318104442.GS2379@kib.kiev.ua> <20150625123156.GA29667@dft-labs.eu> <20150626042546.Q2820@besplex.bde.org> <20150625195352.GB1042@brick.home> <20150702213048.GB7666@dft-labs.eu> <20150919092355.GA14906@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 19 Sep 2015, Mateusz Guzik wrote:
> Can I get some flames on this one?
:-).
> On Thu, Jul 02, 2015 at 11:30:49PM +0200, Mateusz Guzik wrote:
>> On Thu, Jun 25, 2015 at 09:53:52PM +0200, Edward Tomasz Napiera=C5=82a w=
rote:
>>> On 0626T0429, Bruce Evans wrote:
>>>> On Thu, 25 Jun 2015, Mateusz Guzik wrote:
>>>>
>>>>> On Wed, Mar 18, 2015 at 12:44:42PM +0200, Konstantin Belousov wrote:
>>>>>> On Tue, Mar 17, 2015 at 02:44:12AM +0100, Mateusz Guzik wrote:
>>>>
>>>>>>> I replaced them with refcount_acquire_if_not_zero and
>>>>>>> refcount_release_if_not_last.
>>>>>> I dislike the length of the names. Can you propose something shorte=
r ?
>>>>>
>>>>> Unfortunately the original API is alreday quite verbose and I don't h=
ave
>>>>> anything readable which would retain "refcount_acquire" (instead of a
>>>>> "ref_get" or "ref_acq"). Adding "_nz" as a suffix does not look good
>>>>> ("refcount_acquire_if_nz").
>>>>
>>>> refcount -> rc
>>>> acquire -> acq
The names are still verbose, yet not descriptive. refcount -> ref loses
more than refcount -> rc.
>>>>
>>>> The "acq" abbreviation is already used a lot for atomic ops.
>>>
>>> How about refcount_acquire_gt_0() and refcount_release_gt_1()1?
Still verbose, and I prefer the _if_[n]z suffix. The gt suffix is only
shorter since you left out the 'if' part.
>> Current refcount(9) api is not only quite limited, but it also operates
>> on u_int instead of a type opaque to its consumers. This gives fewer
>> places which can assert counter sanity and increases chances of an
>> accidental abuse/type mismatch.
>>
>> As such, how about deprecating current refcount_* funcs and introducing
>> a new family instead.
>>
>> Say funcs would be prefixed with ref_. Consumers would use a ref_t type
>> (the obvious refcount_t is stolen by zfs).
Bug in zfs. rc_t might be OK.
>> Apart from aforementioned 0->1 and 1->0 funcs, this introduces ref_read
>> to obtain the state of passed counter. A cast to volatile pointer +
>> dereference of that guarantees us a read at that point, so we don't have
>> to put the qualifier in the struct.
>>
>> Finally, a blessed type is provided for use by all consumers so that the
>> correct type is enforced at compile time.
>>
>> Roughly speaking:
>>
>> diff --git a/sys/sys/refcount.h b/sys/sys/refcount.h
>> index 4611664..c7c3732 100644
>> --- a/sys/sys/refcount.h
>> +++ b/sys/sys/refcount.h
>> @@ -38,6 +38,11 @@
>> #define=09KASSERT(exp, msg)=09/* */
>> #endif
>>
>> +struct ref {
>> +=09u_int r_count;
>> +};
>> +typedef struct ref ref_t;
>> +
style(9) forbids "typedef struct" since it is less opaque than a pointer
to an opaque struct. The contents of an opaque struct should not be
exposed, but "typedef struct" requires exposing them.
>> static __inline void
>> refcount_init(volatile u_int *count, u_int value)
>> {
This still hard-codes u_int in the API.
In fact, it is impossible for the refcount type to be fully opaque, since
to initialize it you need to pass a count. Callers must know that this
count is an integer and more about the type of this integer.
>> @@ -64,4 +69,64 @@ refcount_release(volatile u_int *count)
>> =09return (old =3D=3D 1);
>> }
As above.
>>
>> +static __inline void
>> +ref_init(ref_t *ref, u_int value)
>> +{
>> +
>> +=09refcount_init(&ref->r_count, value);
>> +}
As above, now in a new API.
>> +
>> +static __inline u_int
>> +ref_read(ref_t *ref)
>> +{
>> +=09u_int count;
>> +
>> +=09count =3D *(volatile u_int *)&(ref->r_count);
>> +=09KASSERT(count < UINT_MAX, ("refcount %p overflowed", ref));
>> +
>> +=09return (count);
>> +}
As above, but more so. Most initializations will be to 0, but return
values may be large. struct ref could have other stuff in it, but the
counter part must be non-opaque so that clients can know the count.
I seee that this just duplicates refcount_read().
>> +
>> +static __inline void
>> +ref_acq(ref_t *ref)
>> +{
>> +
>> +=09refcount_acquire(&ref->r_count);
>> +}
>> +
>> +static __inline int
>> +ref_rel(ref_t *ref)
>> +{
>> +
>> +=09return (refcount_release(&ref->r_count));
>> +}
More obvious duplication.
I don't like anything in refcount.h. It just obfuscates adding or
subtracting 1 and adds some debugging code. refcount_init() doesn't
use an atomic op, so it wouldn't work for resetting an active counter.
The atomic ops that add and subtract 1 have severely limited types in
MI code. They wouldn't be able to use anything except u_int without
large overheads or complications on some arches. (I also don't like
counter64. It hard-codes 64 instead of u_int, and has complications
on all arches, but is not complicated enough to minimize the overheads.)
>> +
>> +static __inline int
>> +ref_acq_gt_0(ref_t *ref)
>> +{
>> +=09u_int old;
>> +
>> +=09for (;;) {
>> +=09=09old =3D ref_read(ref);
>> +=09=09if (old =3D=3D 0)
>> +=09=09=09return (0);
>> +=09=09if (atomic_cmpset_int(&ref->r_count, old, old + 1))
>> +=09=09=09return (1);
>> +=09}
>> +}
>> +
>> +static __inline int
>> +ref_rel_gt_1(ref_t *ref)
>> +{
>> +=09u_int old;
>> +
>> +=09for (;;) {
>> +=09=09old =3D ref_read(ref);
>> +=09=09if (old =3D=3D 1)
>> +=09=09=09return (0);
>> +=09=09if (atomic_cmpset_int(&ref->r_count, old, old - 1))
>> +=09=09=09return (1);
This does bad things if old =3D=3D 0.
>> +=09}
>> +}
These are complicated enough to be more than obfuscations of adding and
subtracting 1.
Acquire/release in the names de-emphasizes that reference counters are
_counters_, and refcount -> ref goes further in this direction. This
can be made further obfscated/opaque by not having public APIs to
return the count (so the problems with the type of this count go
away). Initializations would then have to be to the fixed state of
"not held", and queries could only return held/not held. But these
functions and their 0/1 suffixes go in the opposite direction, and seem
to requires callers to know more than the held/not held state to work.
>> +
>> #endif=09/* ! __SYS_REFCOUNT_H__ */
Old style bugs:
- tab instead of space after #endif
- space after '!'
- excessive underscores
See an old header like stdio.h for the normal style of this endif and its
macro name.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150919205230.F3483>