[TransferEngine] feat: Support CXL shared memory, and provide simple unit tests. by hemist · Pull Request #670 · kvcache-ai/Mooncake

hemist · 2025-07-25T07:16:12Z

This PR providesd a transfer-engine framework based on CXL shared memory. In the current implementation, submitTransfer used memcpy() for data transfer. Future work may switch to DSA to avoid CPU cache flushes.

Copilot

Pull Request Overview

This PR introduces CXL (Compute Express Link) shared memory support to the transfer-engine framework by implementing a new CxlTransport class and providing basic unit tests. The implementation uses memcpy() for data transfer with plans to potentially upgrade to DSA (Data Streaming Accelerator) in the future.

Key changes include:

Implementation of CxlTransport class with CXL device initialization, memory mapping, and data transfer capabilities
Extension of metadata system to support CXL segment descriptors and buffer management
Addition of comprehensive unit tests for CXL transport operations

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
cxl_transport.cpp	Core implementation of CXL transport with device initialization, memory mapping, and transfer operations
cxl_transport.h	Header defining CxlTransport class interface and CXL-specific functionality
cxl_transport_test.cpp	Unit tests for CXL transport operations including multi-write and multi-read scenarios
transfer_metadata.cpp	Extended metadata encoding/decoding to support CXL segment descriptors
transfer_metadata.h	Added CXL-specific fields to metadata structures
CMakeLists.txt	Build configuration for CXL transport test
multi_transport.cpp	Integration of CXL transport into multi-transport system
transfer_engine.cpp	Auto-discovery setup for CXL transport
transport.h	Updated Slice structure for CXL transport
common.cmake	Build system support for CXL feature flag

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

mooncake-transfer-engine/tests/cxl_transport_test.cpp

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

mooncake-transfer-engine/src/transfer_metadata.cpp

stmatengss · 2025-07-28T03:40:42Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

+        cxl_dev_path = std::string(env_cxl_dev_path);
+        cxl_dev_size = cxlGetDeviceSize(cxl_dev_path);
+    } else {
+        cxl_dev_path = "/dev/dax0.0";


The code is hard-coded. Can you modify it to support auto-detection? If no CXL device is found, it should trigger error handling.

stmatengss · 2025-07-28T09:42:51Z

mooncake-transfer-engine/src/transfer_engine.cpp

 #else
+
+#ifdef USE_CXL
+    multi_transports_->installTransport("cxl", local_topology_);


need to check its return status?

stmatengss · 2025-07-28T09:49:29Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

+    while (fgets(line, sizeof(line), fp) != NULL) {
+        char *size_str = strstr(line, "\"size\":");
+        if (size_str) {
+            size_str += 7; // skip over "\"size\":"
+
+            while (*size_str && (*size_str < '0' || *size_str > '9')) {


Why not calculate its size using posix API?

stmatengss · 2025-07-28T09:50:50Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

+        LOG(ERROR) << "CxlTransport::cxlMemcpy invalid arguments: null pointer provided.";
+        return -1; // null pointer
+    }
+    std::memcpy(dest, src, size);


If it is the same address, don't require memcpy

mooncake-common/common.cmake

…rom daxctl to env variable && fix some bugs.

Cxl engine pr

…o match the base class 'Transport'.

stmatengss

Pass Tests. LGTM

mooncake-common/common.cmake

staryxchen · 2025-07-30T10:56:33Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

+    const char* env_cxl_dev_path = std::getenv("MC_CXL_DEV_PATH");

-CxlTransport::~CxlTransport() {}
+    LOG(INFO) << "MC_CXL_DEV_PATH: " << env_cxl_dev_path;


print it only when MC_CXL_DEV_PATH is set?

staryxchen · 2025-07-30T10:58:05Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

+    // for now, get cxl_shm size from env
+    const char* env_cxl_dev_size = std::getenv("MC_CXL_DEV_SIZE");
+
+    LOG(INFO) << "MC_CXL_DEV_SIZE: " << env_cxl_dev_size;


same as line R44

staryxchen · 2025-07-30T10:59:57Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

-                                    TransferStatus &status) {
-    return 0;
+CxlTransport::~CxlTransport() {
+    munmap(cxl_base_addr, cxl_dev_size);


check cxl_base_addr sanity before munmap it

staryxchen · 2025-07-30T11:00:58Z

mooncake-transfer-engine/src/transport/cxl_transport/cxl_transport.cpp

+
+int CxlTransport::cxlDevInit()
+{
+    int fd = open(cxl_dev_path, O_RDWR);


check cxl_dev_path sanity

staryxchen · 2025-08-01T04:53:50Z

Hi @stmatengss
I have reviewed but forget to submit, sry. Could you modify it a little more?

stmatengss · 2025-08-01T05:50:01Z

Hi @stmatengss I have reviewed but forget to submit, sry. Could you modify it a little more?

Sure. Any reviews are welcome.

…unit tests. (kvcache-ai#670) * [TransgerEngine] Add cxl transport test * [Transfer Engine] Add cxl transfer support * [TransferEngine] Update the way of obtaining the size of CXL memory from daxctl to env variable && fix some bugs. * [CXL transport] refactor code & add test * [TransferEngine] fix: modify the parameters of submitTransferTask() to match the base class 'Transport'. * [TransferEngine] fix: update etcd address * [TransferEngine] fix: unit tests error caused by cxl_transport * Update mooncake-common/common.cmake * [TransferEngine] fix: add some validity checks. --------- Co-authored-by: YiHong Lian <lianyihong@ieisystem.com> Co-authored-by: Karl Zhao <zhaozhiyuan05@ieisystem.com> Co-authored-by: Teng Ma <sima.mt@alibaba-inc.com> Co-authored-by: Teng Ma <805522925@qq.com>

[TransgerEngine] Add cxl transport test

b667c53

stmatengss requested review from Copilot and stmatengss July 25, 2025 07:42

Copilot AI reviewed Jul 25, 2025

View reviewed changes

[Transfer Engine] Add cxl transfer support

248a21a

hemist force-pushed the cxl-engine-pr branch from 9aa3623 to 248a21a Compare July 25, 2025 08:11

stmatengss self-assigned this Jul 27, 2025

stmatengss reviewed Jul 28, 2025

View reviewed changes

stmatengss requested changes Jul 29, 2025

View reviewed changes

mooncake-common/common.cmake Show resolved Hide resolved

lianyihong and others added 5 commits July 30, 2025 11:05

[TransferEngine] Update the way of obtaining the size of CXL memory f…

06ed115

…rom daxctl to env variable && fix some bugs.

[CXL transport] refactor code & add test

af3ad8a

Merge branch 'kvcache-ai:main' into cxl-engine-pr

cba1648

Merge pull request #1 from stmatengss/cxl-engine-pr

1bddc52

Cxl engine pr

[TransferEngine] fix: modify the parameters of submitTransferTask() t…

988bb8c

…o match the base class 'Transport'.

hemist force-pushed the cxl-engine-pr branch from d621427 to 988bb8c Compare July 31, 2025 01:08

lianyihong added 2 commits July 31, 2025 06:24

[TransferEngine] fix: update etcd address

fa85d8b

[TransferEngine] fix: unit tests error caused by cxl_transport

09be1e8

stmatengss approved these changes Jul 31, 2025

View reviewed changes

stmatengss reviewed Aug 1, 2025

View reviewed changes

mooncake-common/common.cmake Outdated Show resolved Hide resolved

Update mooncake-common/common.cmake

71fe1bc

stmatengss approved these changes Aug 1, 2025

View reviewed changes

staryxchen requested changes Aug 1, 2025

View reviewed changes

[TransferEngine] fix: add some validity checks.

59dcd1a

stmatengss merged commit 7cdae81 into kvcache-ai:main Aug 1, 2025
10 checks passed

stmatengss added the openanolis label Aug 25, 2025

qiuweit7 mentioned this pull request Jan 21, 2026

[Store][Feature] Add CXL storage for mooncake_store #1365

Merged

15 tasks

Comments

Conversation

hemist commented Jul 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stmatengss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staryxchen commented Aug 1, 2025

Uh oh!

stmatengss commented Aug 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants