已合并
[shmem] fix with aclshmem #29045
[shmem] fix with aclshmem #29045
已合并
王超创建于 1月5日
王超成员
1月5日

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind bug
/kind task
/kind feature

What does this PR do / why do we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewers:

likedislike
Pull Request已成功合入, 合并人@ascend-robot
(感谢 王超 的贡献)
王超成员
1月5日 创建了 pull request,commit c24cdea2
ascend-robot
ascend-robot成员
1月5日 评论:

Thank your for your pull-request.

The full list of commands accepted by me can be found at here.

You can get sig-info at here

likedislike
ascend-robot
ascend-robot成员
1月5日 评论:

以下是根据您提交的修改文件推荐的Reviewer和Committer序列,需各模块评审通过后方可合入

Module List Reviewers Committers
repo-Ascend/pytorch yuhaiyan, wangchao430, liangsongwei, shaoyf, chuboning chenhao_1209, shaoyf, medivh-x, anyrenwei, liwei386
likedislike
ascend-robotascend-robot成员
1月5日 添加了label:ascend-cla/yes
王超成员
1月5日 评论:

compile

likedislike
AtlasAccount
AtlasAccount成员
1月5日 评论:

问题/功能描述

本次PR对NPU共享内存(SHMEM)库进行了全面的API命名重构与功能增强。核心目标是统一接口命名规范,将原有的Shmem_前缀函数和数据结构更新为Aclshmem_ACLSHMEM_前缀,并引入新的初始化标志和属性结构以支持更灵活的通信后端(如SDMA、RoCE)。同时,通过动态检测底层库版本,实现了对新旧API的向后兼容性支持,确保系统在不同版本的底层库环境下均能正确初始化和管理共享内存。

修改方案描述

修改方案主要包括三个方面:1) 接口重构:在头文件中移除旧定义,重命名数据类型,并新增ACLSHMEMX系列的初始化属性、唯一ID结构及通信引擎枚举,为高级功能提供配置支持。2) 兼容性实现:在接口层新增Aclshmemx_*系列函数,通过运行时检测动态选择调用新版或旧版底层API,确保初始化逻辑的向后兼容。3) 调用点更新:在系统清理及内存管理(如分配、释放、指针获取)等关键路径中,将所有Shmem_*函数调用统一替换为对应的Aclshmem_*版本,保持整个代码库接口命名的一致性。

likedislike
AtlasAccount
AtlasAccount成员1月5日进行代码检视1
third_party/shmem/include/shmem_host_def.h
@@ -115,0 +200,4 @@
200+ int n_pes;
201+ char ip_port[ACLSHMEM_MAX_IP_PORT_LEN];
202+ uint64_t local_mem_size;
203+ aclshmem_init_optional_attr_t option_attr = {(1 << 16) + sizeof(aclshmem_init_optional_attr_t), ACLSHMEM_DATA_OP_MTE, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT};
AtlasAccount
AtlasAccount1月5日评论:

代码结构与可维护性: 在C语言头文件中使用了C++风格的默认成员初始化。结构体aclshmemx_init_attr_t的成员option_attr使用了C++的默认初始化语法(= {...})。虽然文件使用了#ifdef __cplusplus来兼容C++,但该结构体定义在extern "C"块之外(第219-221行),这意味着在纯C编译环境中(如C编译器或C++编译器编译C代码时),这种语法是无效的,会导致编译错误。

问题类型: 代码结构与可维护性
文件路径: third_party/shmem/include/shmem_host_def.h
行号: 203
问题代码:

    aclshmem_init_optional_attr_t option_attr = {(1 << 16) + sizeof(aclshmem_init_optional_attr_t), ACLSHMEM_DATA_OP_MTE, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT};

修改建议:

将默认初始化移除,改为在文档中说明用户必须显式初始化该字段,或者提供一个单独的初始化函数/宏(如`ACLSHMEMX_INIT_ATTR_INITIALIZER`)来设置默认值。例如:
1. 删除`= {...}`部分。
2. 添加一个初始化宏:
```c
#define ACLSHMEMX_INIT_ATTR_INITIALIZER \
    { \
        0, 0, "", 0, \
        { (1 << 16) + sizeof(aclshmem_init_optional_attr_t), ACLSHMEM_DATA_OP_MTE, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT, DEFAULT_TIMEOUT, -1 }, \
        NULL \
    }

这样既保证了C/C++兼容性,又提供了便捷的初始化方式。


---
*此评论由代码审查工具自动生成*
likedislike
AtlasAccount
AtlasAccount成员1月5日进行代码检视1
third_party/shmem/include/shmem_host_def.h
@@ -75,0 +114,4 @@
114+*/
115+/// \def ACLSHMEM_UNIQUEID_INITIALIZER
116+/// \brief Initializer macro for the ACLSHMEM unique ID structure
117+#define ACLSHMEM_UNIQUEID_INITIALIZER \
AtlasAccount
AtlasAccount1月5日评论:

代码结构与可维护性: 宏ACLSHMEM_UNIQUEID_INITIALIZER的初始化列表不完整。结构体aclshmemx_uniqueid_t有四个成员:versionmy_pen_pesinternal。但该宏只初始化了versioninternal(通过一个嵌套的{0}),遗漏了my_pen_pes。这会导致使用该宏初始化的结构体中my_pen_pes成员处于未初始化状态,可能包含任意值,从而引发未定义行为。

问题类型: 代码结构与可维护性
文件路径: third_party/shmem/include/shmem_host_def.h
行号: 117
问题代码:

#define ACLSHMEM_UNIQUEID_INITIALIZER                   \
    {                                                   \
        ACLSHMEM_UNIQUEID_VERSION,                      \
        {                                               \
            0                                           \
        }                                               \
    }

修改建议:

修正初始化宏,显式初始化所有成员。根据结构体定义,应初始化`version`、`my_pe`、`n_pes`和`internal`。例如:
```c
#define ACLSHMEM_UNIQUEID_INITIALIZER                   \
    {                                                   \
        ACLSHMEM_UNIQUEID_VERSION,                      \
        0, /* my_pe */                                  \
        0, /* n_pes */                                  \
        { 0 } /* internal */                            \
    }

这样确保结构体被完全初始化,避免未定义行为。


---
*此评论由代码审查工具自动生成*
likedislike
AtlasAccount
AtlasAccount成员1月5日进行代码检视1
third_party/shmem/include/shmem_host_def.h
@@ -59,0 +71,4 @@
71+ 
72+/// \def ACLSHMEM_MAX_IP_PORT_LEN
73+/// \brief Maximum length of the IP and port string in ACLSHMEM (including null terminator)
74+#define ACLSHMEM_MAX_IP_PORT_LEN 64
AtlasAccount
AtlasAccount1月5日评论:

代码一致性: 宏'ACLSHMEM_MAX_IP_PORT_LEN'使用#define定义,而其他类似的长度常量如'SHMEM_UNIQUE_ID_INNER_LEN'和'ACLSHMEM_UNIQUE_ID_INNER_LEN'使用constexpr定义。这种不一致的常量定义方式降低了代码的一致性。

问题类型: 代码一致性
文件路径: third_party/shmem/include/shmem_host_def.h
行号: 74
问题代码:

#define ACLSHMEM_MAX_IP_PORT_LEN 64

修改建议:

统一常量定义风格。由于这是C/C++混合头文件,建议:
1. 如果支持C++11及以上,使用constexpr:
   constexpr uint16_t ACLSHMEM_MAX_IP_PORT_LEN = 64;
2. 如果需要C兼容,使用enum或static const:
   enum { ACLSHMEM_MAX_IP_PORT_LEN = 64 };

此评论由代码审查工具自动生成

likedislike
王超成员
1月5日 评论:

compile

likedislike
王超成员
1月5日 update merge request[project id: 7404318, iid: 29045, commit_id: 8e562f5daecf7fb47ba27972cf547691c1e88b90] virtual merging success
王超成员
1月5日 强制推送  1 个提交:0800a7ec-[shmem] fix with aclshmemxxx
王超成员
1月5日 update merge request[project id: 7404318, iid: 29045, commit_id: 3c126275ad0114af508c1c8db7963ccb6dab66c4] virtual merging success
ascend-robot
ascend-robot成员
1月5日 评论:

以下是根据您提交的修改文件推荐的Reviewer和Committer序列,需各模块评审通过后方可合入

Module List Reviewers Committers
repo-Ascend/pytorch duchengkun, zqwenn, liwei386, dilililiwhy, ShaoFeifan liwei386, dilililiwhy, shaoyf, wangchao285, wangqiang160
likedislike
ascend-robot
ascend-robot成员
1月5日 评论:

CLA Signature Pass

@wangchao430, thanks for your pull request. All authors of the commits have signed the CLA. 👍

likedislike
AtlasAccountAtlasAccount成员
1月5日 添加了label:ci-pipeline-failed
王超成员
1月5日 评论:

compile

likedislike
ascend-robotascend-robot成员
1月5日 删除了label:ci-pipeline-failed
ascend-robotascend-robot成员
1月5日 添加了label:ci-pipeline-running
AtlasAccount
AtlasAccount成员
1月5日 评论:

/torch_npu/csrc/distributed/symm_mem/NPUSHMEMInterface.cpp

likedislike
ascend-robotascend-robot成员
1月5日 删除了label:ci-pipeline-running
ascend-robotascend-robot成员
1月5日 添加了label:ci-pipeline-passed
ascend-robot
ascend-robot成员
1月5日 评论:
流水线 PR-pipeline_pytorch#1594已完成
阶段 任务名 状态 详情
编译构建 Build_X86 >>>
Build_LibTorch >>>
Build_ARM >>>
Build_ARM_inductor 🛑 >>>
Build_X86_py311 🛑 >>>
Build_ARM_py311 🛑 >>>
dist_test_or_not >>>
恶意代码检查 Antipoison >>>
编码安全与规范检查 CodeCheck >>>
check_error >>>
开源片段检查 SCA >>>
开发者测试 UT_DIST_X86 >>>
UT_X86_Part_01 >>>
UT_X86_Part_02 >>>
UT_ARM_A2_Part_01 🛑 >>>
UT_ARM_A2_Part_02 🛑 >>>
UT_inductor_arm 🛑 >>>
流水线 PR-pipeline_pytorch >>>
likedislike
shaoyf成员
1月6日 评论:

/approve

likedislike
ascend-robotascend-robot成员
1月6日 添加了label:lgtm
zhangqiongwen成员
1月6日 评论:

/approve

likedislike
ascend-robotascend-robot成员
1月6日 添加了label:approved
ascend-robot
ascend-robot成员
1月6日 评论:

Review Guide

This Pull-Request Passes Review.
Committers who writed a comment of /approve are: zqwenn, shaoyf.
Reviewers who writed a comment of /lgtm are: zqwenn, shaoyf.

likedislike
ascend-robotascend-robot成员
1月6日 合入了pull request