From owner-freebsd-arch@FreeBSD.ORG Sat Oct 25 19:43:48 2014 Return-Path: Delivered-To: arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 014CD584 for ; Sat, 25 Oct 2014 19:43:47 +0000 (UTC) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client CN "mailhost.stack.nl", Issuer "CA Cert Signing Authority" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id BA715FCB for ; Sat, 25 Oct 2014 19:43:47 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 8D2C7358C54; Sat, 25 Oct 2014 21:43:44 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id 7211928494; Sat, 25 Oct 2014 21:43:44 +0200 (CEST) Date: Sat, 25 Oct 2014 21:43:44 +0200 From: Jilles Tjoelker To: Konstantin Belousov Subject: Re: RfC: fueword(9) and casueword(9) Message-ID: <20141025194344.GA11568@stack.nl> References: <20141021094539.GA1877@kib.kiev.ua> <20141022002825.H2080@besplex.bde.org> <20141021162306.GE1877@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141021162306.GE1877@kib.kiev.ua> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: arch@freebsd.org X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Oct 2014 19:43:48 -0000 On Tue, Oct 21, 2014 at 07:23:06PM +0300, Konstantin Belousov wrote: > On Wed, Oct 22, 2014 at 01:41:12AM +1100, Bruce Evans wrote: > > On Tue, 21 Oct 2014, Konstantin Belousov wrote: > > This API is larger, slower, and harder to use. No better fix is evident, > > except for fubyte() and fuword16(). These never had a problem on any > > supported arch, since bytes are only 8 bits on all supported arches, > > and 16-bit ints are not supported on any arch, so -1 is always out of > > band. Not touching them is a better fix. You didn't change any of > > their callers, but pessimized their implementation to a wrapper around > > fue*(). (BTW, fuword16() and fuswintr() are misdocumented as taking a > > non-const void * arg.). > I reverted the addition of fuebyte(9) and fueword16(9). First I thought > that it would be nicer to provide fully complement KPI with regard to > fuX/fueX, but it seems that lack of fuebyte() and fueword16() will > cause the right questions. > > > @@ -921,7 +933,9 @@ do_lock_normal(struct thread *td, struct umutex *m, uint32_t flags, > > > * can fault on any access. > > > */ > > > for (;;) { > > > - owner = fuword32(__DEVOLATILE(void *, &m->m_owner)); > > > + rv = fueword32(__DEVOLATILE(void *, &m->m_owner), &owner); > > > + if (error == -1) > > > + return (EFAULT); > > > if (mode == _UMUTEX_WAIT) { > > > if (owner == UMUTEX_UNOWNED || owner == UMUTEX_CONTESTED) > > > return (0); > > A new API should try to fix these __DEVOLATILE() abominations. I think it > > is safe, and even correct, to declare the pointers as volatile const void *, > > since the functions really can handle volatile data, unlike copyin(). > > Atomic op functions are declared as taking pointers to volatile for > > similar reasons. Often they are applied to non-volatile data, but > > adding a qualifier is type-safe and doesn't cost efficiency since the > > pointer access is is not known to the compiler. (The last point is not > > so clear -- the compiler can see things in the functions since they are > > inline asm. fueword() isn't inline so its (in)efficiency is not changed.) > > The atomic read functions are not declared as taking pointers to const. > > The __DECONST() abomination might be used to work around this bug. > I prefer to not complicate the fetch(9) KPI due to the mistakes in the > umtx structures definitions. I think that it is bug to mark the lock > words with volatile. I want the fueword(9) interface to be as much > similar to fuword(9), in particular, volatile seems to be not needed. > Below is the updated patch, together with the bug fix noted by Eric. Hmm, consider returning an error number (that is, EFAULT) instead of -1 on failure. This is somewhat like priv_check() which returns EPERM on failure, and probably reduces confusion if the return value is assigned to a variable named "error". In share/man/man9/Makefile, MLINKS are still created for fuebyte.9 and fueword16.9. "successful" is consistently misspelled "successfull". -- Jilles Tjoelker