OSC/UCX: preserve the accumulate ordering for overlapping buffers during acc-lock less epocs and setting a proper wpool context mutex type

[MamziB]

OSC/UCX: preserve the accumulate ordering for overlapping buffers during acc-lock less epocs and setting a proper wpool context mutex type

Signed-off-by: Mamzi Bayatpour mbayatpour@nvidia.com
Co-authored-by: Tomislav Janjusic tomislavj@nvidia.com

[devreal]

Thanks for this work @MamziB! Just a few questions/hints in the comments.

[MamziB]

@devreal Thanks for the comments. Please take a look at the updated commit.

[devreal]

Is it safe to hold the ctx mutex during the flush?

[MamziB]

yes, the ctx mutex is recursive, so it should be ok. it does not hang.

[devreal]

Thanks, it good now. Two more thoughts though:

1. Should we store the rank with the base and tail to avoid false positives and allow us to flush to endpoint instead of the worker?
2. Instead of resetting the array after a flush, it might be more efficient to reset that particular entry in the completion callback. That probably wouldn't even need the mutex in the callback since we're only resetting two integer values to 0 (the window of opportunity for a race is small and even if there is one in the worst we don't recognize that entry as being empty). And there is a good chance that it would reduce the need for flushes since entries are continually reset once operations complete.

Those are optimizations though and we could do them as a follow-up. I'll approve what we have now 👍

[janjust]

@devreal I'll open an issue to track the suggestions as a follow up - thanks for the comments.

[janjust]

@MamziB v5.0.x backport please, when you can.

[MamziB]

@devreal Thanks for the suggestions. My comments are as follows:

1. Yes, we could do that. However, the current implementation of osc ucx flush mandates that all the nonblocking accumulates finish before the flush is done (we flush the worker for that). Therefore, I am not sure if we gain much for changing to flush from target ep to worker. But for sure we need to investigate this suggestion's performance benefits.

2. Actually, this is similar to how I was envisioning the design at the first glance. Adding the mem info to nonblocking req and clearing off the corresponding mem from the array when req is completed. However, since accumulate already has its own lock, this patch is only relevant when we do not have acc lock (meaning only if the application calls MPI_Win_lock/lockall with EXCLUSIVE). Therefore, for the time being, I tried to isolate the mem handling from all other parts of the code. The benefit of the current patch is that all other types of RMA SYNCs will not be impacted by this patch. It also simplifies the osc accumulate design.

#11184