From owner-svn-src-all@FreeBSD.ORG Fri Feb 27 02:35:43 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id AE0D4706; Fri, 27 Feb 2015 02:35:43 +0000 (UTC) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 33BC3E71; Fri, 27 Feb 2015 02:35:42 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 0E72E10457C9; Fri, 27 Feb 2015 13:35:32 +1100 (AEDT) Date: Fri, 27 Feb 2015 13:35:31 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ian Lepore Subject: Re: svn commit: r279338 - head/sys/arm/include In-Reply-To: <201502262305.t1QN5lmY075787@svn.freebsd.org> Message-ID: <20150227125241.E802@besplex.bde.org> References: <201502262305.t1QN5lmY075787@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=A5NVYcmG c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=2WiDrOd2TvLU1v_-dSwA:9 a=CjuIK1q_8ugA:10 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 02:35:43 -0000 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