From owner-freebsd-arch@freebsd.org Sat Sep 19 09:24:01 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 87F38A021C5 for ; Sat, 19 Sep 2015 09:24:01 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wi0-x235.google.com (mail-wi0-x235.google.com [IPv6:2a00:1450:400c:c05::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1552A1A3E; Sat, 19 Sep 2015 09:24:01 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by wiclk2 with SMTP id lk2so56383086wic.1; Sat, 19 Sep 2015 02:23:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:subject:message-id:references:mime-version :content-type:content-disposition:content-transfer-encoding :in-reply-to:user-agent; bh=oPzdRZomiRAV40PI8FzH3ZmOn6/AAhmU56Wb/e1dZLc=; b=QNklyZD6HtQZ+cBArrCV6LN55n0I/06NtGTH1WlMelr4buQ297atHS+38v8JEy3502 mTYQbqHJFj+KomtLYj+2/97M92cl45Zoo/3+ecbMRNidvdv1iuiojtBwENGJiik71lT0 bFY60GxtApBrdDqp9boJCAmBK0fWZ/yPPhQa+vxBSTUOYZyD3NxNO8GrqVa0n89/gqEm h8/Y+SdYMQf+IryrppxGyXqLEbyY7aqFfYtUWxrfdbjZc0eAesnNZIALUO0bhtHUkrBP lGSnDY0/+hyeidelwDrO0yCLqYOZYc7f5AMY3kp39QYN957chRF/lZdIcot2gY2xJYNx k/bg== X-Received: by 10.180.103.35 with SMTP id ft3mr2961448wib.60.1442654638627; Sat, 19 Sep 2015 02:23:58 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id ka10sm13092103wjc.30.2015.09.19.02.23.57 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 19 Sep 2015 02:23:57 -0700 (PDT) Date: Sat, 19 Sep 2015 11:23:55 +0200 From: Mateusz Guzik To: 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) Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20150702213048.GB7666@dft-labs.eu> User-Agent: Mutt/1.5.21 (2010-09-15) 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 09:24:01 -0000 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ła wrote: > > 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 shorter ? > > > > > > > > Unfortunately the original API is alreday quite verbose and I don't have > > > > 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 "acq" abbreviation is already used a lot for atomic ops. > > > > How about refcount_acquire_gt_0() and refcount_release_gt_1()1? > > > > 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). > > 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 KASSERT(exp, msg) /* */ > #endif > > +struct ref { > + u_int r_count; > +}; > +typedef struct ref ref_t; > + > static __inline void > refcount_init(volatile u_int *count, u_int value) > { > @@ -64,4 +69,64 @@ refcount_release(volatile u_int *count) > return (old == 1); > } > > +static __inline void > +ref_init(ref_t *ref, u_int value) > +{ > + > + refcount_init(&ref->r_count, value); > +} > + > +static __inline u_int > +ref_read(ref_t *ref) > +{ > + u_int count; > + > + count = *(volatile u_int *)&(ref->r_count); > + KASSERT(count < UINT_MAX, ("refcount %p overflowed", ref)); > + > + return (count); > +} > + > +static __inline void > +ref_acq(ref_t *ref) > +{ > + > + refcount_acquire(&ref->r_count); > +} > + > +static __inline int > +ref_rel(ref_t *ref) > +{ > + > + return (refcount_release(&ref->r_count)); > +} > + > +static __inline int > +ref_acq_gt_0(ref_t *ref) > +{ > + u_int old; > + > + for (;;) { > + old = ref_read(ref); > + if (old == 0) > + return (0); > + if (atomic_cmpset_int(&ref->r_count, old, old + 1)) > + return (1); > + } > +} > + > +static __inline int > +ref_rel_gt_1(ref_t *ref) > +{ > + u_int old; > + > + for (;;) { > + old = ref_read(ref); > + if (old == 1) > + return (0); > + if (atomic_cmpset_int(&ref->r_count, old, old - 1)) > + return (1); > + } > +} > + > #endif /* ! __SYS_REFCOUNT_H__ */ > > -- > Mateusz Guzik -- Mateusz Guzik