RfC: fueword(9) and casueword(9)

[Jilles Tjoelker]

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