Merge pull request #688 from jsquyres/pr/usnic-libfabric-msg-prefix-fix

usnic fixes for differences between libfabric v1.0.0 and v1.1.0

Author: jsquyres

opal/mca/btl/usnic/btl_usnic.h

@@ -93,7 +93,7 @@ extern opal_rng_buff_t opal_btl_usnic_rand_buff;
 
 /* Set to >0 to randomly drop received frags.  The higher the number,
    the more frequent the drops. */
-#define WANT_RECV_FRAG_DROPS 0
+#define WANT_RECV_DROPS 0
 /* Set to >0 to randomly fail to send an ACK, mimicing a lost ACK.
    The higher the number, the more frequent the failed-to-send-ACK. */
 #define WANT_FAIL_TO_SEND_ACK 0
@@ -102,10 +102,10 @@ extern opal_rng_buff_t opal_btl_usnic_rand_buff;
    the failed-to-resend-frag. */
 #define WANT_FAIL_TO_RESEND_FRAG 0
 
-#if WANT_RECV_FRAG_DROPS > 0
-#define FAKE_RECV_FRAG_DROP (opal_rand(&opal_btl_usnic_rand_buff) < WANT_RECV_FRAG_DROPS)
+#if WANT_RECV_DROPS > 0
+#define FAKE_RECV_DROP (opal_rand(&opal_btl_usnic_rand_buff) < WANT_RECV_DROPS)
 #else
-#define FAKE_RECV_FRAG_DROP 0
+#define FAKE_RECV_DROP 0
 #endif
 
 #if WANT_FAIL_TO_SEND_ACK > 0
@@ -213,6 +213,19 @@ typedef struct opal_btl_usnic_component_t {
     /* Prefix for the connectivity map filename (map will be output if
        the prefix is non-NULL) */
     char *connectivity_map_prefix;
+
+    /** Expected return value from fi_cq_readerr() upon success.  In
+        libfabric v1.0.0 / API v1.0, the usnic provider returned
+        sizeof(fi_cq_err_entry) upon success.  In libfabric >=v1.1 /
+        API >=v1.1, the usnic provider returned 1 upon success. */
+    ssize_t cq_readerr_success_value;
+    ssize_t cq_readerr_try_again_value;
+
+    /** Offset into the send buffer where the payload will go.  For
+        libfabric v1.0.0 / API v1.0, this is 0.  For libfabric >=v1.1
+        / API >=v1.1, this is the endpoint.msg_prefix_size (i.e.,
+        component.transport_header_len). */
+    uint32_t prefix_send_offset;
 } opal_btl_usnic_component_t;
 
 OPAL_MODULE_DECLSPEC extern opal_btl_usnic_component_t mca_btl_usnic_component;

opal/mca/btl/usnic/btl_usnic_ack.c

@@ -201,8 +201,7 @@ opal_btl_usnic_ack_send(
     /* Get an ACK frag.  If we don't get one, just discard this ACK. */
     ack = opal_btl_usnic_ack_segment_alloc(module);
     if (OPAL_UNLIKELY(NULL == ack)) {
-        opal_output(0, "====================== No frag for sending the ACK -- skipped");
-        abort();
+        return;
     }
 
     /* send the seq of the lowest item in the window that
@@ -252,6 +251,7 @@ opal_btl_usnic_ack_complete(opal_btl_usnic_module_t *module,
                                    opal_btl_usnic_ack_segment_t *ack)
 {
     opal_btl_usnic_ack_segment_return(module, ack);
+    ++module->mod_channels[ack->ss_channel].credits;
 }
 
 /*****************************************************************************/

opal/mca/btl/usnic/btl_usnic_compat.c

@@ -715,6 +715,25 @@ opal_btl_usnic_put(struct mca_btl_base_module_t *base_module,
     sfrag->sf_size = size;
     sfrag->sf_ack_bytes_left = size;
 
+
+
+    /* JMS NOTE: This is currently broken, and is deactivated by
+       removing the MCA_BTL_FLAGS_PUT from .btl_flags in btl_module.c.
+
+       Overwriting the uf_local_seg values is not a good idea, and
+       doesn't do anything to actually send the data in the
+       progression past finish_put_or_send().
+
+       The proper fix is to change the plumbing here to eventually
+       call fi_sendv() with an iov[0] = the internal buffer that's
+       already allocated, and iov[1] = the user's buffer.  The usnic
+       provider in fi_sendv() will be smart enough to figure out which
+       is more performance: memcpy'ing the 2 buffers together and
+       doing a single xfer down to the hardware, or actually doing a
+       SG list down to the hardware. */
+
+
+
     opal_btl_usnic_frag_t *frag;
     frag = &sfrag->sf_base;
     frag->uf_local_seg[0].seg_len = size;

opal/mca/btl/usnic/btl_usnic_component.c

@@ -163,6 +163,7 @@ static int usnic_component_open(void)
     mca_btl_usnic_component.usnic_all_modules = NULL;
     mca_btl_usnic_component.usnic_active_modules = NULL;
     mca_btl_usnic_component.transport_header_len = -1;
+    mca_btl_usnic_component.prefix_send_offset = 0;
 
     /* initialize objects */
     OBJ_CONSTRUCT(&mca_btl_usnic_component.usnic_procs, opal_list_t);
@@ -630,7 +631,29 @@ static mca_btl_base_module_t** usnic_component_init(int* num_btl_modules,
     hints.ep_attr = &ep_attr;
     hints.fabric_attr = &fabric_attr;
 
-    ret = fi_getinfo(FI_VERSION(1, 0), NULL, 0, 0, &hints, &info_list);
+    /* This code understands libfabric API v1.0 and v1.1.  Even if we
+       were compiled with libfabric API v1.0, we still want to request
+       v1.1 -- here's why:
+
+       - In libfabric v1.0.0 (i.e., API v1.0), the usnic provider did
+         not check the value of the "version" parameter passed into
+         fi_getinfo()
+
+       - If you pass FI_VERSION(1,0) to libfabric v1.1.0 (i.e., API
+         v1.1), the usnic provider will disable FI_MSG_PREFIX support
+         (on the assumption that the application will not handle
+         FI_MSG_PREFIX properly).  This can happen if you compile OMPI
+         against libfabric v1.0.0 (i.e., API v1.0) and run OMPI
+         against libfabric v1.1.0 (i.e., API v1.1).
+
+       So never request API v1.0 -- always request a minimum of
+       v1.1. */
+    uint32_t libfabric_api;
+    libfabric_api = FI_VERSION(FI_MAJOR_VERSION, FI_MINOR_VERSION);
+    if (libfabric_api == FI_VERSION(1, 0)) {
+        libfabric_api = FI_VERSION(1, 1);
+    }
+    ret = fi_getinfo(libfabric_api, NULL, 0, 0, &hints, &info_list);
     if (0 != ret) {
         opal_output_verbose(5, USNIC_OUT,
                             "btl:usnic: disqualifiying myself due to fi_getinfo failure: %s (%d)", strerror(-ret), ret);
@@ -664,6 +687,29 @@ static mca_btl_base_module_t** usnic_component_init(int* num_btl_modules,
     opal_output_verbose(5, USNIC_OUT,
                         "btl:usnic: usNIC fabrics found");
 
+    /* Due to ambiguities in documentation, in libfabric v1.0.0 (i.e.,
+       API v1.0) the usnic provider returned sizeof(struct
+       fi_cq_err_entry) from fi_cq_readerr() upon success.
+
+       The ambiguities were clarified in libfabric v1.1.0 (i.e., API
+       v1.1); the usnic provider returned 1 from fi_cq_readerr() upon
+       success.
+
+       So query to see what version of the libfabric API we are
+       running with, and adapt accordingly. */
+    libfabric_api = fi_version();
+    if (1 == FI_MAJOR(libfabric_api) &&
+        0 == FI_MINOR(libfabric_api)) {
+        // Old fi_cq_readerr() behavior: success=sizeof(...), try again=0
+        mca_btl_usnic_component.cq_readerr_success_value =
+            sizeof(struct fi_cq_err_entry);
+        mca_btl_usnic_component.cq_readerr_try_again_value = 0;
+    } else {
+        // New fi_cq_readerr() behavior: success=1, try again=-FI_EAGAIN
+        mca_btl_usnic_component.cq_readerr_success_value = 1;
+        mca_btl_usnic_component.cq_readerr_try_again_value = -FI_EAGAIN;
+    }
+
     /* libnl initialization */
     opal_proc_t *me = opal_proc_local_get();
     opal_process_name_t *name = &(me->proc_name);
@@ -1087,34 +1133,28 @@ static int usnic_handle_completion(
     seg = (opal_btl_usnic_segment_t*)completion->op_context;
     rseg = (opal_btl_usnic_recv_segment_t*)seg;
 
+    /* Make the completion be Valgrind-defined */
+    opal_memchecker_base_mem_defined(seg, sizeof(*seg));
+
     /* Handle work completions */
     switch(seg->us_type) {
 
     /**** Send ACK completions ****/
     case OPAL_BTL_USNIC_SEG_ACK:
         opal_btl_usnic_ack_complete(module,
                 (opal_btl_usnic_ack_segment_t *)seg);
-{ opal_btl_usnic_send_segment_t *sseg = (opal_btl_usnic_send_segment_t *)seg;
-++module->mod_channels[sseg->ss_channel].credits;
-}
         break;
 
     /**** Send of frag segment completion ****/
     case OPAL_BTL_USNIC_SEG_FRAG:
         opal_btl_usnic_frag_send_complete(module,
                 (opal_btl_usnic_frag_segment_t*)seg);
-{ opal_btl_usnic_send_segment_t *sseg = (opal_btl_usnic_send_segment_t *)seg;
-++module->mod_channels[sseg->ss_channel].credits;
-}
         break;
 
     /**** Send of chunk segment completion ****/
     case OPAL_BTL_USNIC_SEG_CHUNK:
         opal_btl_usnic_chunk_send_complete(module,
                 (opal_btl_usnic_chunk_segment_t*)seg);
-{ opal_btl_usnic_send_segment_t *sseg = (opal_btl_usnic_send_segment_t *)seg;
-++module->mod_channels[sseg->ss_channel].credits;
-}
         break;
 
     /**** Receive completions ****/
@@ -1145,17 +1185,27 @@ usnic_handle_cq_error(opal_btl_usnic_module_t* module,
     }
 
     rc = fi_cq_readerr(channel->cq, &err_entry, 0);
-    if (rc != sizeof(err_entry)) {
-        BTL_ERROR(("%s: cq_readerr ret = %d",
-               module->fabric_info->fabric_attr->name, rc));
+    if (rc == mca_btl_usnic_component.cq_readerr_try_again_value) {
+        return;
+    } else if (rc != mca_btl_usnic_component.cq_readerr_success_value) {
+        BTL_ERROR(("%s: cq_readerr ret = %d (expected %d)",
+                   module->fabric_info->fabric_attr->name, rc,
+                   (int) mca_btl_usnic_component.cq_readerr_success_value));
         channel->chan_error = true;
-    } else if (err_entry.prov_errno == 1) {
+    }
+
+    /* Silently count CRC errors.  Truncation errors are usually a
+       different symptom of a CRC error. */
+    else if (FI_ECRC == err_entry.prov_errno ||
+             FI_ETRUNC == err_entry.prov_errno) {
 #if MSGDEBUG1
         static int once = 0;
         if (once++ == 0) {
-            BTL_ERROR(("%s: Channel %d, CRC error",
-                   module->fabric_info->fabric_attr->name,
-                   channel->chan_index));
+            BTL_ERROR(("%s: Channel %d, %s",
+                       module->fabric_info->fabric_attr->name,
+                       channel->chan_index,
+                       FI_ECRC == err_entry.prov_errno ?
+                       "CRC error" : "message truncation"));
         }
 #endif
 
@@ -1171,23 +1221,10 @@ usnic_handle_cq_error(opal_btl_usnic_module_t* module,
             rseg->rs_next = channel->repost_recv_head;
             channel->repost_recv_head = rseg;
         }
-    } else if (FI_ETRUNC == err_entry.prov_errno) {
-        /* This error is usually a different symptom of a CRC error */
-#if MSGDEBUG1
-        static int once = 0;
-        if (once++ == 0) {
-            BTL_ERROR(("%s: Channel %d, message truncation",
-                   module->fabric_info->fabric_attr->name,
-                   channel->chan_index));
-        }
-#endif
-
-        /* silently count CRC errors */
-        ++module->stats.num_crc_errors;
     } else {
         BTL_ERROR(("%s: CQ[%d] prov_err = %d",
                module->fabric_info->fabric_attr->name, channel->chan_index,
-               err_entry.prov_errno));
+                   err_entry.prov_errno));
         channel->chan_error = true;
     }
 }

opal/mca/btl/usnic/btl_usnic_frag.c

@@ -30,23 +30,22 @@
 #include "btl_usnic_ack.h"
 
 static void
-common_send_seg_helper(
-    opal_btl_usnic_send_segment_t *seg,
-    int offset)
+common_send_seg_helper(opal_btl_usnic_send_segment_t *seg)
 {
     opal_btl_usnic_segment_t *bseg;
 
-    bseg = &seg->ss_base;
-
-    bseg->us_btl_header = (opal_btl_usnic_btl_header_t *)
-        (((char*) bseg->us_list.ptr) + offset);
-    bseg->us_btl_header->sender = mca_btl_usnic_component.my_hashed_rte_name;
-
+    /* send ptr for fi_send().  ss_len will be filled in right before
+       the actual send. */
+    seg->ss_ptr = (uint8_t *) seg->ss_base.us_list.ptr;
     seg->ss_send_posted = 0;
     seg->ss_ack_pending = false;
 
-    /* send ptr, len will be filled in just before send */
-    seg->ss_ptr = (uint8_t *)bseg->us_btl_header;
+    /* Offset the BTL header by (prefix_send_offset) bytes into the
+       raw buffer */
+    bseg = &seg->ss_base;
+    bseg->us_btl_header = (opal_btl_usnic_btl_header_t *)
+        (seg->ss_ptr + mca_btl_usnic_component.prefix_send_offset);
+    bseg->us_btl_header->sender = mca_btl_usnic_component.my_hashed_rte_name;
 }
 
 static void
@@ -59,7 +58,7 @@ chunk_seg_constructor(
     bseg->us_type = OPAL_BTL_USNIC_SEG_CHUNK;
 
     /* some more common initializaiton */
-    common_send_seg_helper(seg, mca_btl_usnic_component.transport_header_len);
+    common_send_seg_helper(seg);
 
     /* payload starts next byte beyond BTL chunk header */
     bseg->us_payload.raw = (uint8_t *)(bseg->us_btl_chunk_header + 1);
@@ -77,7 +76,7 @@ frag_seg_constructor(
     bseg->us_type = OPAL_BTL_USNIC_SEG_FRAG;
 
     /* some more common initializaiton */
-    common_send_seg_helper(seg, mca_btl_usnic_component.transport_header_len);
+    common_send_seg_helper(seg);
 
     /* payload starts next byte beyond BTL header */
     bseg->us_payload.raw = (uint8_t *)(bseg->us_btl_header + 1);
@@ -95,7 +94,7 @@ ack_seg_constructor(
     bseg->us_type = OPAL_BTL_USNIC_SEG_ACK;
 
     /* some more common initializaiton */
-    common_send_seg_helper(ack, mca_btl_usnic_component.transport_header_len);
+    common_send_seg_helper(ack);
 
     /* ACK value embedded in BTL header */
     bseg->us_btl_header->payload_type = OPAL_BTL_USNIC_PAYLOAD_TYPE_ACK;
@@ -176,12 +175,13 @@ send_frag_constructor(opal_btl_usnic_send_frag_t *frag)
 static void
 send_frag_destructor(opal_btl_usnic_send_frag_t *frag)
 {
-    mca_btl_base_descriptor_t *desc;
-
+#if OPAL_ENABLE_DEBUG
     /* make sure nobody twiddled these values after the constructor */
+    mca_btl_base_descriptor_t *desc;
     desc = &frag->sf_base.uf_base;
     assert(desc->USNIC_SEND_LOCAL == frag->sf_base.uf_local_seg);
     assert(0 == frag->sf_base.uf_local_seg[0].seg_len);
+#endif
 
     /* PML may change desc->des_remote to point elsewhere, cannot assert that it
      * still points to our embedded segment */