Re: Lockless uidinfo.

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@xxxxxxxxxxx> [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
_______________________________________________
freebsd-arch@xxxxxxxxxxx mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "freebsd-arch-unsubscribe@xxxxxxxxxxx"

Relevant Pages

  • Re: Lockless uidinfo.
    ... Memory barriers on another CPU don't mean anything about the CPU thread 2 is ... objects are refcounted in a table, the table holds a reference on the object, ... older code that used atomic_cmpset_longloop in 'diff> 0' case, ...
    (freebsd-arch)
  • [PATCH (take 4)] Documentation/memory-barriers.txt: various fixes
    ... - The CPU memory barriers. ... MMIO write barrier. ... CPU's caches by some other cache event: ...
    (Linux-Kernel)
  • [PATCH] Documentation/memory-barriers.txt: various fixes
    ... - The CPU memory barriers. ... MMIO write barrier. ... the first load retrieves the address to which the second ...
    (Linux-Kernel)
  • 6.2 custom kernel build HELP
    ... reference to `ng_make_node_common' ... CPU: IntelXeonCPU 2.80GHz ... acpi0: on motherboard ... <ACPI PCI bus> on pcib0 ...
    (freebsd-questions)
  • [PATCH 6/8] unify smp parts of system.h
    ... * Force strict CPU ordering. ... * and add some real memory barriers if so. ... * Ordering is not guaranteed by anything other than these primitives, ... * in cases like this where there are no data dependencies. ...
    (Linux-Kernel)