From owner-freebsd-arch@freebsd.org Sat Sep 19 12:28:50 2015 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id ECF019CFB31 for ; Sat, 19 Sep 2015 12:28:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id A55D81E89; Sat, 19 Sep 2015 12:28:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id A2A5B3C00DE; Sat, 19 Sep 2015 21:55:45 +1000 (AEST) Date: Sat, 19 Sep 2015 21:55:44 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik cc: Bruce Evans , freebsd-arch@freebsd.org, Konstantin Belousov , jhb@freebsd.org Subject: Re: new api for reference counters (was: atomic v_usecount and v_holdcnt) In-Reply-To: <20150919092355.GA14906@dft-labs.eu> Message-ID: <20150919205230.F3483@besplex.bde.org> 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> MIME-Version: 1.0 X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=W4DFLkik c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=nlC_4_pT8q9DhB4Ho9EA:9 a=JoytVGf8-DkyGwkeplEA:9 a=45ClL6m2LaAA:10 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 19 Sep 2015 12:28:51 -0000 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