UCT/MD: Address review comments

src/uct/ib/mlx5/dv/ib_mlx5dv_md.c

@@ -117,9 +117,9 @@ static void uct_ib_mlx5_devx_ksm_log(uct_ib_mlx5_md_t *md, int atomic,
                                      uint64_t iova, ucs_status_t status, int mt)
 {
     ucs_debug("KSM %s-thread memory registration status \"%s\" "
-              "range %p..%p iova %p%s on %s",
+              "range %p..%p iova 0x%" PRIx64 "%s on %s",
               mt ? "multi" : "single", ucs_status_string(status), address,
-              UCS_PTR_BYTE_OFFSET(address, length), (void*)iova,
+              UCS_PTR_BYTE_OFFSET(address, length), iova,
               atomic ? " atomic" : "", uct_ib_device_name(&md->super.dev));
 }
 
@@ -172,9 +172,10 @@ uct_ib_mlx5_devx_reg_ksm_data_mt(uct_ib_mlx5_md_t *md, int atomic,
                                       list_size, ksm_data->mrs[0]->length, in,
                                       mkey_index, reason, mr_p, mkey);
     ucs_free(in);
-out:
+
     uct_ib_mlx5_devx_ksm_log(md, atomic, address, ksm_data->length, iova,
                              status, 1);
+out:
     return status;
 }
 
@@ -418,9 +419,6 @@ uct_ib_mlx5_devx_reg_ksm_data(uct_ib_mlx5_md_t *md,
     ucs_status_t status;
 
     if (memh->super.flags & UCT_IB_MEM_MULTITHREADED) {
-        ucs_assertv_always(mr->ksm_data->length > iova_offset,
-                           "mr->ksm_data->length=%zu iova_offset=%u",
-                           mr->ksm_data->length, iova_offset);
         status = uct_ib_mlx5_devx_reg_ksm_data_mt(md, atomic, address,
                                                   mr->ksm_data, iova_offset,
                                                   mkey_index, reason, mr_p,

test/gtest/uct/ib/test_ib_md.cc

@@ -235,8 +235,8 @@ void test_ib_md::test_mkey_pack_mt_internal(unsigned access_mask,
     int ret;
     uct_mem_h memh;
 
-    if (!is_supported_pack_mem_flags(access_mask)) {
-        UCS_TEST_SKIP_R("mkey pack isn't supported");
+    if (!check_invalidate_support(access_mask)) {
+        UCS_TEST_SKIP_R("mkey invalidation isn't supported");
     }
 
     if (!has_ksm()) {
@@ -283,14 +283,18 @@ void test_ib_md::test_mkey_pack_mt_internal(unsigned access_mask,
 void test_ib_md::test_mkey_pack_mt(bool invalidate)
 {
     test_mkey_pack_mt_internal(UCT_MD_MEM_ACCESS_REMOTE_ATOMIC, invalidate);
-    test_mkey_pack_mt_internal(md_flags_remote_rma, invalidate);
-    test_mkey_pack_mt_internal(UCT_MD_MEM_ACCESS_ALL, invalidate);
     test_mkey_pack_mt_internal(UCT_MD_MEM_ACCESS_RMA, invalidate);
+    test_mkey_pack_mt_internal(UCT_MD_MEM_ACCESS_ALL, invalidate);
 }
 
 UCS_TEST_P(test_ib_md, pack_mkey_mt, "REG_MT_THRESH=128K", "REG_MT_CHUNK=128K")
 {
     test_mkey_pack_mt(false);
+}
+
+UCS_TEST_P(test_ib_md, pack_mkey_mt_invalidate, "REG_MT_THRESH=128K",
+           "REG_MT_CHUNK=128K")
+{
     test_mkey_pack_mt(true);
 }
 

test/gtest/uct/ib/test_ib_xfer.cc

@@ -159,11 +159,12 @@ class uct_p2p_mix_test_mt : public uct_p2p_mix_test {
     bool check_md_flags()
     {
 #if HAVE_DEVX
-        auto *ib_mlx5_md = ucs_derived_of(sender().md(), uct_ib_mlx5_md_t);
-        auto *ib_md      = ucs_derived_of(sender().md(), uct_ib_md_t);
-        if (!(ib_md->dev.flags & UCT_IB_DEVICE_FLAG_MLX5_PRM)) {
+        auto *ib_md = ucs_derived_of(sender().md(), uct_ib_md_t);
+        if (strcmp(ib_md->name, UCT_IB_MD_NAME(mlx5))) {
             return false;
         }
+
+        auto *ib_mlx5_md = ucs_derived_of(sender().md(), uct_ib_mlx5_md_t);
         return (ib_mlx5_md->flags & UCT_IB_MLX5_MD_FLAG_KSM) &&
                (ib_mlx5_md->flags & UCT_IB_MLX5_MD_FLAG_INDIRECT_ATOMICS);
 #else
@@ -185,15 +186,14 @@ class uct_p2p_mix_test_mt : public uct_p2p_mix_test {
         }
 
         /* Large buffer can cause failures because of too many chunks */
-        m_send_size = ucs_min(m_send_size, 256 * reg_mt_chunk);
+        m_buffer_size = ucs_min(m_buffer_size, 256 * reg_mt_chunk);
         /* We need at least two chunks */
-        m_send_size = ucs_max(m_send_size, reg_mt_chunk + 1);
+        m_buffer_size = ucs_max(m_buffer_size, reg_mt_chunk + 1);
     }
 
     virtual void cleanup() override
     {
         uct_p2p_mix_test::cleanup();
-
         pop_config();
     }
 

test/gtest/uct/test_md.cc

@@ -100,7 +100,7 @@ void test_md::test_reg_mem(unsigned access_mask,
     params.flags       = UCT_MD_MEM_DEREG_FLAG_INVALIDATE;
     params.comp        = &comp().comp;
 
-    if (!is_supported_pack_mem_flags(access_mask)) {
+    if (!check_invalidate_support(access_mask)) {
         params.field_mask = UCT_MD_MEM_DEREG_FIELD_COMPLETION |
                             UCT_MD_MEM_DEREG_FIELD_FLAGS |
                             UCT_MD_MEM_DEREG_FIELD_MEMH;
@@ -171,7 +171,7 @@ test_md::test_md()
     /* coverity[uninit_member] */
 }
 
-bool test_md::is_supported_pack_mem_flags(unsigned reg_flags) const
+bool test_md::check_invalidate_support(unsigned reg_flags) const
 {
     return (reg_flags & md_flags_remote_rma) ?
            check_caps(UCT_MD_FLAG_INVALIDATE_RMA) :

test/gtest/uct/test_md.h

@@ -30,7 +30,7 @@ class test_md : public testing::TestWithParam<test_md_param>,
 
     test_md();
 
-    bool is_supported_pack_mem_flags(unsigned reg_flags) const;
+    bool check_invalidate_support(unsigned reg_flags) const;
 
     bool is_bf_arm() const;
 

test/gtest/uct/test_p2p_mix.cc

@@ -13,7 +13,7 @@ extern "C" {
 
 
 uct_p2p_mix_test::uct_p2p_mix_test():
-    uct_p2p_test(0), m_send_size(0), m_max_short(0), m_max_bcopy(0),
+    uct_p2p_test(0), m_buffer_size(0), m_max_short(0), m_max_bcopy(0),
     m_max_zcopy(0)
 {}
 
@@ -77,19 +77,20 @@ ucs_status_t uct_p2p_mix_test::put_short(const mapped_buffer &sendbuf,
 }
 
 
-size_t uct_p2p_mix_test::pack_bcopy(void *dest, void *arg) {
+size_t uct_p2p_mix_test::pack_bcopy(void *dest, void *arg)
+{
     auto pack_arg = static_cast<bcopy_pack_arg *>(arg);
 
-    mem_buffer::copy_from(dest, pack_arg->buffer, pack_arg->length,
-                          pack_arg->mem_type);
-    return pack_arg->length;
+    mem_buffer::copy_from(dest, pack_arg->sendbuf->ptr(), pack_arg->max_bcopy,
+                          pack_arg->sendbuf->mem_type());
+    return pack_arg->max_bcopy;
 }
 
 ucs_status_t uct_p2p_mix_test::put_bcopy(const mapped_buffer &sendbuf,
                                          const mapped_buffer &recvbuf,
                                          uct_completion_t *comp)
 {
-    bcopy_pack_arg pack_arg = {sendbuf.ptr(), m_max_bcopy, sendbuf.mem_type()};
+    bcopy_pack_arg pack_arg = {&sendbuf, m_max_bcopy};
     ssize_t packed_len = uct_ep_put_bcopy(sender().ep(0), pack_bcopy,
                                           (void*)&pack_arg, recvbuf.addr(),
                                           recvbuf.rkey());
@@ -195,16 +196,17 @@ void uct_p2p_mix_test::check_buffers(const mapped_buffer &sendbuf,
     /* Empty function that is overridden in descendants */
 }
 
-void uct_p2p_mix_test::run(unsigned count) {
+void uct_p2p_mix_test::run(unsigned count)
+{
     if (m_avail_send_funcs.size() == 0) {
         UCS_TEST_SKIP_R("unsupported");
     }
     if (!(sender().md_attr().access_mem_types & UCS_BIT(UCS_MEMORY_TYPE_HOST))) {
         UCS_TEST_SKIP_R("skipping on non-host memory");
     }
 
-    mapped_buffer sendbuf(m_send_size, 0, sender());
-    mapped_buffer recvbuf(m_send_size, 0, receiver());
+    mapped_buffer sendbuf(m_buffer_size, 0, sender());
+    mapped_buffer recvbuf(m_buffer_size, 0, receiver());
 
     check_buffers(sendbuf, recvbuf);
 
@@ -216,7 +218,8 @@ void uct_p2p_mix_test::run(unsigned count) {
 }
 
 template<bool use_last_byte>
-void uct_p2p_mix_test::add_atomic_ops() {
+void uct_p2p_mix_test::add_atomic_ops()
+{
     if (sender().iface_attr().cap.atomic64.fop_flags & UCS_BIT(UCT_ATOMIC_OP_ADD)) {
         m_avail_send_funcs.push_back(&uct_p2p_mix_test::atomic_fop<uint64_t, UCT_ATOMIC_OP_ADD, use_last_byte>);
     }
@@ -247,21 +250,22 @@ void uct_p2p_mix_test::add_atomic_ops() {
     if (sender().iface_attr().cap.atomic32.fop_flags & UCS_BIT(UCT_ATOMIC_OP_SWAP)) {
         m_avail_send_funcs.push_back(&uct_p2p_mix_test::atomic_fop<uint32_t, UCT_ATOMIC_OP_SWAP, use_last_byte>);
     }
-
     if (sender().iface_attr().cap.atomic64.fop_flags & UCS_BIT(UCT_ATOMIC_OP_CSWAP)) {
         m_avail_send_funcs.push_back(&uct_p2p_mix_test::cswap64<use_last_byte>);
     }
 }
 
-size_t uct_p2p_mix_test::max_buffer_size() {
-        if (has_mm() || has_transport("self")) {
-            /* Reduce testing time */
-            return UCS_MBYTE;
-        }
-        return UCS_GBYTE;
+size_t uct_p2p_mix_test::max_buffer_size() const
+{
+    if (has_mm() || has_transport("self")) {
+        /* Reduce testing time */
+        return UCS_MBYTE;
     }
+    return UCS_GBYTE;
+}
 
-void uct_p2p_mix_test::init() {
+void uct_p2p_mix_test::init()
+{
     uct_p2p_test::init();
     ucs_status_t status = uct_iface_set_am_handler(receiver().iface(), AM_ID,
                                                    am_callback, NULL,
@@ -270,10 +274,9 @@ void uct_p2p_mix_test::init() {
 
     m_max_short = m_max_bcopy = m_max_zcopy = max_buffer_size();
     if (sender().iface_attr().cap.flags & UCT_IFACE_FLAG_AM_SHORT) {
-        m_max_short = ucs_min(m_max_short, sender().iface_attr().cap.am.max_short);
-
         m_avail_send_funcs.push_back(&uct_p2p_mix_test::am_short);
         m_avail_send_funcs.push_back(&uct_p2p_mix_test::am_short_iov);
+        m_max_short = ucs_min(m_max_short, sender().iface_attr().cap.am.max_short);
     }
     if (sender().iface_attr().cap.flags & UCT_IFACE_FLAG_AM_ZCOPY) {
         m_avail_send_funcs.push_back(&uct_p2p_mix_test::am_zcopy);
@@ -288,13 +291,14 @@ void uct_p2p_mix_test::init() {
         m_max_bcopy = ucs_min(m_max_bcopy, sender().iface_attr().cap.put.max_bcopy);
     }
 
-    m_send_size = ucs_max(m_max_zcopy, ucs_max(m_max_short, m_max_bcopy));
+    m_buffer_size = std::max({m_max_short, m_max_bcopy, m_max_zcopy});
 
     add_atomic_ops<false>();
     add_atomic_ops<true>();
 }
 
-void uct_p2p_mix_test::cleanup() {
+void uct_p2p_mix_test::cleanup()
+{
     while (am_pending) {
         progress();
     }

test/gtest/uct/test_p2p_mix.h

@@ -75,19 +75,18 @@ class uct_p2p_mix_test : public uct_p2p_test {
 
     virtual void cleanup();
 
-    size_t max_buffer_size();
+    size_t max_buffer_size() const;
 
-    size_t                   m_send_size;
+    size_t                   m_buffer_size;
 private:
     std::vector<send_func_t> m_avail_send_funcs;
     size_t                   m_max_short, m_max_bcopy, m_max_zcopy;
 
     static uint32_t          am_pending;
 
     struct bcopy_pack_arg {
-        void              *buffer;
-        size_t            length;
-        ucs_memory_type_t mem_type;
+        const mapped_buffer *sendbuf;
+        size_t               max_bcopy;
     };
 };