Date: Fri, 27 Feb 2015 08:18:26 -0700 From: Ian Lepore <ian@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> 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: <1425050306.1281.13.camel@freebsd.org> In-Reply-To: <20150227125241.E802@besplex.bde.org> References: <201502262305.t1QN5lmY075787@svn.freebsd.org> <20150227125241.E802@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 2015-02-27 at 13:35 +1100, Bruce Evans wrote:
> 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
>
::sigh:: As usual, thousands of words, maybe there's actionable info in
there, but I sure don't have time to ferret it out.
If there's something simple you'd like me to do, please say so. Simply.
-- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1425050306.1281.13.camel>