Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 27 Feb 2015 13:35:31 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Ian Lepore <ian@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r279338 - head/sys/arm/include
Message-ID:  <20150227125241.E802@besplex.bde.org>
In-Reply-To: <201502262305.t1QN5lmY075787@svn.freebsd.org>
References:  <201502262305.t1QN5lmY075787@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 26 Feb 2015, Ian Lepore wrote:

> Log:
>  Add casting to make atomic ops work for pointers.  (Apparently nobody has
>  ever done atomic ops on pointers before now on arm).

Apparently, arm code handled pointers correctly before.  des broke
i386 in the same way and didn't back out the changes as requested, but
all other arches including amd64 remained unbroken, so there can't be
any MI code that handles the pointers incorrectly enough to "need" it.
Checking shows no MD code either.

> Modified: head/sys/arm/include/atomic.h
> ==============================================================================
> --- head/sys/arm/include/atomic.h	Thu Feb 26 22:46:01 2015	(r279337)
> +++ head/sys/arm/include/atomic.h	Thu Feb 26 23:05:46 2015	(r279338)
> @@ -1103,13 +1103,23 @@ atomic_store_long(volatile u_long *dst,
> 	*dst = src;
> }
>
> -#define atomic_clear_ptr		atomic_clear_32
> -#define atomic_set_ptr			atomic_set_32
> -#define atomic_cmpset_ptr		atomic_cmpset_32
> -#define atomic_cmpset_rel_ptr		atomic_cmpset_rel_32
> -#define atomic_cmpset_acq_ptr		atomic_cmpset_acq_32
> -#define atomic_store_ptr		atomic_store_32
> -#define atomic_store_rel_ptr		atomic_store_rel_32
> +#define atomic_clear_ptr(p, v) \
> +	atomic_clear_32((volatile uint32_t *)(p), (uint32_t)(v))
> +#define atomic_set_ptr(p, v) \
> +	atomic_set_32((volatile uint32_t *)(p), (uint32_t)(v))
> +#define atomic_cmpset_ptr(p, cmpval, newval) \
> +	atomic_cmpset_32((volatile u_int32_t *)(p), (u_int32_t)(cmpval), \
> +	    (u_int32_t)(newval))
> +#define atomic_cmpset_rel_ptr(p, cmpval, newval) \
> +	atomic_cmpset_rel_32((volatile u_int32_t *)(p), (u_int32_t)(cmpval), \
> +	    (u_int32_t)(newval))
> +#define atomic_cmpset_acq_ptr(p, cmpval, newval) \
> +	atomic_cmpset_acq_32((volatile u_int32_t *)(p), (u_int32_t)(cmpval), \
> +	    (u_int32_t)(newval))
> +#define atomic_store_ptr(p, v) \
> +	atomic_store_32((volatile uint32_t *)(p), (uint32_t)(v))
> +#define atomic_store_rel_ptr(p, v) \
> +	atomic_store_rel_32((volatile uint32_t *)(p), (uint32_t)(v))
>
> #define atomic_add_int			atomic_add_32
> #define atomic_add_acq_int		atomic_add_acq_32

These bogus casts reduce type safety.  atomic*ptr is designed and
documented to take only (indirect) uintptr_t * and (direct) uintptr_t
args, not pointer args bogusly cast or otherwise type-punned to these.
Most callers actually use the API correctly.  E.g., the mutex cookie
is a uintptr_t, not a pointer.  This affected the design of mutexes
a little.  The cookie could be either a pointer representing an integer
ot an integer representing a pointer, or a union of these, and the
integer is simplest.

These casts don't even break the warning in most cases where they have
an effect.  They share this implementation bug with the i386 ones.
Casting to [volatile] uint32_t * only works if the pointer is already
[volatile] uint32_t * or [volatile] void *.  For full breakage, it
us necessary to cast to volatile void * first.  Omitting the cast to
void * should only work here because most or all callers ensure that
the pointer already has one of these types, so the cast here has no
effect.

Casting the input arg to uint32_t does work to break the warning,
because uint32_t is the same as uintptr_t and -Wcast-qual is broken
for casting away qualifiers in this way.

Here are all matches with atomic.*ptr in .c files in /sys/:

./dev/hatm/if_hatm_intr.c:		if (atomic_cmpset_ptr((uintptr_t *)list, (uintptr_t)buf->link,
./dev/hatm/if_hatm_intr.c:		if (atomic_cmpset_ptr((uintptr_t *)&sc->mbuf_list[g],
./dev/hatm/if_hatm_intr.c:			if (atomic_cmpset_ptr((uintptr_t *)&sc->mbuf_list[g],

Bogus casts in the caller.  As partly mentioned above, casting to
uintptr_t * shouldn't even break the warning.  It is necessary to go through
void *, and for that casting to void * here is sufficient (the prototype
will complete the type pun).

./dev/cxgbe/t4_main.c:	atomic_store_rel_ptr(loc, new);
./dev/cxgbe/t4_main.c:	atomic_store_rel_ptr(loc, new);
./dev/cxgbe/t4_main.c:	atomic_store_rel_ptr(loc, new);

Correct.  The variables are uintptr_t * and uinptr_t, respectively.

./dev/cxgbe/tom/t4_listen.c:	wr = (struct wrqe *)atomic_readandclear_ptr(&synqe->wr);

Correct.  syncqe->wr is uintptr_t.

./dev/cxgbe/tom/t4_listen.c:	atomic_store_rel_ptr(&synqe->wr, (uintptr_t)wr);
./dev/cxgbe/tom/t4_listen.c:	if (atomic_cmpset_ptr(&synqe->wr, (uintptr_t)wr, 0)) {

Correct.  For some reason, wr is a pointer so it must be cast.  This cast is
not bogus.  (Conversion from any pointer type to uintptr_t and back is
possible.  This may change its representation, although it doesn't on any
supported arch.   If the representation does change, input args continue
to work right but the API becomes unsuitable for handling output args.

./dev/cxgb/cxgb_main.c:	atomic_store_rel_ptr(loc, new);

./dev/proto/proto_core.c:	if (!atomic_cmpset_acq_ptr(&r->r_opened, 0UL, (uintptr_t)td->td_proc))
./dev/proto/proto_core.c:	if (!atomic_cmpset_acq_ptr(&r->r_opened, (uintptr_t)td->td_proc, 0UL))

Correct except for bogus type 0UL.  Unsigned long is neither necessary nor
sufficient.  (uintptr_t)0 would be technically correct but verbose.  Plain
0 always works for the same reasons as 0UL -- the prototype converts it
to uintptr_t.

./dev/cfi/cfi_dev.c:	if (!atomic_cmpset_acq_ptr((uintptr_t *)&sc->sc_opened,

Bogus cast.

./dev/sfxge/sfxge_tx.c:	put = atomic_readandclear_ptr(putp);
./dev/sfxge/sfxge_tx.c:		} while (atomic_cmpset_ptr(putp, old, new) == 0);

Apparently correct (details not checked).  Apparently all the big NIC
drivers are careful.  Otherwise they would not compile on amd64.

./dev/hwpmc/hwpmc_mod.c:	atomic_store_rel_ptr((uintptr_t *)&pp->pp_pmcs[ri].pp_pmc,
./dev/pty/pty.c:	if (!atomic_cmpset_ptr((uintptr_t *)&dev->si_drv1, 0, 1))
./dev/ismt/ismt.c:		acquired = atomic_cmpset_ptr(
./dev/ismt/ismt.c:		atomic_store_rel_ptr((uintptr_t *)&sc->bus_reserved,

Bogus casts.

./cddl/dev/dtrace/dtrace_debug.c:	while (atomic_cmpset_acq_ptr(&dtrace_debug_data[cpu].lock, 0, tid) == 0)		/* Loop until the lock is obtained. */
./cddl/dev/dtrace/dtrace_debug.c:	atomic_store_rel_ptr(&dtrace_debug_data[cpu].lock, 0);
./cddl/contrib/opensolaris/uts/common/dtrace/systrace.c:	(void) atomic_cas_ptr(&sysent[sysnum].sy_callc,
./cddl/contrib/opensolaris/uts/common/dtrace/systrace.c:	(void) atomic_cas_ptr(&sysent32[sysnum].sy_callc,
./cddl/contrib/opensolaris/uts/common/dtrace/systrace.c:		(void) atomic_cas_ptr(&sysent[sysnum].sy_callc,
./cddl/contrib/opensolaris/uts/common/dtrace/systrace.c:		(void) atomic_cas_ptr(&sysent32[sysnum].sy_callc,
./cddl/contrib/opensolaris/uts/common/os/fm.c:	(void) atomic_cas_ptr((void *)&fm_panicstr, NULL, (void *)format);
./cddl/contrib/opensolaris/uts/common/fs/zfs/dnode.c:		winner = atomic_cas_ptr(&dnh->dnh_dnode, NULL, dn);

Apparently correct.  Apparently most of the big/portable software is careful,
not just big NIC drivers

./kern/kern_sx.c:		if (atomic_cmpset_acq_ptr(&sx->sx_lock, x, x + SX_ONE_SHARER)) {
./kern/kern_sx.c:		atomic_set_ptr(&sx->sx_lock, SX_LOCK_RECURSED);
./kern/kern_sx.c:		rval = atomic_cmpset_acq_ptr(&sx->sx_lock, SX_LOCK_UNLOCKED,
./kern/kern_sx.c:	success = atomic_cmpset_ptr(&sx->sx_lock, SX_SHARERS_LOCK(1) | x,
./kern/kern_sx.c:	    atomic_cmpset_rel_ptr(&sx->sx_lock, x, SX_SHARERS_LOCK(1) |
./kern/kern_sx.c:	atomic_store_rel_ptr(&sx->sx_lock, SX_SHARERS_LOCK(1) |
./kern/kern_sx.c:		atomic_set_ptr(&sx->sx_lock, SX_LOCK_RECURSED);
./kern/kern_sx.c:	while (!atomic_cmpset_acq_ptr(&sx->sx_lock, SX_LOCK_UNLOCKED, tid)) {
./kern/kern_sx.c:			if (atomic_cmpset_acq_ptr(&sx->sx_lock,
./kern/kern_sx.c:			if (!atomic_cmpset_ptr(&sx->sx_lock, x,
./kern/kern_sx.c:			atomic_clear_ptr(&sx->sx_lock, SX_LOCK_RECURSED);
./kern/kern_sx.c:	atomic_store_rel_ptr(&sx->sx_lock, x);
./kern/kern_sx.c:			if (atomic_cmpset_acq_ptr(&sx->sx_lock, x,
./kern/kern_sx.c:			if (!atomic_cmpset_ptr(&sx->sx_lock, x,
./kern/kern_sx.c:			if (atomic_cmpset_rel_ptr(&sx->sx_lock, x,
./kern/kern_sx.c:			if (atomic_cmpset_rel_ptr(&sx->sx_lock,
./kern/kern_sx.c:		if (!atomic_cmpset_rel_ptr(&sx->sx_lock,
./kern/kern_mutex.c:		atomic_set_ptr(&m->mtx_lock, MTX_RECURSED);
./kern/kern_mutex.c:		atomic_set_ptr(&m->mtx_lock, MTX_RECURSED);
./kern/kern_mutex.c:		    !atomic_cmpset_ptr(&m->mtx_lock, v, v | MTX_CONTESTED)) {

Correct.  Of course, the main author of the API knows what it is.

./kern/kern_mutex.c:	atomic_store_rel_ptr((volatile void *)&td->td_lock, (uintptr_t)new);

Bogus cast.  Less bogus than usual.  The main author of the API was not
vigilant when this was changed :-).  The normal bogus cast in MI code is
to uintptr_t *, to type pun the bits to uintptr_t.  Here we go through
void * and also volatile.  void * has the same effect as an unwarned-about
uintptr_t * since it causes the compiler to forget that the pointed-to type
is a pointer and the prototype then converts to volatile uintptr_t *.
volatile here is needed because td_lock is volatile.

./kern/kern_mutex.c:			atomic_clear_ptr(&m->mtx_lock, MTX_RECURSED);
./kern/kern_lock.c:			if (atomic_cmpset_rel_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:			if (atomic_cmpset_rel_ptr(&lk->lk_lock, x, LK_UNLOCKED))
./kern/kern_lock.c:		if (!atomic_cmpset_rel_ptr(&lk->lk_lock, LK_SHARERS_LOCK(1) | x,
./kern/kern_lock.c:				if (atomic_cmpset_acq_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:				if (!atomic_cmpset_acq_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:		if (atomic_cmpset_ptr(&lk->lk_lock, LK_SHARERS_LOCK(1) | x | v,
./kern/kern_lock.c:		while (!atomic_cmpset_acq_ptr(&lk->lk_lock, LK_UNLOCKED,
./kern/kern_lock.c:				    !atomic_cmpset_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:				if (atomic_cmpset_acq_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:				if (!atomic_cmpset_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:			if (atomic_cmpset_rel_ptr(&lk->lk_lock, tid | x,
./kern/kern_lock.c:			if (atomic_cmpset_rel_ptr(&lk->lk_lock, tid,
./kern/kern_lock.c:			atomic_store_rel_ptr(&lk->lk_lock, v);
./kern/kern_lock.c:		while (!atomic_cmpset_acq_ptr(&lk->lk_lock, LK_UNLOCKED, tid)) {
./kern/kern_lock.c:				if (!atomic_cmpset_ptr(&lk->lk_lock, x, v)) {
./kern/kern_lock.c:				if (!atomic_cmpset_ptr(&lk->lk_lock, x,
./kern/kern_lock.c:		if (atomic_cmpset_rel_ptr(&lk->lk_lock, tid | x,

Correct.  Of course, the main author of the API knows what it is.

./kern/kern_descrip.c:	atomic_store_rel_ptr((volatile void *)&fdp->fd_files, (uintptr_t)ntable);
./kern/kern_descrip.c:	atomic_store_rel_ptr((volatile uintptr_t *)&fp->f_ops, (uintptr_t)ops);

Different bogus casts.  The volatiles are bogus here since the variables
are not volatile (volatile is only needed in the API because some variables
are not declared volatile; it makes them volatile automatically).  The
cast with void * eventually works via a type pun.  The one with uintptr_t *
should fail.

./kern/kern_rwlock.c:		rval = atomic_cmpset_acq_ptr(&rw->rw_lock, RW_UNLOCKED,
./kern/kern_rwlock.c:			if (atomic_cmpset_acq_ptr(&rw->rw_lock, v,
./kern/kern_rwlock.c:			if (!atomic_cmpset_ptr(&rw->rw_lock, v,
./kern/kern_rwlock.c:		if (atomic_cmpset_acq_ptr(&rw->rw_lock, x, x + RW_ONE_READER)) {
./kern/kern_rwlock.c:			if (atomic_cmpset_rel_ptr(&rw->rw_lock, x,
./kern/kern_rwlock.c:			if (atomic_cmpset_rel_ptr(&rw->rw_lock, x,
./kern/kern_rwlock.c:		if (!atomic_cmpset_rel_ptr(&rw->rw_lock, RW_READERS_LOCK(1) | v,
./kern/kern_rwlock.c:				if (!atomic_cmpset_ptr(&rw->rw_lock, v,
./kern/kern_rwlock.c:			if (atomic_cmpset_acq_ptr(&rw->rw_lock, v, tid | x)) {
./kern/kern_rwlock.c:			if (!atomic_cmpset_ptr(&rw->rw_lock, v,
./kern/kern_rwlock.c:	atomic_store_rel_ptr(&rw->rw_lock, v);
./kern/kern_rwlock.c:			success = atomic_cmpset_ptr(&rw->rw_lock, v, tid);
./kern/kern_rwlock.c:		success = atomic_cmpset_ptr(&rw->rw_lock, v, tid | x);
./kern/kern_rwlock.c:	if (atomic_cmpset_rel_ptr(&rw->rw_lock, tid, RW_READERS_LOCK(1)))
./kern/kern_rwlock.c:	atomic_store_rel_ptr(&rw->rw_lock, RW_READERS_LOCK(1) | v);

Correct.  Of course, the main author of the API knows what it is.

./kern/sched_ule.c:	atomic_store_rel_ptr((volatile uintptr_t *)&td->td_lock,
./sparc64/sparc64/pmap.c:		atomic_cmpset_rel_ptr((uintptr_t *)&pc->pc_pmap,
./sparc64/sparc64/pmap.c:	atomic_store_acq_ptr((uintptr_t *)PCPU_PTR(pmap), (uintptr_t)pm);
./amd64/amd64/sys_machdep.c:	atomic_store_rel_ptr((volatile uintptr_t *)&mdp->md_ldt,

The usual bogus casts.  Bogus volatile.

./amd64/vmm/io/vlapic.c:	atomic_set_int(&irrptr[idx], mask);
./amd64/vmm/io/vlapic.c:		val = atomic_load_acq_int(&irrptr[idx]);
./amd64/vmm/io/vlapic.c:	atomic_clear_int(&irrptr[idx], 1 << (vector % 32));

Apparently correct.  It must have use the right integer types, at least
accidentally, to work on amd64.

./ofed/drivers/net/mlx4/sys_tune.c:	atomic_t *busy_cpus_ptr;
./ofed/drivers/net/mlx4/sys_tune.c:		busy_cpus = atomic_read(busy_cpus_ptr);
./ofed/drivers/net/mlx4/sys_tune.c:		     ( atomic_cmpxchg(busy_cpus_ptr, busy_cpus,
./ofed/drivers/net/mlx4/sys_tune.c:			atomic_add(1, busy_cpus_ptr);

Apparently correct.  Some danger of types only matching accidentally since
it doesn't spell them uintptr_t.

./netgraph/netflow/netflow.c:	if (atomic_cmpset_ptr((volatile uintptr_t *)&priv->fib_data[fib],
./ufs/ffs/ffs_alloc.c:			atomic_store_rel_ptr((volatile uintptr_t *)&vfp->f_ops,
./ufs/ffs/ffs_alloc.c:			atomic_store_rel_ptr((volatile uintptr_t *)&vfp->f_ops,

The usual bogus casts.  Bogus volatiles (at least the visible netgraph one).

Summary: there are about 97 callers of atomic*ptr().  About 80 of them use
it correctly.  Unless I missed something, all of the other 17 already
supply essentially the same bogus casts that this commits adds, as needed
for them to compile on amd64.  So the change has no effect now.  Similary
on i386.  The also can't have any effect in future except in MD arm and
i386 code unless other arches are broken to march.  Then the number of
cases with bogus casts could easily expand from 17.

However, the case of exotic arches where conversion from pointers to
uintptr_t changes the representation cannot really work even if the
caller takes care.  Suppose you have a pointer sc->sc_p that you want
to update atomically.  This is impossible using atomic*ptr(), since 
that only operates on a converted representation of the pointer. 
Updating the pointer atomically might be possible using other atomic,
but if the pointer just has a different size than uintptr_t, it cannot
even be accessed using atomic_ptr*().

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150227125241.E802>