[PR #353] [MERGED] fix combine timeout due to delayed forwarder min head update #533

New issue

Closed

opened 2026-10-27 21:03:44 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-27 21:03:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/DeepEP/pull/353
Author: @ZhiyiHu1999
Created: 8/6/2025
Status: ✅ Merged
Merged: 8/26/2025
Merged by: @LyricZhao

Base: main ← Head: fix/combine_timeout_due_to_lacking_forwarder_min_head_update

📝 Commits (2)

fecd746 fix combine timeout due to forwarder min head update
4603442 Update head before and after combine_token; add assertion for nvl_buffer_size_per_rdma_rank

📊 Changes

1 file changed (+4 additions, -1 deletions)

View changed files

📝 csrc/kernels/internode.cu (+4 -1)

📄 Description

This PR provides a fix for #314.

The configuration that causes the combine timeout in #314 is:
num_sms = 48
nvl_chunk_size = 32
nvl_buffer_size = 80
rdma_chunk_size = 20
rdma_buffer_size = 128

In kNVLAndRDMAForwarder, we first read expected_head, then compare with the tail, combine the token, issue RDMA send, finally update forwarder_nvl_head with expected_head + 1, then the coordinator updates the min_head.

In the above configuration, a rare case will happen: in the first iteration of the below 'for' loop, some threads will be trapped in the while loop comparing nvl_tail and nvl_head. As we update the forwarder_nvl_head at the end of the iteration, even if eapected_head is larger than the forwarder_nvl_head, it won't get updated, same for min_head.

github.com/deepseek-ai/DeepEP@26cf250a33/csrc/kernels/internode.cu (L1646-L1664)

So, I just simply update forwarder_nvl_head with expected_head (not expected_head + 1 in the fix) for the rare cases.

Besides, for the kRDMAReceiver, the design is to update rdma_receiver_rdma_head with expected_head, before consuming the token. So maybe an update for rdma_receiver_rdma_head with expected_head + 1 should be added after the token consuming in the last 'for' loop iteration. I guess there may be rare cases where this could potentially cause a timeout. But I haven't found a configuration for that.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/DeepEP/pull/353 **Author:** [@ZhiyiHu1999](https://github.com/ZhiyiHu1999) **Created:** 8/6/2025 **Status:** ✅ Merged **Merged:** 8/26/2025 **Merged by:** [@LyricZhao](https://github.com/LyricZhao) **Base:** `main` ← **Head:** `fix/combine_timeout_due_to_lacking_forwarder_min_head_update` --- ### 📝 Commits (2) - [`fecd746`](https://github.com/deepseek-ai/DeepEP/commit/fecd746e61f1e788720b168d802941e31616187f) fix combine timeout due to forwarder min head update - [`4603442`](https://github.com/deepseek-ai/DeepEP/commit/46034421344c04255e199c273709209818cf22af) Update head before and after combine_token; add assertion for nvl_buffer_size_per_rdma_rank ### 📊 Changes **1 file changed** (+4 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `csrc/kernels/internode.cu` (+4 -1) </details> ### 📄 Description This PR provides a fix for #314. The configuration that causes the combine timeout in #314 is: num_sms = 48 nvl_chunk_size = 32 nvl_buffer_size = 80 rdma_chunk_size = 20 rdma_buffer_size = 128 In `kNVLAndRDMAForwarder`, we first read `expected_head`, then compare with the `tail`, combine the token, issue RDMA send, finally update `forwarder_nvl_head` with `expected_head + 1`, then the coordinator updates the `min_head`. In the above configuration, a rare case will happen: in the **first** iteration of the below 'for' loop, some threads will be trapped in the while loop comparing `nvl_tail` and `nvl_head`. As we update the `forwarder_nvl_head` at the end of the iteration, even if `eapected_head` is larger than the `forwarder_nvl_head`, it won't get updated, same for `min_head`. https://github.com/deepseek-ai/DeepEP/blob/26cf250a3388c770ee628d2fd0271e1c6c52157a/csrc/kernels/internode.cu#L1646-L1664 So, I just simply update `forwarder_nvl_head` with `expected_head` (not `expected_head + 1` in the fix) for the rare cases. Besides, for the `kRDMAReceiver`, the design is to update `rdma_receiver_rdma_head` with `expected_head`, before consuming the token. So maybe an update for `rdma_receiver_rdma_head` with `expected_head + 1` should be added after the token consuming in the last 'for' loop iteration. I guess there may be rare cases where this could potentially cause a timeout. But I haven't found a configuration for that. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-27 21:03:44 +00:00