btl/uct: fix deadlock in connection code

This commit fixes a deadlock that can occur when using a TL that
supports the connect to endpoint model. The deadlock was occurring
while processing an incoming connection requests. This was done from
an active-message callback. For some unknown reason (at this time)
this callback was sometimes hanging. To avoid the issue the connection
active-message is saved for later processing.

At the same time I cleaned up the connection code to eliminate
duplicate messages when possible.

This commit also fixes some bugs in the active-message send path:

 - Correctly set all fragment fields in prepare_src.

 - Fix bug when using buffered-send. We were not reading the return
   code correctly (which is in bytes). This resulted in a message
   getting sent multiple times.

 - Don't try to progress sends from the btl_send function when in an
   active-message callback. It could lead to deep recursion and an
   eventual crash if we get a trace like
   send->progress->am_complete->ob1_callback->send->am_complete...

Closes #5820
Closes #5821

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>

Author: hjelmn

opal/mca/btl/uct/btl_uct.h

@@ -68,7 +68,7 @@ struct mca_btl_uct_module_t {
     opal_hash_table_t id_to_endpoint;
 
     /** mutex to protect the module */
-    opal_mutex_t lock;
+    opal_recursive_mutex_t lock;
 
     /** async context */
     ucs_async_context_t *ucs_async;
@@ -108,6 +108,9 @@ struct mca_btl_uct_module_t {
 
     /** frags that were waiting on connections that are now ready to send */
     opal_list_t pending_frags;
+
+    /** pending connection requests */
+    opal_fifo_t pending_connection_reqs;
 };
 typedef struct mca_btl_uct_module_t mca_btl_uct_module_t;
 
@@ -278,6 +281,7 @@ ucs_status_t mca_btl_uct_am_handler (void *arg, void *data, size_t length, unsig
 struct mca_btl_base_endpoint_t *mca_btl_uct_get_ep (struct mca_btl_base_module_t *module, opal_proc_t *proc);
 
 int mca_btl_uct_query_tls (mca_btl_uct_module_t *module, mca_btl_uct_md_t *md, uct_tl_resource_desc_t *tl_descs, unsigned tl_count);
+int mca_btl_uct_process_connection_request (mca_btl_uct_module_t *module, mca_btl_uct_conn_req_t *req);
 
 /**
  * @brief Checks if a tl is suitable for using for RDMA

opal/mca/btl/uct/btl_uct_am.c

@@ -25,7 +25,7 @@ mca_btl_base_descriptor_t *mca_btl_uct_alloc (mca_btl_base_module_t *btl, mca_bt
     mca_btl_uct_module_t *uct_btl = (mca_btl_uct_module_t *) btl;
     mca_btl_uct_base_frag_t *frag = NULL;
 
-    if ((size + 8) <= (size_t) MCA_BTL_UCT_TL_ATTR(uct_btl->am_tl, 0).cap.am.max_short) {
+    if (size <= (size_t) MCA_BTL_UCT_TL_ATTR(uct_btl->am_tl, 0).cap.am.max_short) {
         frag = mca_btl_uct_frag_alloc_short (uct_btl, endpoint);
     } else if (size <= uct_btl->super.btl_eager_limit) {
         frag = mca_btl_uct_frag_alloc_eager (uct_btl, endpoint);
@@ -40,6 +40,10 @@ mca_btl_base_descriptor_t *mca_btl_uct_alloc (mca_btl_base_module_t *btl, mca_bt
         frag->base.des_flags   = flags;
         frag->base.order       = order;
         frag->uct_iov.length = size;
+        if (NULL != frag->base.super.registration) {
+            /* zero-copy fragments will need callbacks */
+            frag->base.des_flags |= MCA_BTL_DES_SEND_ALWAYS_CALLBACK;
+        }
     }
 
     return (mca_btl_base_descriptor_t *) frag;
@@ -95,14 +99,18 @@ struct mca_btl_base_descriptor_t *mca_btl_uct_prepare_src (mca_btl_base_module_t
             return NULL;
         }
 
+        frag->uct_iov.length   = total_size;
         frag->base.order       = order;
         frag->base.des_flags   = flags;
         if (total_size > (size_t) MCA_BTL_UCT_TL_ATTR(uct_btl->am_tl, 0).cap.am.max_short) {
+            frag->segments[0].seg_len = reserve;
             frag->segments[1].seg_len = *size;
             frag->segments[1].seg_addr.pval = data_ptr;
             frag->base.des_segment_count = 2;
         } else {
+            frag->segments[0].seg_len = total_size;
             memcpy ((void *)((intptr_t) frag->segments[1].seg_addr.pval + reserve), data_ptr, *size);
+            frag->base.des_segment_count = 1;
         }
     }
 
@@ -130,7 +138,7 @@ static size_t mca_btl_uct_send_frag_pack (void *data, void *arg)
     data = (void *)((intptr_t) data + 8);
 
     /* this function should only ever get called with fragments with two segments */
-    for (size_t i = 0 ; i < 2 ; ++i) {
+    for (size_t i = 0 ; i < frag->base.des_segment_count ; ++i) {
         const size_t seg_len = frag->segments[i].seg_len;
         memcpy (data, frag->segments[i].seg_addr.pval, seg_len);
         data = (void *)((intptr_t) data + seg_len);
@@ -140,57 +148,84 @@ static size_t mca_btl_uct_send_frag_pack (void *data, void *arg)
     return length;
 }
 
-int mca_btl_uct_send_frag (mca_btl_uct_module_t *uct_btl, mca_btl_base_endpoint_t *endpoint, mca_btl_uct_base_frag_t *frag,
-                           int32_t flags, mca_btl_uct_device_context_t *context, uct_ep_h ep_handle)
+static void mca_btl_uct_append_pending_frag (mca_btl_uct_module_t *uct_btl, mca_btl_uct_base_frag_t *frag,
+                                             mca_btl_uct_device_context_t *context, bool ready)
 {
+    frag->ready = ready;
+    frag->base.des_flags |= MCA_BTL_DES_SEND_ALWAYS_CALLBACK;
+    opal_atomic_wmb ();
+
+    opal_list_append (&uct_btl->pending_frags, (opal_list_item_t *) frag);
+}
+
+int mca_btl_uct_send_frag (mca_btl_uct_module_t *uct_btl, mca_btl_uct_base_frag_t *frag, bool append)
+{
+    mca_btl_uct_device_context_t *context = frag->context;
+    const ssize_t msg_size = frag->uct_iov.length + 8;
+    ssize_t size;
     ucs_status_t ucs_status;
+    uct_ep_h ep_handle = NULL;
 
-    mca_btl_uct_context_lock (context);
+    /* if we get here then we must have an endpoint handle for this context/endpoint pair */
+    (void) mca_btl_uct_endpoint_test_am (uct_btl, frag->endpoint, frag->context, &ep_handle);
+    assert (NULL != ep_handle);
 
-    do {
+    /* if another thread set this we really don't care too much as this flag is only meant
+     * to protect against deep recursion */
+    if (!context->in_am_callback) {
+        mca_btl_uct_context_lock (context);
+        /* attempt to post the fragment */
         if (NULL != frag->base.super.registration) {
             frag->comp.dev_context = context;
-
             ucs_status = uct_ep_am_zcopy (ep_handle, MCA_BTL_UCT_FRAG, &frag->header, sizeof (frag->header),
                                           &frag->uct_iov, 1, 0, &frag->comp.uct_comp);
+
+            if (OPAL_LIKELY(UCS_INPROGRESS == ucs_status)) {
+                uct_worker_progress (context->uct_worker);
+                mca_btl_uct_context_unlock (context);
+                return OPAL_SUCCESS;
+            }
         } else {
             /* short message */
-            /* restore original flags */
-            frag->base.des_flags = flags;
-
-            if (1 == frag->base.des_segment_count) {
+            if (1 == frag->base.des_segment_count && (frag->uct_iov.length + 8) < MCA_BTL_UCT_TL_ATTR(uct_btl->am_tl, 0).cap.am.max_short) {
                 ucs_status = uct_ep_am_short (ep_handle, MCA_BTL_UCT_FRAG, frag->header.value, frag->uct_iov.buffer,
                                               frag->uct_iov.length);
-            } else {
-                ucs_status = uct_ep_am_bcopy (ep_handle, MCA_BTL_UCT_FRAG, mca_btl_uct_send_frag_pack, frag, 0);
+
+                if (OPAL_LIKELY(UCS_OK == ucs_status)) {
+                    uct_worker_progress (context->uct_worker);
+                    mca_btl_uct_context_unlock (context);
+                    /* send is complete */
+                    mca_btl_uct_frag_complete (frag, OPAL_SUCCESS);
+                    return 1;
+                }
             }
-        }
 
-        if (UCS_ERR_NO_RESOURCE != ucs_status) {
-            /* go ahead and progress the worker while we have the lock */
-            (void) uct_worker_progress (context->uct_worker);
-            break;
+            size = uct_ep_am_bcopy (ep_handle, MCA_BTL_UCT_FRAG, mca_btl_uct_send_frag_pack, frag, 0);
+            if (OPAL_LIKELY(size == msg_size)) {
+                uct_worker_progress (context->uct_worker);
+                mca_btl_uct_context_unlock (context);
+                /* send is complete */
+                mca_btl_uct_frag_complete (frag, OPAL_SUCCESS);
+                return 1;
+            }
         }
 
-        /* wait for something to complete before trying again */
-        while (!uct_worker_progress (context->uct_worker));
-    } while (1);
-
-    mca_btl_uct_context_unlock (context);
+        /* wait for something to happen */
+        uct_worker_progress (context->uct_worker);
+        mca_btl_uct_context_unlock (context);
 
-    if (UCS_OK == ucs_status) {
-        /* restore original flags */
-        frag->base.des_flags = flags;
-        /* send is complete */
-        mca_btl_uct_frag_complete (frag, OPAL_SUCCESS);
-        return 1;
+        mca_btl_uct_device_handle_completions (context);
     }
 
-    if (OPAL_UNLIKELY(UCS_INPROGRESS != ucs_status)) {
+    if (!append) {
         return OPAL_ERR_OUT_OF_RESOURCE;
     }
 
-    return 0;
+    OPAL_THREAD_LOCK(&uct_btl->lock);
+    mca_btl_uct_append_pending_frag (uct_btl, frag, context, true);
+    OPAL_THREAD_UNLOCK(&uct_btl->lock);
+
+    return OPAL_SUCCESS;
 }
 
 int mca_btl_uct_send (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endpoint, mca_btl_base_descriptor_t *descriptor,
@@ -199,7 +234,6 @@ int mca_btl_uct_send (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endpo
     mca_btl_uct_module_t *uct_btl = (mca_btl_uct_module_t *) btl;
     mca_btl_uct_device_context_t *context = mca_btl_uct_module_get_am_context (uct_btl);
     mca_btl_uct_base_frag_t *frag = (mca_btl_uct_base_frag_t *) descriptor;
-    int flags = frag->base.des_flags;
     uct_ep_h ep_handle;
     int rc;
 
@@ -208,28 +242,21 @@ int mca_btl_uct_send (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endpo
 
 
     frag->header.data.tag = tag;
-
-    /* add the callback flag before posting to avoid potential races with other threads */
-    frag->base.des_flags |= MCA_BTL_DES_SEND_ALWAYS_CALLBACK;
+    frag->context = context;
 
     rc = mca_btl_uct_endpoint_check_am (uct_btl, endpoint, context, &ep_handle);
     if (OPAL_UNLIKELY(OPAL_SUCCESS != rc)) {
-        OPAL_THREAD_LOCK(&endpoint->ep_lock);
+        OPAL_THREAD_LOCK(&uct_btl->lock);
         /* check one more time in case another thread is completing the connection now */
         if (OPAL_SUCCESS != mca_btl_uct_endpoint_test_am (uct_btl, endpoint, context, &ep_handle)) {
-            frag->context_id = context->context_id;
-            frag->ready = false;
-            OPAL_THREAD_LOCK(&uct_btl->lock);
-            opal_list_append (&uct_btl->pending_frags, (opal_list_item_t *) frag);
-            OPAL_THREAD_UNLOCK(&endpoint->ep_lock);
+            mca_btl_uct_append_pending_frag (uct_btl, frag, context, false);
             OPAL_THREAD_UNLOCK(&uct_btl->lock);
-
             return OPAL_SUCCESS;
         }
-        OPAL_THREAD_UNLOCK(&endpoint->ep_lock);
+        OPAL_THREAD_UNLOCK(&uct_btl->lock);
     }
 
-    return mca_btl_uct_send_frag (uct_btl, endpoint, frag, flags, context, ep_handle);
+    return mca_btl_uct_send_frag (uct_btl, frag, true);
 }
 
 struct mca_btl_uct_sendi_pack_args_t {
@@ -255,9 +282,7 @@ static size_t mca_btl_uct_sendi_pack (void *data, void *arg)
 
 static inline size_t mca_btl_uct_max_sendi (mca_btl_uct_module_t *uct_btl, int context_id)
 {
-    const mca_btl_uct_tl_t *tl = uct_btl->am_tl;
-    return (MCA_BTL_UCT_TL_ATTR(tl, context_id).cap.am.max_short > MCA_BTL_UCT_TL_ATTR(tl, context_id).cap.am.max_bcopy) ?
-        MCA_BTL_UCT_TL_ATTR(tl, context_id).cap.am.max_short : MCA_BTL_UCT_TL_ATTR(tl, context_id).cap.am.max_bcopy;
+    return MCA_BTL_UCT_TL_ATTR(uct_btl->am_tl, context_id).cap.am.max_bcopy;
 }
 
 int mca_btl_uct_sendi (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endpoint, opal_convertor_t *convertor,
@@ -270,7 +295,7 @@ int mca_btl_uct_sendi (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endp
     /* message with header */
     const size_t msg_size = total_size + 8;
     mca_btl_uct_am_header_t am_header;
-    ucs_status_t ucs_status = UCS_OK;
+    ucs_status_t ucs_status = UCS_ERR_NO_RESOURCE;
     uct_ep_h ep_handle;
     int rc;
 

opal/mca/btl/uct/btl_uct_am.h

@@ -27,8 +27,7 @@ int mca_btl_uct_sendi (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endp
 int mca_btl_uct_send (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endpoint, mca_btl_base_descriptor_t *descriptor,
                       mca_btl_base_tag_t tag);
 
-int mca_btl_uct_send_frag (mca_btl_uct_module_t *uct_btl, mca_btl_base_endpoint_t *endpoint, mca_btl_uct_base_frag_t *frag,
-                           int32_t flags, mca_btl_uct_device_context_t *context, uct_ep_h ep_handle);
+int mca_btl_uct_send_frag (mca_btl_uct_module_t *uct_btl, mca_btl_uct_base_frag_t *frag, bool append);
 
 mca_btl_base_descriptor_t *mca_btl_uct_alloc (mca_btl_base_module_t *btl, mca_btl_base_endpoint_t *endpoint,
                                               uint8_t order, size_t size, uint32_t flags);

opal/mca/btl/uct/btl_uct_component.c

@@ -272,7 +272,8 @@ static mca_btl_uct_module_t *mca_btl_uct_alloc_module (const char *md_name, mca_
     OBJ_CONSTRUCT(&module->eager_frags, opal_free_list_t);
     OBJ_CONSTRUCT(&module->max_frags, opal_free_list_t);
     OBJ_CONSTRUCT(&module->pending_frags, opal_list_t);
-    OBJ_CONSTRUCT(&module->lock, opal_mutex_t);
+    OBJ_CONSTRUCT(&module->lock, opal_recursive_mutex_t);
+    OBJ_CONSTRUCT(&module->pending_connection_reqs, opal_fifo_t);
 
     module->md = md;
     module->md_name = strdup (md_name);
@@ -298,10 +299,13 @@ ucs_status_t mca_btl_uct_am_handler (void *arg, void *data, size_t length, unsig
                                   .seg_len = length - sizeof (*header)};
     mca_btl_uct_base_frag_t frag = {.base = {.des_segments = &seg, .des_segment_count = 1}};
 
+    /* prevent recursion */
+    tl_context->in_am_callback = true;
+
     reg = mca_btl_base_active_message_trigger + header->data.tag;
-    mca_btl_uct_context_unlock (tl_context);
     reg->cbfunc (&uct_btl->super, header->data.tag, &frag.base, reg->cbdata);
-    mca_btl_uct_context_lock (tl_context);
+
+    tl_context->in_am_callback = false;
 
     return UCS_OK;
 }
@@ -491,8 +495,7 @@ static int mca_btl_uct_component_progress_pending (mca_btl_uct_module_t *uct_btl
 
         opal_list_remove_item (&uct_btl->pending_frags, (opal_list_item_t *) frag);
 
-        if (OPAL_SUCCESS > mca_btl_uct_send (&uct_btl->super, frag->endpoint, &frag->base,
-                                              frag->header.data.tag)) {
+        if (OPAL_SUCCESS > mca_btl_uct_send_frag (uct_btl, frag, false)) {
             opal_list_prepend (&uct_btl->pending_frags, (opal_list_item_t *) frag);
         }
     }
@@ -523,9 +526,16 @@ static int mca_btl_uct_component_progress (void)
         }
 
         if (module->conn_tl) {
+            mca_btl_uct_pending_connection_request_t *request;
+
             if (module->conn_tl != module->am_tl && module->conn_tl != module->rdma_tl) {
                 ret += mca_btl_uct_tl_progress (module->conn_tl, 0);
             }
+
+            while (NULL != (request = (mca_btl_uct_pending_connection_request_t *) opal_fifo_pop_atomic (&module->pending_connection_reqs))) {
+                mca_btl_uct_process_connection_request (module, (mca_btl_uct_conn_req_t *) request->request_data);
+                OBJ_RELEASE(request);
+            }
         }
 
         if (0 != opal_list_get_size (&module->pending_frags)) {

opal/mca/btl/uct/btl_uct_device_context.h

@@ -89,15 +89,12 @@ mca_btl_uct_module_get_tl_context_specific (mca_btl_uct_module_t *module, mca_bt
     mca_btl_uct_device_context_t *context = tl->uct_dev_contexts[context_id];
 
     if (OPAL_UNLIKELY(NULL == context)) {
-        mca_btl_uct_device_context_t *new_context;
-
-        new_context = mca_btl_uct_context_create (module, tl, context_id, true);
-        if (!opal_atomic_compare_exchange_strong_ptr ((opal_atomic_intptr_t *) &tl->uct_dev_contexts[context_id],
-                                                      (intptr_t *) &context, (intptr_t) new_context)) {
-            mca_btl_uct_context_destroy (new_context);
-        } else {
-            context = new_context;
+        OPAL_THREAD_LOCK(&module->lock);
+        context = tl->uct_dev_contexts[context_id];
+        if (OPAL_UNLIKELY(NULL == context)) {
+            context = tl->uct_dev_contexts[context_id] = mca_btl_uct_context_create (module, tl, context_id, true);
         }
+        OPAL_THREAD_UNLOCK(&module->lock);
     }
 
     return context;