Repository navigation
UCT/IB/MLX5: Add AArch64 ST64B BF copy mode - #11542
Conversation
|
Initially tested on supporting platform with added gtest. |
Add UCT_IB_MLX5_HAVE_ST64B so the ST64B build condition is defined once.
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap: the Residual note: |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap: the |
|
🤖 Starting review — findings will be posted here when done. |
|
|
||
| ep = ucs_derived_of(m_e1->ep(0), uct_rc_mlx5_base_ep_t); | ||
| ASSERT_TRUE(ep->tx.wq.reg != NULL); | ||
| ASSERT_EQ(UCT_IB_MLX5_MMIO_MODE_BF_POST, ep->tx.wq.reg->mode); |
There was a problem hiding this comment.
Can we also assert the selected copy mode here, next to the mmio_mode check? test_bf_copy() asserts reg->mode but never wq->bf_copy_mode. On LS64 hardware auto also resolves to ST64B, so if BF_COPY_MODE ever stops reaching the txwq (six call sites now thread it) the generic case would silently run ST64B and still pass. Something like an expected-mode argument to test_bf_copy() plus EXPECT_EQ(expected, ep->tx.wq.bf_copy_mode) under #if UCT_IB_MLX5_HAVE_ST64B.
|
|
||
| self->tx.mmio_mode = config->mlx5_common.mmio_mode; | ||
| self->tx.wq.super.type = UCT_IB_MLX5_OBJ_TYPE_LAST; | ||
| self->tx.mmio_mode = config->mlx5_common.mmio_mode; |
There was a problem hiding this comment.
minor: assignment alignment — docs/CodeStyle.md: "the longest left-hand side gets one space before =, others align to it." The longest LHS here has four.
| self->tx.mmio_mode = config->mlx5_common.mmio_mode; | |
| self->tx.mmio_mode = config->mlx5_common.mmio_mode; | |
| self->tx.bf_copy_mode = config->mlx5_common.bf_copy_mode; | |
| self->tx.wq.super.type = UCT_IB_MLX5_OBJ_TYPE_LAST; |
|
Residual coverage gap (already reported on this PR, not repeated inline): the |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: Log timestamps show the test started at Implicated commit: File: src/uct/ib/mlx5/ib_mlx5.c:802-837 ( Suggested fix:
Related: PR #11542 (#11542, "UCT/IB/MLX5: Add AArch64 ST64B BF copy m
|
| ucs_offsetof(uct_ib_mlx5_iface_config_t, mmio_mode), | ||
| UCS_CONFIG_TYPE_ENUM(uct_ib_mlx5_mmio_modes)}, | ||
|
|
||
| {"BF_COPY_MODE", "auto", |
There was a problem hiding this comment.
do you think we can define new mmio mode instead?
There was a problem hiding this comment.
Merging them into one user-facing value doesn't really work, because the two autos answer different questions: MMIO auto depends on thread mode, need_lock and WC UAR support, while BF copy auto depends only on the LS64 HWCAP. It would also lose the mixed cases, e.g. MMIO_MODE=auto BF_COPY_MODE=generic to keep automatic doorbell selection but disable ST64B.
I did prototype merging them internally (following recent bot comment): resolve both at setup time into extra uct_ib_mlx5_mmio_mode_t values past _LAST (BF_POST_ST64B, BF_POST_MT_ST64B) that the config table cannot reach, so the configuration is unchanged. That drops txwq->bf_copy_mode, leaves a single dispatch in ring_doorbell(), and removes the per-WQE branch you flagged in ib_mlx5.inl.
But the code change is significant and perf is unchanged, so I'd prefer to keep the current split.
What?
Add an aarch64
st64bBlueFlame copy mode for mlx5, with runtime selection and generic-copy fallback.Why?
st64bcan issue a single 64-byte store for BlueFlame doorbell writes, which may improve MMIO write-combining behavior on LS64-capable ARM CPUs.How?
Detect LS64 support with
getauxval(AT_HWCAP3) & HWCAP3_LS64, add an mlx5 BF copy function usingst64b, select it through the BF copy-mode config, and add a gtest variant for the ST64B MMIO path.Testing