fix thread offset + stride check

This commit fixes the check which makes sure that the combination of
thread offset and stride does not cause pinning beyond the last hardware
thread.

Additionally, also improved pinning-related warnings and notes.

Change-Id: Ia33610cacb7526dca2ff69491aac2973c7333022

diff --git a/src/gmxlib/gmx_thread_affinity.c b/src/gmxlib/gmx_thread_affinity.c
index 919af6eef8643ec6ea421345e1a9d6a69b42c27f..6836a497a3c80cbb9203af0759b83c2d43a96311 100644 (file)
--- a/src/gmxlib/gmx_thread_affinity.c
+++ b/src/gmxlib/gmx_thread_affinity.c
@@ -68,6 +68,7 @@ get_thread_affinity_layout(FILE *fplog,
     const int * pkg_id;
     const int * core_id;
     const int * hwthread_id;
+    gmx_bool    bPickPinStride;
 
     if (pin_offset < 0)
     {
@@ -84,6 +85,7 @@ get_thread_affinity_layout(FILE *fplog,
 
     if (rc != 0)
     {
+        /* topology information not available or invalid, ignore it */
         nhwthreads      = hwinfo->nthreads_hw_avail;
         *locality_order = NULL;
 
@@ -91,23 +93,36 @@ get_thread_affinity_layout(FILE *fplog,
         {
             /* We don't know anything about the hardware, don't pin */
             md_print_warn(cr, fplog,
-                          "We don't know how many logical cores we have, will not pin threads");
+                          "NOTE: We don't know how many logical cores we have, will not pin threads");
 
             return -1;
         }
     }
 
+    if (nthreads > nhwthreads)
+    {
+        /* We are oversubscribing, don't pin */
+        md_print_warn(NULL, fplog,
+                      "WARNING: Oversubscribing the CPU, will not pin threads");
+
+        return -1;
+    }
+
     if (pin_offset + nthreads > nhwthreads)
     {
         /* We are oversubscribing, don't pin */
         md_print_warn(NULL, fplog,
-                      "More threads requested than available logical cores, will not pin threads");
+                      "WARNING: The requested pin offset is too large for the available logical cores,\n"
+                      "         will not pin threads");
 
         return -1;
     }
 
-    /* Check if we need to choose the pinning stride */
-    if (*pin_stride == 0)
+
+    /* do we need to choose the pinning stride? */
+    bPickPinStride = (*pin_stride == 0);
+
+    if (bPickPinStride)
     {
         if (rc == 0 && pin_offset + nthreads*nhwthreads_per_core <= nhwthreads)
         {
@@ -127,25 +142,29 @@ get_thread_affinity_layout(FILE *fplog,
              */
             *pin_stride = (nhwthreads - pin_offset)/nthreads;
         }
-
-        if (fplog != NULL)
-        {
-            fprintf(fplog, "Pinning threads with a logical core stride of %d\n",
-                    *pin_stride);
-        }
     }
     else
     {
-        if (pin_offset + nthreads*(*pin_stride) > nhwthreads)
+        /* Check the placement of the thread with the largest index to make sure
+         * that the offset & stride doesn't cause pinning beyond the last hardware thread. */
+        if (pin_offset + (nthreads-1)*(*pin_stride) >= nhwthreads)
         {
             /* We are oversubscribing, don't pin */
             md_print_warn(NULL, fplog,
-                          "The requested pinning stride is too large for the available logical cores, will not pin threads");
+                          "WARNING: The requested pinning stride is too large for the available logical cores,\n"
+                          "         will not pin threads");
 
             return -1;
         }
     }
 
+    if (fplog != NULL)
+    {
+        fprintf(fplog, "Pinning threads with a%s logical core stride of %d\n",
+                bPickPinStride ? "n auto-selected" : " user-specified",
+                *pin_stride);
+    }
+
     return 0;
 }
 
@@ -250,6 +269,7 @@ gmx_set_thread_affinity(FILE                *fplog,
                                     nthread_node,
                                     offset, &hw_opt->core_pinning_stride,
                                     &locality_order);
+
     if (rc != 0)
     {
         /* Incompatible layout, don't pin, warning was already issued */
@@ -310,24 +330,29 @@ gmx_set_thread_affinity(FILE                *fplog,
 
             /* sbuf1 contains rank info, while sbuf2 OpenMP thread info */
             sbuf1[0] = sbuf2[0] = '\0';
+            /* Only add rank info if we have more than one rank. */
+            if (cr->nnodes > 1)
+            {
 #ifdef GMX_MPI
 #ifdef GMX_THREAD_MPI
-            sprintf(sbuf1, "In thread-MPI thread #%d: ", cr->nodeid);
+                sprintf(sbuf1, "In tMPI thread #%d: ", cr->nodeid);
 #else       /* GMX_LIB_MPI */
-            sprintf(sbuf1, "In MPI process #%d: ", cr->nodeid);
+                sprintf(sbuf1, "In MPI process #%d: ", cr->nodeid);
 #endif
 #endif      /* GMX_MPI */
+            }
 
             if (nthread_local > 1)
             {
-                sprintf(sbuf2, "of %d/%d thread%s ",
+                sprintf(sbuf2, "for %d/%d thread%s ",
                         nthread_local - nth_affinity_set, nthread_local,
-                        (nthread_local - nth_affinity_set) > 1 ? "s" : "");
+                        nthread_local > 1 ? "s" : "");
             }
 
             md_print_warn(NULL, fplog,
-                          "NOTE: %sAffinity setting %sfailed.\n"
-                          "      This can cause performance degradation!",
+                          "WARNING: %sAffinity setting %sfailed.\n"
+                          "         This can cause performance degradation! If you think your setting are\n"
+                          "         correct, contact the GROMACS developers.",
                           sbuf1, sbuf2);
         }
     }