Lockless uidinfo.

[John Baldwin]

On Saturday 18 August 2007 12:14:49 pm Pawel Jakub Dawidek wrote:
> On Sat, Aug 18, 2007 at 08:50:41AM -0700, Alfred Perlstein wrote:
> > * Pawel Jakub Dawidek <pjd at FreeBSD.org> [070818 07:59] wrote:
> > > Yes, to lookup uidinfo you need to hold uihashtbl_mtx mutex, so once you
> > > hold it and ui_ref is 0, noone will be able to reference it, because it
> > > has to wait to look it up.
> > 
> > And the field doesn't need to be volatile to prevent cached/opportunitic
> > reads?
> 
> The only chance of something like this will be the scenario below:
> 
> thread1 (uifind)		thread2 (uifree)
> ----------------		----------------
> 				refcount_release(&uip->ui_ref))
> 				/* ui_ref == 0 */
> mtx_lock(&uihashtbl_mtx);
> refcount_acquire(&uip->ui_ref);
> /* ui_ref == 1 */
> mtx_unlock(&uihashtbl_mtx);
> 				mtx_lock(&uihashtbl_mtx);
> 				if (uip->ui_ref > 0) {
> 					mtx_unlock(&uihashtbl_mtx);
> 					return;
> 				}
> 
> Now, you suggest that ui_ref in 'if (uip->ui_ref > 0)' may still have
> cached 0? I don't think it is possible, first refcount_acquire() uses
> read memory bariers (but we may still need ui_ref to volatile for this
> to make any difference) and second, think of ui_ref as a field protected
> by uihashtbl_mtx mutex in this very case.
> 
> Is my thinking correct?

Memory barriers on another CPU don't mean anything about the CPU thread 2 is 
on.  Memory barriers do not flush caches on other CPUs, etc.  Normally when 
objects are refcounted in a table, the table holds a reference on the object, 
but that doesn't seem to be the case here.  Have you tried doing something 
very simple in uifree():

{
	mtx_lock(&uihashtbl_mtx);
	if (refcount_release(...)) {
		LIST_REMOVE();
		mtx_unlock(&uihashtbl_mtx);
		...
		free();
	} else
		mtx_unlock(&uihashtbl_mtx);
}

I wouldn't use a more complex algo in uifree() unless the simple one is shown 
to perform badly.  Needless complexity is a hindrance to future maintenance.

Also, even if you do go with the more complex route, I'd rather you reduce 
diffs with the current code by keeping the test as 'uip->ui_ref == 0' and 
keeping the removal code in the if-block.

In chgproccnt() you should use atomic_fetchadd_long() to avoid a race when 
reading ui_proccnt.


	old = atomic_fetchadd_long(&uip->ui_proccnt, diff);
	if (old + diff < 0)
		printf("....");

OTOH, atomic_fetchadd_long() doesn't yet exist, so you will need to fix that, 
or just always use an atomic_cmpset() loop.

-- 
John Baldwin