Page MenuHomePhabricator

changing OMP rtl to use shared memory instead of env variable
ClosedPublic

Authored by terdner on Oct 21 2020, 10:49 AM.

Details

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).

Diff Detail

Event Timeline

terdner created this revision.Oct 21 2020, 10:49 AM
terdner requested review of this revision.Oct 21 2020, 10:49 AM

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

@AndreyChurbanov I expect you to comment & accept ;)

This revision is now accepted and ready to land.Oct 23 2020, 9:58 AM
terdner updated this revision to Diff 300675.Oct 26 2020, 8:01 AM

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

This revision was landed with ongoing or failed builds.Oct 26 2020, 9:02 AM
This revision was automatically updated to reflect the committed changes.

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,

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

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,

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?

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,

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?

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.

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?

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,

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

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.

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.

  1. 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!

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