[Bug] `cudaMemcpyAsync` blocks proxy thread making program hang

[liangyuRain]

Hi, we observe that when many kernels are pushed to launch queue, the cudaMemcpyAsync used in flushing fifo can get blocked due to full launch queue, which in turn blocks the whole proxy thread? This can make the program hang as kernels already in execution are waiting for port channel operations to complete. The problem is severe when setting CUDA_DEVICE_MAX_CONNECTIONS=1 as required by some implementations NVIDIA/Megatron-LM#533. It also seems that the cudaMemcpyAsync is not necessary in this case as port channel flush also check the trigger buffer in host memory?

Related to #503

[Binyang2014]

Could you clarify why cudaMemcpyAsync isn't necessary in this case? The flush operation here is intended to prevent the FIFO queue from becoming full. Are you suggesting we should avoid using cudaMemcpyAsync, or do you mean there's no need to update the tail at all?

[liangyuRain]

Based on my understanding, the cudaMemcpyAsync is to update the tailReplica on the device side, but the fifo can progress without updating tailReplica given the 2nd condition:

mscclpp/include/mscclpp/fifo_device.hpp

Line 94 in affca7d

(atomicLoad(&(this->triggers[curFifoHead % size].fst), memoryOrderRelaxed) != 0),

In our experiment, commenting out the cudaMemcpyAsync causes no issue and has no performance degradation. Given it can cause hanging in cases, we should probably avoid it or find other way to update tailReplica without the risk of blocking (launching copy from a thread other than the proxy thread?).

[Binyang2014]

If fifo queue size is large enough, I think there is no issue. But if tail+size < head, then for each push operation, it needs to check (atomicLoad(&(this->triggers[curFifoHead % size].fst), memoryOrderRelaxed) != 0)

mscclpp/include/mscclpp/fifo_device.hpp

Lines 48 to 66 in affca7d

	MSCCLPP_DEVICE_INLINE uint64_t push(ProxyTrigger trigger, int64_t maxSpinCount = 1000000) {
	uint64_t curFifoHead = atomicFetchAdd(this->head, (uint64_t)1, memoryOrderRelaxed);
	// make the last bit intentionally non-zero so that we can safely poll. Don't worry, we will change it back in host
	// side
	trigger.snd ^= ((uint64_t)1 << (uint64_t)63);
	// Only one of two conditions need to be met to proceed. Either the tail has advanced enough or where we need to
	// write to is 0. However, the first condition is faster to check since the tail is flushed periodically anyways but
	// for the second condition we need to read CPU memory.
	// As atomic access is slow, we first check using the bare pointer and then use the atomic load if the
	// condition is not met.
	if (curFifoHead >= size + *(this->tailReplica)) {
	OR_POLL_MAYBE_JAILBREAK((curFifoHead >= size + atomicLoad(this->tailReplica, memoryOrderRelaxed)),
	(atomicLoad(&(this->triggers[curFifoHead % size].fst), memoryOrderRelaxed) != 0),
	maxSpinCount);
	}
	ProxyTrigger* triggerPtr = &(this->triggers[curFifoHead % size]);

.

I don't understand why launch copy from another thread may solve this issue. The different thread will have different launch queue?

[liangyuRain]

Checking the atomicLoad is necessary, but the tailReplica is not necessary for progress.

The situation we have is like this. When the cuda kernel launch queue is full, the cudaMemcpyAsync here becomes a blocking call as in link. The call needs to wait for kernels in execution to finish, so new kernels can be popped from the launch queue. However, because the cudaMemcpyAsync is blocking the proxy thread, the port channel operations performed by the kernels currently in execution cannot progress and are blocking the kernels in execution, resulting in a deadlock.

Launching copy from another thread is helpful because it prevents the cudaMemcpyAsync from blocking the proxy thread, so even if the launch queue is full, proxy thread can still progress and not blocking the kernels in execution.

[Binyang2014]

Makes sense to me, we'd better to use another thread for cudaMemcpyAsync

[Binyang2014]

You are using IB connection for portChannel right? I think cudaMemcpyAsync is used for putting data for cudaIpc connection, which may also cause the similar issue

[liangyuRain]

Yes I am using IB connection for portChannel. The cudaMemcpyAsync causing problem in our case is not the ones used by cudaIpcConnection, but the one used by Fifo::flushTail. The same deadlock can definitely also arise in the case of cudaIpc port channel.

[chhwang]

@liangyuRain We are experimenting with a version that does not require flushing the tail. Please check it out if it works good for your team. #544