MPI_Wtime initialization can be faulty and lead to 0.0 being returned during one second

[adegomme]

Background information

What version of Open MPI are you using? (e.g., v4.1.6, v5.0.1, git branch name and hash, etc.)

latest

Describe how Open MPI was installed (e.g., from a source/distribution tarball, from a git clone, from an operating system distribution package, etc.)

compiled directly from latest sources

---

Details of the problem

In 9bced03 a change to the handling of MPI_Wtime was performed, to set the origin of time at the first call, return a time of 0.0 for this one and then return values relative it later.

This improves precision, but there is a small catch/bug we found when running some simulations recently.

The issue is that the test to check if the value has already been initialized is performed by simply checking that the amount of seconds in the stored value is 0, which is the default value
See line :

ompi/ompi/mpi/c/wtime.c

Line 65 in e5cc709

if (OPAL_UNLIKELY(0 == ompi_wtime_time_origin.tv_sec)) {

In most of the cases the first call to clock_gettime or others will return time since boot or epoch, and this value will be more than 0 second, which works. But in our particular case boot is "superfast", and we enter MPI before the 1s mark on our platform timers. This means that all calls to MPI_Wtime before the 1s mark will consider the timers as uninitialized, reinitialize it and return 0.0 in the process. Which lead to some odd timing and bandwidth results.

Fix options are either to mark things explicitely as uninitialized/initialized properly through a dedicated boolean (tested, works), or to set a really impossible uninitialized value (-1 or else), or to check nanoseconds as well.

Even if this only triggers on a particular emulated platform, another aspect is that this seems to be non-reentrant, and even if I don't see a case where we would suffer from having several threads setting almost the same value as the default one, it might be a good idea to make it reentrant in the process.

[ggouaillardet]

Thanks for the report.

Since invoking MPI_Wtime() before MPI_Init() is an undefined behavior, the initialization could occur here too.
@bosilca @jsquyres any thoughts?

[jsquyres]

@ggouaillardet I don't think the OP is asking about calling MPI_Wtime() before MPI_Init() -- I think they're saying that opal_clock_gettime() somehow legitimately returns 0 for tv_sec, and OMPI therefore thinks that the time is uninitialized.

I think what @adegomme is proposing is something as simple as this:

diff --git a/ompi/mpi/c/wtime.c b/ompi/mpi/c/wtime.c
index b7918ad5d0..01498d9f0c 100644
--- a/ompi/mpi/c/wtime.c
+++ b/ompi/mpi/c/wtime.c
@@ -47,7 +47,7 @@
  * and accuracy of the user visible timer.
  * More info: https://github.com/mpi-forum/mpi-issues/issues/77#issuecomment-369663119
  */
-struct timespec ompi_wtime_time_origin = {.tv_sec = 0};
+struct timespec ompi_wtime_time_origin = {.tv_sec = -1};
 #else  /* OMPI_BUILD_MPI_PROFILING */
 extern struct timespec ompi_wtime_time_origin;
 #endif
@@ -62,7 +62,7 @@ double MPI_Wtime(void)
     // https://github.com/open-mpi/ompi/issues/3003 for more details.
     struct timespec tp;
     (void) opal_clock_gettime(&tp);
-    if (OPAL_UNLIKELY(0 == ompi_wtime_time_origin.tv_sec)) {
+    if (OPAL_UNLIKELY(-1 == ompi_wtime_time_origin.tv_sec)) {
         ompi_wtime_time_origin = tp;
     }
     wtime  = (double)(tp.tv_nsec - ompi_wtime_time_origin.tv_nsec)/1.0e+9;

AFAICT, .tv_sec is signed, so assigning -1 should be ok.

@ggouaillardet @bosilca Does this look ok to you?

[ggouaillardet]

I got that Jeff.
Your fix works for me.
My point was that if we do the initialisation at MPI_Init() we could remove the if() branch in MPI_Wtime().
That could also be considered as good enough for MPI_WTIME_IS_GLOBAL.

[bosilca]

I like to have the timer relative the MPI_Init. Please check the above PR as a potential solution to this issue.

[adegomme]

If I read this correctly, every MPI_Wtime call before MPI_Init would now return the raw opal_clock_gettime value. I know standard says it's undefined to call this before MPI_Init, but the change might have side effects with some non compliant codes.

t1=MPI_Wtime
MPI_Init
t2=MPI_Wtime
t=t2-t1
would suddenly return a big negative value, for example (I understand correct way would just be using t2 value, but it's not really explicit).

[bosilca]

Sure, users can do non-standard calls but it does not mean they have to work properly. However, the code

MPI_Init
t1=MPI_Wtime

will now return in t1 the duration of the MPI_Init call. Potentially useful but not required by the standard.

[jsquyres]

@adegomme Does your code call MPI_Wtime before MPI_Init?

[adegomme]

no, I'm just thinking of potential annoying side effects of the fix on other codes. On my side both proposals would work.

[jsquyres]

no, I'm just thinking of potential annoying side effects of the fix on other codes. On my side both proposals would work.

Fair enough; thanks for asking the question.

We generally don't go out of our way to accommodate apps that don't adhere to the MPI spec. So I think we'll be ok making this change (i.e., that you have to initialize MPI before calling MPI_Wtime).