UCX osc thread-safety concerns

[devreal]

Digging further through the UCX osc code, I noticed that there appear to be non-atomic memory accesses to variables that are potentially accessed concurrently in multiple threads, without proper locking. The following two instances seem suspicious in master and v4.0.x:

master:
mca_osc_ucx_component.num_incomplete_req_ops is incremented in ompi_osc_ucx_rget and ompi_osc_ucx_rput and decremented in req_completion. The counter seems to be only used in OMPI_OSC_UCX_REQUEST_ALLOC to trigger progress if deemed necessary. I'm not sure exactly what impact the race condition might have though.

v4.0.x:
Similarly, module->global_ops_num and module->per_target_ops_nums[target] are incremented and decremented in at least incr_and_check_ops_num() and ompi_osc_ucx_flush() without mutual exclusion. Again, this seems to trigger a flush if the number of outstanding ops grows too large (1M afaics) so the impact may be limited.

[artpol84]

@devreal, quite possible.

I've recently spotted an issue with another global variable (6678ac0).

We are currently evaluating our OSC implementations in MT context and will consider this issue as part of this effort.

Thank you once again for reporting.

[devreal]

It seems like there is a more important issue here than just the counter updates being unprotected: the hash table (opal_hash_table) used to store the window lock information is modified without protection. Opal's hash table implementation is not thread-safe afaics, the caller has to ensure mutual exclusion. If several threads attempt to lock a window at different targets they will put the hash table into an invalid state, leading to window synchronization errors. In effect, window locks are not thread-safe in the UCX osc, both in master and v4.0.x (haven't checked v3.0.x yet).

As a side note: using the opal hash table might not be a good choice after all as it's design for infrequent removals. Using an allocated array of ompi_osc_ucx_lock_t seems more efficient both in terms of memory and operation complexity...

[janjust]

Thanks @devreal, I've been looking at this with an MT-stress test, and it's been barfing on me and this explains why.

[jladd-mlnx]

@janjust - Tommy, is this issue resolved with your latest UCX OSC global table refactor #7681? Can we close this?

[gpaulsen]

@jladd-mlnx Can you please re-check the status of this?

[gpaulsen]

@open-mpi/ucx ?

[janjust]

@gpaulsen I'm actively working on a set of osc/ucx issues, this one included.
The thread safety tests in #7813 are passing with PR #8212 ; however, I still have a few issues to commit to that PR.

[gpaulsen]

@janjust thanks for the update. We're monitoring for v5.0.0.

[janjust]

@devreal going through some old old issues, in this one, the thread safety was fixed in #10709 for main/v5.0.x
v4.0 is super low on our priority list - we can leave it open or close (up to you)