MPI attribute code need threading audit

[ompiteam]

In reviewing bug https://svn.open-mpi.org/trac/ompi/ticket/176, I have determined that the locking code in source:/trunk/ompi/attribute/attribute.c may not be thread safe in all cases and needs to be audited. It was written with the best of intentions :-) but then never tested and I think there are some obscure race conditions that ''could'' happen.

For example, in ompi_attr_create_keyval(), we have the following:

    OPAL_THREAD_LOCK(&alock);
    ret = CREATE_KEY(key);
    if (OMPI_SUCCESS == ret) {
        ret = opal_hash_table_set_value_uint32(keyval_hash, *key, attr);
    }
    OPAL_THREAD_UNLOCK(&alock);
    if (OMPI_SUCCESS != ret) {
        return ret;
    }

    /* Fill in the list item */

    attr->copy_attr_fn = copy_attr_fn;
    /* ...fill in more attr->values ... */

This could clearly be a problem since we set the empty keyval on the hash and therefore it's available to any other thread as soon as the lock is released -- potentially ''before'' we finish setting all the values on the attr variable (which is poorly named -- it's a keyval, not an attribute).

This one problem is easily fixed (ensure to setup attr before we assign it to the keyval hash), but it reflects that the rest of the attribute code should really be audited. Hence, this ticket is a placemarker to remember to audit this code because it may not be thread safe.

[ompiteam]

Imported from trac issue 184. Created by jsquyres on 2006-07-05T12:39:43, last modified: 2008-05-29T18:01:57

[ompiteam]

Trac comment by jsquyres on 2007-12-03 09:20:29:

If threading support is going into v1.3, this ticket should be moved to the v1.3 milestone.

[ompiteam]

Trac comment by jsquyres on 2008-05-29 18:01:57:

This will not happen for v1.3.

[jsquyres]

@gpaulsen if someone could write a multi-threaded attribute test (i.e., a bunch of threads all simultaneously reading/writing attributes on an object), that would easily show any problems that we may have here.

Honestly, the attribute code does not need to be high performance. I would not be heartbroken at all if the fine-grained locking code in the attribute code went away and was replaced with a simpler one-global-lock-for-all-attribute-things approach.

This is not a sexy issue, but it still is important for MPI_THREAD_MULTIPLE reasons.

[jsquyres]

I'm guessing this issue was addressed long ago. The code in ompi/attribute/attribute.c does not resemble the code cited in the body of this issue -- the locking scheme is quite different.

Also, there's a ton of issues/PRs that reference this one, but I think most (all?) of them are accidents: e.g., people just citing the 4th item in a list, and github automatically linked it back here to issue number 4.

I'm going to close this issue as completed.