From 7c08c485a9b878f7a63ce916e6b5cbb4597cbff6 Mon Sep 17 00:00:00 2001
From: Szilard Pall <pszilard@cbr.su.se>
Date: Mon, 27 May 2013 19:07:30 +0200
Subject: [PATCH] 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
---
 src/gmxlib/gmx_thread_affinity.c | 61 ++++++++++++++++++++++----------
 1 file changed, 43 insertions(+), 18 deletions(-)

diff --git a/src/gmxlib/gmx_thread_affinity.c b/src/gmxlib/gmx_thread_affinity.c
index 919af6eef8..6836a497a3 100644
--- 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);
         }
     }
-- 
2.22.0