changing OMP rtl to use shared memory instead of env variable

[terdner]

Summary

Per customer request, changing the unix environment variable in the kmp_registration_library.
This change only affects UNIX and dynamic library
It will call shared memory to keep track of runtime info instead of the unix env variable which is not thread safe.

(Windows env variables using SetEnvironmentVariable/GetEnvironmentVariable are thread safe).

[jdoerfert]

FWIW, looks reasonable to me and is what we decided to do. Thanks!

@AndreyChurbanov I expect you to comment & accept ;)

[AndreyChurbanov]

LGTM

[terdner]

ran git diff with "-U999999 " option so that all code is visible.

[rogfer01]

Hi, one question about this change:

We're seeing that sometimes the files in /dev/shm are not always removed, is this expected?

Leaving files is not a problem if the same user is running the application because the second shm_open in line 6416 of kmp_runtime.cpp above will be able to open the file. However for a user (other than the creator of the shm file) this returns EACCES and then the runtime refuses to start with a very confusing error.

This may not be easily observable in CI systems because they often use the same user (in the same nodes).

Kind regards,

[terdner]

Hi @rogfer01,

After committing this code we found a case where if there is an abort some files in /dev/shm did not get cleaned up properly.   We have created a new review that fixes this problem:

https://reviews.llvm.org/D90974

If you pick up code after that review was merged and still see the problem please let me know.    As far as I have seen, this more recent patch fixes the issue.

Thanks,
Todd

[rogfer01]

Hi Todd,

thanks for the prompt answer. We do have D90974.

I wonder if due to abnormal termination of the process some files might not have a chance to be removed? In that case I understand that the 0666 in shm_open(..., O_CREAT, ...) will still honour umask (commonly it is 0022 so the files are created with rw-r--r--). Was the intent of this change to create a rw-rw-rw- file instead? That would avoid the EACCES, in case another user hits the same file because it wasn't removed. Not sure if this is the intended behaviour here.

Kind regards,

[jdoerfert]

In D89898#2400384, @rogfer01 wrote:

I wonder if due to abnormal termination of the process some files might not have a chance to be removed? In that case I understand that the 0666 in shm_open(..., O_CREAT, ...) will still honour umask (commonly it is 0022 so the files are created with rw-r--r--). Was the intent of this change to create a rw-rw-rw- file instead? That would avoid the EACCES, in case another user hits the same file because it wasn't removed. Not sure if this is the intended behaviour here.

[Drive by] What about appending the process id or a random number to the file name, or both?

[rogfer01]

Hi @jdoerfert

[Drive by] What about appending the process id or a random number to the file name, or both?

yeah, the patch already does this. The space of PIDs it is not too large (most systems still use 16-bit PIDs) so collisions are not that unlikely. It is just a bit confusing that an OpenMP application fails due to this, sporadically, at startup.

Perhaps the scenario of leftover files is not that common: probably only if the process receives a SIGKILL. Most scheduling systems will send SIGTERM first and users often use SIGINT or SIGQUIT.

Kind regards,

[AndreyChurbanov]

In D89898#2400546, @rogfer01 wrote:

Hi @jdoerfert

[Drive by] What about appending the process id or a random number to the file name, or both?

yeah, the patch already does this. The space of PIDs it is not too large (most systems still use 16-bit PIDs) so collisions are not that unlikely. It is just a bit confusing that an OpenMP application fails due to this, sporadically, at startup.

Perhaps the scenario of leftover files is not that common: probably only if the process receives a SIGKILL. Most scheduling systems will send SIGTERM first and users often use SIGINT or SIGQUIT.

Kind regards,

Random number won't work, because the goal of the library registering is to catch several libraries in a single process. So a process should have deterministic file name (or environment variable name).
What we could implement to solve this problem is:

1. Remove shared memory file when a signal caught. This may help when application receives any signal except for SIGKILL which cannot be caught. For this one can set environment variable KMP_HANDLE_SIGNALS=1 (it is off by default).
2. Introduce option to revert library behavior to setting environment variable, say KMP_REGISTER_LIB_ENV=1. That may help applications those do not read environment variables in multiple threads, and be killed by SIGKILL signal.

Maybe there exist some other option?

[rogfer01]

Random number won't work, because the goal of the library registering is to catch several libraries in a single process. So a process should have deterministic file name (or environment variable name).

Can we add the UID of the current process to the filename, so we do not attempt to open a shared file created by another user? This fails currently. I don't expect the UID to change during the execution of most OpenMP applications.

Also note that a single abort() (or a failing assert such one that could happen when running the OpenMP testsuite) will generate a leftover file.

[terdner]

Adding the UID is pretty simple to implement. Does this solve the problem (because a user can already open a file that is owned by UID) or just make it more rare because now you are only hitting files that you left behind, and not others?

[rogfer01]

Hi Todd,

In D89898#2403115, @terdner wrote:
Adding the UID is pretty simple to implement. Does this solve the problem (because a user can already open a file that is owned by UID) or just make it more rare because now you are only hitting files that you left behind, and not others?

The latter.

I think we can split the issue in two:
a) files that are left behind when a process using OpenMP ends abnormally (think of OpenMP application developers using accidentally causing errors during their development)
b) failing at the startup of an OpenMP process because we attempt to open a file that we didn't create.

Adding the UID would make b) rarer. Solving a) seems much harder to me now, but making b) a seldom event might be enough for now.

Note that I assume that hitting a file we left behind by ourselves is not a problem for the runtime.

Kind regards,

[AndreyChurbanov]

Note that I assume that hitting a file we left behind by ourselves is not a problem for the runtime.

You will still need to explicitly ask library to work with files left behind via setting KMP_DUPLICATE_LIB_OK=1.
Otherwise library should abort, as it will think that another copy of the library already works in the same process, that is not supported.

Better to avoid files to be left whenever possible.
For applications interrupted abnormally we can add SHM file cleanup in the signal handler, then setting KMP_HANDLE_SIGNALS=1 will remove SHM file (given that at least one parallel region executed/started before interrupt).

We don't catch signals by default because users won't see expected call stack.
E.g. if they call abort() the call stack will point to the runtime internals as opposed to user's code.
If this is lesser problem than files left behind, we can think of catching signals by default... Of cause this will still not work for SIGKILL signal which cannot be caught.
If you want library to not leave files in case SIGKILL, the only solution I see is to optionally switch library registration back to setting environment variable as opposed to creating shared memory file.
Optionally because environment variable also has problems, thus cannot be a default option.

Regards,
Andrey

[terdner]

Ok, Here is what I propose we do:

1. I will change the name of the shared memory file to include UID at the end. This way it will only conflict with users of the same UID which will reduce the likelihood of this happening because you will no longer collide with other UID. this also implies that if a run changes UID the openMP runtime library will no longer recognize that a runtime is running.
2. I will put a call into unregister the library in z_Linux.utls.cpp at about line 1153 (in the case statement for SIGTERM). This should call __kmp_unregister_library whenever runtime gets SIGTERM, SIGQUIT, SIGSYS, SIGSEGV etc (see list in file). This may remove some of the leftover files when a run is terminated.

As Andrey said, user should set KMP_DUPLICATE_LIB_OK=1 if they want to allow the runtime to run when an existing file with the same name is outstanding. If that env variable is not set runtime will still exit out.

I will make these changes today in my local area and run some regressions and confirm it doesn't have any unintended consequences.

[rogfer01]

Hi Todd,

1. I will change the name of the shared memory file to include UID at the end. This way it will only conflict with users of the same UID which will reduce the likelihood of this happening because you will no longer collide with other UID. this also implies that if a run changes UID the openMP runtime library will no longer recognize that a runtime is running.

I think this trade-off is acceptable as I believe changing the uid is not a usual thing that OpenMP applications have to do.

2. I will put a call into unregister the library in z_Linux.utls.cpp at about line 1153 (in the case statement for SIGTERM). This should call __kmp_unregister_library whenever runtime gets SIGTERM, SIGQUIT, SIGSYS, SIGSEGV etc (see list in file). This may remove some of the leftover files when a run is terminated.

As Andrey said, user should set KMP_DUPLICATE_LIB_OK=1 if they want to allow the runtime to run when an existing file with the same name is outstanding. If that env variable is not set runtime will still exit out.

I think this would be only necessary if we can't tolerate false positives at all

A false positive should require (if I read the code correctly) the three following circumstances: 1) we hit the same shm file (left by an earlier process that ended abnormally) 2) the address read from the shm is mapped in the process (ASLR mechanisms which are commonly enabled might reduce the likelihood of this) 3) we believe a runtime is running (when there is none) because the memory contains the value that means "a runtime is running".

I don't have actual evidence on this, but I believe this is rather unlikely.

I will make these changes today in my local area and run some regressions and confirm it doesn't have any unintended consequences.

Thanks a lot!

[terdner]

[PATCH] D91869 is the update to include the UID in the name. This addresses Roger's request.
https://reviews.llvm.org/D91869

openmp/runtime/cmake/LibompHandleFlags.cmake

   if(${IA32})
     libomp_append(libflags_local -lirc_pic LIBOMP_HAVE_IRC_PIC_LIBRARY)
   endif()
+  if(LIBOMP_HAVE_SHM_OPEN_WITH_LRT)
+    libomp_append(libflags_local -lrt)
+  endif()
   if(${CMAKE_SYSTEM_NAME} MATCHES "DragonFly|FreeBSD")
     libomp_append(libflags_local "-Wl,--no-as-needed" LIBOMP_HAVE_AS_NEEDED_FLAG)
     libomp_append(libflags_local "-lm")

openmp/runtime/cmake/config-ix.cmake

 include(CheckIncludeFile)
 include(CheckLibraryExists)
 include(CheckIncludeFiles)
+include(CheckSymbolExists)
 include(LibompCheckLinkerFlag)
 include(LibompCheckFortranFlag)
 

   libomp_check_fortran_flag(-m32 LIBOMP_HAVE_M32_FORTRAN_FLAG)
 endif()
 
+# Check for Unix shared memory
+check_symbol_exists(shm_open "sys/mman.h" LIBOMP_HAVE_SHM_OPEN_NO_LRT)
+if (NOT LIBOMP_HAVE_SHM_OPEN_NO_LRT)
+  set(CMAKE_REQUIRED_LIBRARIES -lrt)
+  check_symbol_exists(shm_open "sys/mman.h" LIBOMP_HAVE_SHM_OPEN_WITH_LRT)
+  set(CMAKE_REQUIRED_LIBRARIES)
+endif()
+
 # Check linker flags
 if(WIN32)
   libomp_check_linker_flag(/SAFESEH LIBOMP_HAVE_SAFESEH_FLAG)

openmp/runtime/src/kmp_runtime.cpp

 
 #include "tsan_annotations.h"
 
+#if KMP_OS_WINDOWS
+// windows does not need include files as it doesn't use shared memory
+#else
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#define SHM_SIZE 1024
+#endif
+
 #if defined(KMP_GOMP_COMPAT)
 char const __kmp_version_alt_comp[] =
     KMP_VERSION_PREFIX "alternative compiler support: yes";

   }
 
   KMP_MB(); /* Flush all pending memory write invalidates.  */
-
   /* find out who we are and what we should do */
   {
     int gtid = (gtid_req >= 0) ? gtid_req : __kmp_gtid_get_specific();

       if (__kmp_debug_buf)
         __kmp_dump_debug_buffer();
 #endif
+      // added unregister library call here when we switch to shm linux
+      // if we don't, it will leave lots of files in /dev/shm
+      // cleanup shared memory file before exiting.
+      __kmp_unregister_library();
       return;
     }
   }

 
     char *value = NULL; // Actual value of the environment variable.
 
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB // shared memory is with dynamic library
+    char *shm_name = __kmp_str_format("/%s", name);
+    int shm_preexist = 0;
+    char *data1;
+    int fd1 = shm_open(shm_name, O_CREAT | O_EXCL | O_RDWR, 0666);
+    if ((fd1 == -1) && (errno == EEXIST)) {
+      // file didn't open because it already exists.
+      // try opening existing file
+      fd1 = shm_open(shm_name, O_RDWR, 0666);
+      if (fd1 == -1) { // file didn't open
+        // error out here
+        __kmp_fatal(KMP_MSG(FunctionError, "Can't open SHM"), KMP_ERR(0),
+                    __kmp_msg_null);
+      } else {
+        // able to open existing file
+        shm_preexist = 1;
+      }
+    } else if (fd1 == -1) { // SHM didn't open; it was due to error other than
+      // already exists.
+      // error out here.
+      __kmp_fatal(KMP_MSG(FunctionError, "Can't open SHM2"), KMP_ERR(errno),
+                  __kmp_msg_null);
+    }
+    if (shm_preexist == 0) {
+      // we created SHM now set size
+      if (ftruncate(fd1, SHM_SIZE) == -1) {
+        // error occured setting size;
+        __kmp_fatal(KMP_MSG(FunctionError, "Can't set size of SHM"),
+                    KMP_ERR(errno), __kmp_msg_null);
+      }
+    }
+    data1 =
+        (char *)mmap(0, SHM_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd1, 0);
+    if (data1 == MAP_FAILED) {
+      // failed to map shared memory
+      __kmp_fatal(KMP_MSG(FunctionError, "Can't map SHM"), KMP_ERR(errno),
+                  __kmp_msg_null);
+    }
+    if (shm_preexist == 0) { // set data to SHM, set value
+      KMP_STRCPY_S(data1, SHM_SIZE, __kmp_registration_str);
+    }
+    // Read value from either what we just wrote or existing file.
+    value = __kmp_str_format("%s", data1); // read value from SHM
+    munmap(data1, SHM_SIZE);
+    close(fd1);
+#else // Windows and unix with static library
     // Set environment variable, but do not overwrite if it is exist.
     __kmp_env_set(name, __kmp_registration_str, 0);
-    // Check the variable is written.
+    // read value to see if it got set
     value = __kmp_env_get(name);
-    if (value != NULL && strcmp(value, __kmp_registration_str) == 0) {
+#endif
 
+    if (value != NULL && strcmp(value, __kmp_registration_str) == 0) {
       done = 1; // Ok, environment variable set successfully, exit the loop.
-
     } else {
-
       // Oops. Write failed. Another copy of OpenMP RTL is in memory.
       // Check whether it alive or dead.
       int neighbor = 0; // 0 -- unknown status, 1 -- alive, 2 -- dead.

         done = 1; // Exit the loop.
       } break;
       case 2: { // Neighbor is dead.
+
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB // shared memory is with dynamic library
+        // close shared memory.
+        shm_unlink(shm_name); // this removes file in /dev/shm
+#else
         // Clear the variable and try to register library again.
         __kmp_env_unset(name);
+#endif
       } break;
       default: { KMP_DEBUG_ASSERT(0); } break;
       }
     }
     KMP_INTERNAL_FREE((void *)value);
-  }
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB // shared memory is with dynamic library
+    KMP_INTERNAL_FREE((void *)shm_name);
+#endif
+  } // while
   KMP_INTERNAL_FREE((void *)name);
 
 } // func __kmp_register_library_startup

 void __kmp_unregister_library(void) {
 
   char *name = __kmp_reg_status_name();
-  char *value = __kmp_env_get(name);
+  char *value = NULL;
+
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB // shared memory is with dynamic library
+  char *shm_name = __kmp_str_format("/%s", name);
+  int fd1 = shm_open(shm_name, O_RDONLY, 0666);
+  if (fd1 == -1) {
+    // file did not open. return.
+    return;
+  }
+  char *data1 = (char *)mmap(0, SHM_SIZE, PROT_READ, MAP_SHARED, fd1, 0);
+  if (data1 != MAP_FAILED) {
+    value = __kmp_str_format("%s", data1); // read value from SHM
+    munmap(data1, SHM_SIZE);
+  }
+  close(fd1);
+#else
+  value = __kmp_env_get(name);
+#endif
 
   KMP_DEBUG_ASSERT(__kmp_registration_flag != 0);
   KMP_DEBUG_ASSERT(__kmp_registration_str != NULL);
   if (value != NULL && strcmp(value, __kmp_registration_str) == 0) {
-    // Ok, this is our variable. Delete it.
+//  Ok, this is our variable. Delete it.
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB // shared memory is with dynamic library
+    shm_unlink(shm_name); // this removes file in /dev/shm
+#else
     __kmp_env_unset(name);
+#endif
   }
 
+#if KMP_OS_UNIX && KMP_DYNAMIC_LIB // shared memory is with dynamic library
+  KMP_INTERNAL_FREE(shm_name);
+#endif
+
   KMP_INTERNAL_FREE(__kmp_registration_str);
   KMP_INTERNAL_FREE(value);
   KMP_INTERNAL_FREE(name);