v1.10: coll/libnbc: fix race condition with multi threaded apps

[ggouaillardet]

protect the mca_coll_libnbc_component.active_requests list with
the new mca_coll_libnbc_component.lock mutex.

Thanks Jie Hu for the report

Signed-off-by: Gilles Gouaillardet [email protected]

(back-ported from commit 2c94a3a)

[bosilca]

@ggouaillardet This code is pretty complicated, and honestly it deserves a comment on why this approach is correct. I haven't had time to dig deep into the code but from a quick look I am not sure about the correctness of this code in a multi-threaded scenario. You added a mutex to protect the modifications of the active request list, but because this list can be modified outside the progress function, your local next (the one protected by the newly added mutex) might become stale. However, if the progress is the only function allowed to remove active requests, then my comment is not holding and the proposed code is correct (but deserves documentation).

[jjhursey]

We can keep the discussion here (moving my question from PR #2441):

I'm curious why you didn't just reuse mca_coll_libnbc_component.progress_lock? Since the progress function is already protected by that lock then you would not have to grab another lock in there, but instead just protect the opal_list_append.

... Thinking about this a bit more, and let me know if I'm correct here...
The next stage of the collective is started from inside the NBC_Progress call, so the lock would need to be released across that call in order not to deadlock. In that case it would make sense to have a separate lock, but it feels like we might be able to get away with only one lock to reduce the overhead of the progress function. But I'm going to have to think more about that.

[ggouaillardet]

@bosilca the MPI layer opal_list_append() and the progress thread opal_list_remove_item(),
so i think the code i correct and it does deserve some documentation as you pointed. will do.

@jjhursey my understanding is that mca_coll_libnbc_component.progress_lock role is to avoid recursivity in the progress thread. i need to double check that, and hopefully get rid of this lock since it looks both inefficient and inelegant.
regardless, mca_coll_libnbc_component.progress_lock is held for "some time" by the progress thread, whereas mca_coll_libnbc_component.lock is only held to access the mca_coll_libnbc_component.active_requests. bottom line using a second lock avoids the MPI layer being slowed down by the progress thread.

[jjhursey]

@ggouaillardet This commit looks correct to me. It protects the .active_requests list when needed, and I don't see how we could get into a situation where we try to lock twice. Using the separate lock makes sense to me. The progress_lock is, as you mention, meant to prevent recursively calling that progress function. There might be a more elegant way to do this, but that can be a different PR.

@bosilca The progress function is the only place where requests are removed from this structure so I think the locking is safe.

I think that all this PR needs is a comment about what this lock is protecting. Maybe inside the ompi_coll_libnbc_progress function or where it is declared. Then I think this PR is good to merge.

[ggouaillardet]

@jjhursey thanks ! wil add comment today, sorry for the delay

[ggouaillardet]

@bosilca i added some comments in 1509816 and updated the PR for the three release branches

[bosilca]

Lovely. This looks ready to go !