Lockless uidinfo.

[Bruce Evans]

On Sun, 19 Aug 2007, Jeff Roberson wrote:

> On Sun, 19 Aug 2007, Bruce Evans wrote:
>
>> On Sun, 19 Aug 2007, Jeff Roberson wrote:
>> 
>>> On Sun, 19 Aug 2007, Bruce Evans wrote:
>> 
>>>> atomic_*long() shouldn't exist (and doesn't exist in my version) since
>>>> longs should actually be long (twice as long as registers) and thus
>>>> especially epensive to lock.
>>> 
>>> Well unfortunately this is not how the compiler implements them on the 
>>> architectures that we support.  So in this case long is 32bit on 32bit 
>>> machines and 64bit on 64bit machines and as such requires each 
>>> architecture to treat them specially.  I don't think it's unreasonable to 
>>> add an atomic_fetchadd_long() that conforms to the definition of long that 
>>> is actually in use.
>> 
>> The compiler has nothing to do with this.  The implementation is FreeBSD's
>> and it is poor, like I said.
>
> Well this really has little to do with the problem at hand.  The long 
> decision has already been made and it's not practical to change it now. 
> Adding apis that accept the types that we've decided on should not be 
> crippled because you don't like the types.  I agree that we can't have 
> atomics that are wider than architectures support, but that isn't the case 
> here.

Long is wider than the arch supports for i386's with correctly sized longs
(option _LARGE_LONG).  I haven't tested that lately, since 64-bit arches
provide testing for most of the unportabilities exposed by _LARGE_LONG.

Removing atomic_*long is quite practical, since this mistake is rarely
used.  Grepping for atromic.*long outside of atomic.h shows:

% ./amd64/include/pmap.h:#define	pte_load_clear(pte)	atomic_readandclear_long(pte)

Really wants the pte type.

% ./contrib/ipfilter/netinet/ip_compat.h:#    define	ATOMIC_INCL(x)		atomic_add_long((uint32_t*)&(x), 1)
% ./contrib/ipfilter/netinet/ip_compat.h:#    define	ATOMIC_DECL(x)		atomic_add_long((uint32_t*)&(x), -1)
% ./contrib/ipfilter/netinet/ip_compat.h:#    define	ATOMIC_INCL(x)		atomic_add_long(&(x), 1)
% ./contrib/ipfilter/netinet/ip_compat.h:#    define	ATOMIC_DECL(x)		atomic_add_long(&(x), -1)
% ./contrib/ipfilter/netinet/ip_compat.h:#   define	ATOMIC_INCL(x)		atomicAddUlong(&(x), 1)
% ./contrib/ipfilter/netinet/ip_compat.h:#   define	ATOMIC_DECL(x)		atomicAddUlong(&(x), -1)

The above are for non-FreeBSD (mainly Solaris) only, in ifdef tangle.

% ./contrib/ipfilter/netinet/ip_compat.h:#   define	ATOMIC_INCL(x)		atomic_add_long(&(x), 1)
% ./contrib/ipfilter/netinet/ip_compat.h:#   define	ATOMIC_DECL(x)		atomic_add_long(&(x), -1)

These seem to be used mainly for statistics.

% ./contrib/opensolaris/uts/common/sys/bitmap.h:	{ atomic_or_long(&(BT_WIM(bitmap, bitindex)), BT_BIW(bitindex)); }
% ./contrib/opensolaris/uts/common/sys/bitmap.h:	{ atomic_and_long(&(BT_WIM(bitmap, bitindex)), ~BT_BIW(bitindex)); }
% ./contrib/opensolaris/uts/common/sys/bitmap.h:	{ result = atomic_set_long_excl(&(BT_WIM(bitmap, bitindex)),	\
% ./contrib/opensolaris/uts/common/sys/bitmap.h:	{ result = atomic_clear_long_excl(&(BT_WIM(bitmap, bitindex)),	\

These seem to be unused.  FreeBSD doesn't implement atomic ops with these
spellings.  Longs are an especially poor choice for bitmaps since any scalar
type will work and asking for longs asks for expensive locking to implement
the atomicity when longs are actually long.

% ./sys/sched.h:#define SCHED_STAT_INC(var)     atomic_add_long(&(var), 1)

More statistics.  The statistics variables are actually long, but this is
useless since they are exported using SYSCTL_INT(), so userland can only
see their low 32 bits (with overflow to a negative value if the highest of
those bits is set).

% ./sys/umtx.h:	if (atomic_cmpset_acq_long(&umtx->u_owner, UMTX_UNOWNED, id) == 0)
% ./sys/umtx.h:	if (atomic_cmpset_acq_long(&umtx->u_owner, UMTX_UNOWNED, id) == 0)
% ./sys/umtx.h:	if (atomic_cmpset_acq_long(&umtx->u_owner, UMTX_UNOWNED, id) == 0)
% ./sys/umtx.h:	if (atomic_cmpset_rel_long(&umtx->u_owner, id, UMTX_UNOWNED) == 0)

u_owner has type u_long.  The bug is missing in <sys/*mutex.h>, where
uintptr_t and atomic_cmpset_acq_ptr() is used for the similar m_lock
variable.

% ./sparc64/include/pmap.h:	atomic_add_long(&var, 1)

More statistics.  Correctly exported using SYSCTL_LONG() (signed counters,
so not SYSCTL_ULONG).

% ./amd64/amd64/pmap.c:				if (!atomic_cmpset_long(pte, obits, pbits))
% ./amd64/amd64/pmap.c:		atomic_set_long(pte, PG_W);
% ./amd64/amd64/pmap.c:		atomic_clear_long(pte, PG_W);
% ./amd64/amd64/pmap.c:			if (!atomic_cmpset_long(pte, oldpte, oldpte &
% ./amd64/amd64/pmap.c:				atomic_clear_long(pte, PG_A);
% ./amd64/amd64/pmap.c:			atomic_clear_long(pte, PG_M);
% ./amd64/amd64/pmap.c:			atomic_clear_long(pte, PG_A);
% ./amd64/amd64/minidump_machdep.c:	atomic_set_long(&vm_page_dump[idx], 1ul << bit);
% ./amd64/amd64/minidump_machdep.c:	atomic_clear_long(&vm_page_dump[idx], 1ul << bit);

No problems using atomic.*long in MD code, so I didn't look at the details.
There is also no need to use atomic.*long, since atomic.*64 would work
identically modulo a large _LARGE_LONG option making longs actually long.
Then long would be just wrong for ptes.

% ./vm/redzone.c:	atomic_add_long(&redzone_extra_mem, redzone_size_ntor(nsize) - nsize);
% ./vm/redzone.c:	atomic_subtract_long(&redzone_extra_mem,

Really want the vm_size_t type?

% ./compat/ndis/subr_ndis.c:	atomic_add_long((u_long *)addend, 1);
% ./compat/ndis/subr_ndis.c:	atomic_subtract_long((u_long *)addend, 1);

addend has type (uint32_t *), so the cast to (u_long *) seems to be just a
type mismatch asking for buffer overrun if long is longer.

% ./compat/ndis/subr_ntoskrnl.c:	atomic_add_long((volatile u_long *)addend, 1);
% ./compat/ndis/subr_ntoskrnl.c:	atomic_subtract_long((volatile u_long *)addend, 1);

As above.  The bogus cast now requires volatile since addend now has type
(volatile uint32_t *).

% ./dev/cxgb/cxgb_offload.c:		atomic_cmpset_ptr((long *)&t->tid_tab[tid].ctx, (long)NULL, (long)ctx);

Not an atomic.*long call, but just 3 bogus casts which have little effect
since the prototype will cause conversions to the types that the function
actually takes.

% ./kern/kern_clock.c:			atomic_add_long(&cp_time[CP_NICE], 1);
% ./kern/kern_clock.c:			atomic_add_long(&cp_time[CP_USER], 1);
% ./kern/kern_clock.c:			atomic_add_long(&cp_time[CP_INTR], 1);
% ./kern/kern_clock.c:				atomic_add_long(&cp_time[CP_SYS], 1);
% ./kern/kern_clock.c:				atomic_add_long(&cp_time[CP_IDLE], 1);

Recent mistakes.   cp_time[] has always been long[] for bogus historical
reasons.  On 64-bit arches, a 64-bit type for cp_time[] is slightly better,
but this was not very important since cp_time[] takes 480+ days to 
overflow 32 bits with HZ = 100.  Now with HZ = 1000, it takes only 24+
days to overflow 31 bits.  This shows that long is neither necessary nor
sufficient for cp_time[].  It is too small for 32-bit arches.  This is
hard to fix without changing the cp_time[] API, except of course if longs
are actually long -- then no API change is needed, but atomic_add_long()
cannot be simple, efficient, or what it is now.

BTW, it's bogus for the statclock counters in struct rusage_ext to be
64 bits while the above are only 32 bits.  On 32-bit machines, the
above counters normally overflow long before per-process statclock
counters since they are system- wide and HZ is normally 8-10 times
larger than stathz.

% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags, add_arg);
% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags, add_arg);
% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags, OP_PENDING);
% ./netgraph/ng_base.c:	atomic_add_long(&ngq->q_flags, READER_INCREMENT);
% ./netgraph/ng_base.c:	atomic_subtract_long(&ngq->q_flags, READER_INCREMENT);
% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags, READER_INCREMENT);
% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags, WRITER_ACTIVE);
% ./netgraph/ng_base.c:			atomic_subtract_long(&ngq->q_flags, WRITER_ACTIVE);
% ./netgraph/ng_base.c:	atomic_add_long(&ngq->q_flags, WRITER_ACTIVE - READER_INCREMENT);
% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags,
% ./netgraph/ng_base.c:		atomic_add_long(&ngq->q_flags, OP_PENDING);
% ./netgraph/ng_base.c:	atomic_add_long(&ngq->q_flags, READER_INCREMENT - WRITER_ACTIVE);
% ./netgraph/ng_base.c:	atomic_subtract_long(&ngq->q_flags, READER_INCREMENT);
% ./netgraph/ng_base.c:	atomic_subtract_long(&ngq->q_flags, WRITER_ACTIVE);
% ./netgraph/ng_base.c:			atomic_add_long(&ngq->q_flags, -OP_PENDING);

q_flags has type u_long.  I don't know if this is essential on 64-bit
machines, or if so if overflow is a problem on 32-bit machines.  It's
strange to do addition and subrtraction on flags.

% ./sparc64/sparc64/pmap.c:		atomic_clear_long(&tp->tte_data, TD_CV);
% ./sparc64/sparc64/pmap.c:		atomic_set_long(&tp->tte_data, TD_CV);
% ./sparc64/sparc64/pmap.c:	data = atomic_readandclear_long(&tp->tte_data);
% ./sparc64/sparc64/pmap.c:	data = atomic_clear_long(&tp->tte_data, TD_REF | TD_SW | TD_W);
% ./sparc64/sparc64/pmap.c:			data = atomic_set_long(&tp->tte_data, TD_WIRED);
% ./sparc64/sparc64/pmap.c:			data = atomic_clear_long(&tp->tte_data, TD_WIRED);
% ./sparc64/sparc64/pmap.c:			data = atomic_clear_long(&tp->tte_data, TD_REF);
% ./sparc64/sparc64/pmap.c:		data = atomic_clear_long(&tp->tte_data, TD_W);
% ./sparc64/sparc64/pmap.c:		data = atomic_clear_long(&tp->tte_data, TD_REF);
% ./sparc64/sparc64/pmap.c:		data = atomic_clear_long(&tp->tte_data, TD_SW | TD_W);

MD, not a problem.

Summary: most uses of atomic.*long are for statistics and/or in secondary
subsystems.

Longs for statistics are convenient but not essential or right.  Most
statistics counters should be 32 bits if that is enough, else 64 bits,
_not_ depending on whether long is 64 bits, but possibly depending on
whether 64 bits would be too inefficient due to extra locking required
to fake 64-bit atomic accesses.

Bruce