From owner-svn-src-all@FreeBSD.ORG Fri Feb 27 15:18:35 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5F095248; Fri, 27 Feb 2015 15:18:35 +0000 (UTC) Received: from pmta1.delivery9.ore.mailhop.org (pmta1.delivery9.ore.mailhop.org [54.186.172.23]) by mx1.freebsd.org (Postfix) with ESMTP id 39BF59A4; Fri, 27 Feb 2015 15:18:35 +0000 (UTC) Received: from smtp4.ore.mailhop.org (172.31.36.112) by pmta1.delivery1.ore.mailhop.org id hu25c820r84p; Fri, 27 Feb 2015 15:18:22 +0000 (envelope-from ) Received: from c-73-34-117-227.hsd1.co.comcast.net ([73.34.117.227] helo=ilsoft.org) by smtp4.ore.mailhop.org with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.82) (envelope-from ) id 1YRMgO-0007eX-FY; Fri, 27 Feb 2015 15:18:28 +0000 Received: from revolution.hippie.lan (revolution.hippie.lan [172.22.42.240]) by ilsoft.org (8.14.9/8.14.9) with ESMTP id t1RFIQ52018736; Fri, 27 Feb 2015 08:18:26 -0700 (MST) (envelope-from ian@freebsd.org) X-Mail-Handler: DuoCircle Outbound SMTP X-Originating-IP: 73.34.117.227 X-Report-Abuse-To: abuse@duocircle.com (see https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information for abuse reporting information) X-MHO-User: U2FsdGVkX18GtCaUS2hXtDmQivBRGEC1 Message-ID: <1425050306.1281.13.camel@freebsd.org> Subject: Re: svn commit: r279338 - head/sys/arm/include From: Ian Lepore To: Bruce Evans Date: Fri, 27 Feb 2015 08:18:26 -0700 In-Reply-To: <20150227125241.E802@besplex.bde.org> References: <201502262305.t1QN5lmY075787@svn.freebsd.org> <20150227125241.E802@besplex.bde.org> Content-Type: text/plain; charset="us-ascii" X-Mailer: Evolution 3.12.10 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Feb 2015 15:18:35 -0000 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