Date: Sat, 25 Oct 2014 21:43:44 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Konstantin Belousov <kostikbel@gmail.com> Cc: arch@freebsd.org Subject: Re: RfC: fueword(9) and casueword(9) Message-ID: <20141025194344.GA11568@stack.nl> In-Reply-To: <20141021162306.GE1877@kib.kiev.ua> References: <20141021094539.GA1877@kib.kiev.ua> <20141022002825.H2080@besplex.bde.org> <20141021162306.GE1877@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141025194344.GA11568>