Open MPI logo

Open MPI Development Mailing List Archives

  |   Home   |   Support   |   FAQ   |   all Development mailing list

Subject: [OMPI devel] Ticket #1267 - thread locks in ompi_proc_t code
From: Ralph H Castain (rhc_at_[hidden])
Date: 2008-07-07 10:22:11


I am seeking input before making a change to the ompi/proc/proc.c code to
resolve the referenced ticket. The change could potentially impact how the
ompi_proc_t struct is used in the rest of the MPI code base. If this doesn't
impact you, please ignore the remainder of this note.

I was asked last week to take a look at ticket #1267, filed by Tim Prins
several months ago. This ticket references a report on the devel list about
thread locks when calling comm_spawn and using MPI_Init_thread. The thread
lock is caused by the constructor/destructor in the ompi_proc_t class
explicitly removing the referenced ompi_proc_t from the static local global
ompi_proc_list, and calling OPAL_THREAD_LOCK/OPAL_THREAD_UNLOCK around that
list operation.

As far as I can see, Tim correctly resolved the constructor conflict by
simply removing the thread lock/unlock and list append operation from the
constructor. A scan of the code shows that OBJ_NEW is -only- called from
within the ompi/proc/proc.c code, so this won't be an issue.

However, I noted several issues surrounding the creation and release of
ompi_proc_t objects that -may- cause problems in making a similar change to
the destructor to fix the rest of the threading problem. These may have been
created in response to the list modification code currently existing in the
ompi_proc_t object destructor - or they may be due to other factors.

Specifically, the MPI code base outside of ompi/proc/proc.c:

1. -never- treats the ompi_proc_t as an opal object. Instead, the code
specifically calls calloc to create space for the structures, and then
manually constructs them.

2. consistently calls OBJ_RELEASE on the resulting structures, even though
they were never created as opal objects via OBJ_NEW.

I confess to being puzzled here as the destructor will attempt to remove the
referenced ompi_proc_t* pointer from the ompi_proc_list in ompi/proc/proc.c,
but (since OBJ_NEW wasn't called) that object won't be on the list. Looking
at the code itself, it appears we rely on the list function to realize that
this object isn't on the list and ignore the request to remove it. We don't
check the return code from the opal_list_remove_item, and so ignore the
returned error.

My point here is to seek comment about the proposed fix for the problem
referenced in the ticket. My proposal is to remove the thread lock/unlock
and list manipulation from the ompi_proc_t destructor. From what I can see
(as described above), this should not impact the rest of the code base. I
will then add thread lock/unlock protection explicitly to ompi_proc_finalize
to protect its list operations.

It appears to me that this change would also open the way to allowing the
remainder of the code base to treat ompi_proc_t as an object, using OBJ_NEW
to correctly and consistently initialize those objects. I note that any
change to ompi_proc_t today (which has occurred in the not-too-distant
past!) can create a problem throughout the current code base due to the
manual construction of this object. This is why we have objects in the first
place - I suspect people didn't use OBJ_NEW because of the automatic change
it induced in the ompi_proc_list in ompi/proc/proc.c.

Any comments would be welcome.
Ralph