Initial Software-based Performance Counters PR #4885

davideberius · 2018-03-07T21:27:36Z

Added Software-based Performance Counters driver code along with several counters.

This code is the implementation of Software-base Performance Counters as described in the paper 'Using Software-Base Performance Counters to Expose Low-Level Open MPI Performance Information' in EuroMPI/USA '17 (http://icl.cs.utk.edu/news_pub/submissions/software-performance-counters.pdf). More practical usage information can be found here: https://github.com/davideberius/ompi/wiki/How-to-Use-Software-Based-Performance-Counters-(SPCs)-in-Open-MPI.

All software events functions are put in macros that become no-ops when SOFTWARE_EVENTS_ENABLE is not defined. The internal timer units have been changed to cycles to avoid division operations which was a large source of overhead as discussed in the paper. Added a --with-spc configure option to enable SPCs in the Open MPI build. This defines SOFTWARE_EVENTS_ENABLE. Added an MCA parameter, mpi_spc_enable, for turning on specific counters. Added an MCA parameter, mpi_spc_dump_enabled, for turning on and off dumping SPC counters in MPI_Finalize. Added an SPC test and example.

ompiteam-bot · 2018-03-07T21:27:38Z

Can one of the admins verify this patch?

thananon · 2018-03-07T21:40:16Z

Reference paper for this PR.

jsquyres · 2018-03-07T21:46:47Z

ok to test

ibm-ompi · 2018-03-07T21:58:57Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/fc3f553cdcceef1058554ddce9862137

ibm-ompi · 2018-03-07T22:11:14Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/e0e2bd8f20c25081c15e358cdd70da84

ggouaillardet · 2018-03-08T06:00:41Z

@davideberius a bit is missing in example/Makefile

here is a patch that should fix it

commit df715db8ab2ac50e2851f700c59707ea200589d7
Author: Gilles Gouaillardet <[email protected]>
Date:   Thu Mar 8 14:52:15 2018 +0900

    examples: fix Makefile for spc_example
    
    Signed-off-by: Gilles Gouaillardet <[email protected]>

diff --git a/examples/Makefile b/examples/Makefile
index f7d7687..e77e783 100644
--- a/examples/Makefile
+++ b/examples/Makefile
@@ -13,8 +13,8 @@
 # Copyright (c) 2011-2016 Cisco Systems, Inc.  All rights reserved.
 # Copyright (c) 2012      Los Alamos National Security, Inc.  All rights reserved.
 # Copyright (c) 2013      Mellanox Technologies, Inc.  All rights reserved.
-# Copyright (c) 2017      Research Organization for Information Science
-#                         and Technology (RIST). All rights reserved.
+# Copyright (c) 2017-2018 Research Organization for Information Science
+#                         and Technology (RIST).  All rights reserved.
 # $COPYRIGHT$
 #
 # Additional copyrights may follow
@@ -133,6 +133,8 @@ ring_c: ring_c.c
        $(MPICC) $(CFLAGS) $(LDFLAGS) $? $(LDLIBS) -o $@
 connectivity_c: connectivity_c.c
        $(MPICC) $(CFLAGS) $(LDFLAGS) $? $(LDLIBS) -o $@
+spc_example: spc_example.c
+       $(MPICC) $(CFLAGS) $(LDFLAGS) $? $(LDLIBS) -o $@
 
 hello_cxx: hello_cxx.cc
        $(MPICXX) $(CXXFLAGS) $(LDFLAGS) $? $(LDLIBS) -o $@

ibm-ompi · 2018-03-08T19:51:17Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/c5876e6706ec5e36ae6922d74a9b71d2

thananon · 2018-03-08T20:00:28Z

Hmm, I wonder if it's something GNU specific in the code that breaks XLC?

  CC       pml_ob1_isend.lo
  CC       pml_ob1_rdma.lo
  CC       pml_ob1_rdmafrag.lo
  CC       pml_ob1_recvfrag.lo
  CC       pml_ob1_recvreq.lo
  CC       pml_ob1_sendreq.lo
  CC       pml_ob1_start.lo
1586-494 (U) INTERNAL COMPILER ERROR: Signal 11.
Calling signal handler...
/opt/ibm/xlC/13.1.5/bin/.orig/xlc_r: error: 1501-230 Internal compiler error; please contact your Service Representative. For more information visit:
http://www.ibm.com/support/docview.wss?uid=swg21110810
make[2]: *** [pml_ob1_isend.lo] Error 1
make[2]: *** Waiting for unfinished jobs....
make[2]: Leaving directory `/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/ompi/mca/pml/ob1'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/ompi'
make: *** [all-recursive] Error 1

jjhursey · 2018-03-08T20:40:22Z

Here is the verbose make at that point, in case it helps:

shell$ make V=1
depbase=`echo pml_ob1_isend.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ../../../../libtool  --tag=CC   --mode=compile xlc_r -DHAVE_CONFIG_H -I. -I../../../../opal/include -I../../../../ompi/include -I../../../../oshmem/include -I../../../../opal/mca/hwloc/hwloc2a/hwloc/include/private/autogen -I../../../../opal/mca/hwloc/hwloc2a/hwloc/include/hwloc/autogen -I../../../../ompi/mpiext/cuda/c   -I../../../.. -I../../../../orte/include -I/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/opal/mca/event/libevent2022/libevent -I/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/opal/mca/event/libevent2022/libevent/include -I/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/opal/mca/hwloc/hwloc2a/hwloc/include    -O3 -DNDEBUG -finline-functions -fno-strict-aliasing  -MT pml_ob1_isend.lo -MD -MP -MF $depbase.Tpo -c -o pml_ob1_isend.lo pml_ob1_isend.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile:  xlc_r -DHAVE_CONFIG_H -I. -I../../../../opal/include -I../../../../ompi/include -I../../../../oshmem/include -I../../../../opal/mca/hwloc/hwloc2a/hwloc/include/private/autogen -I../../../../opal/mca/hwloc/hwloc2a/hwloc/include/hwloc/autogen -I../../../../ompi/mpiext/cuda/c -I../../../.. -I../../../../orte/include -I/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/opal/mca/event/libevent2022/libevent -I/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/opal/mca/event/libevent2022/libevent/include -I/home/mpiczar/jenkins/workspace/ompi_public_pr_master_xl/ompi-src/opal/mca/hwloc/hwloc2a/hwloc/include -O3 -DNDEBUG -finline-functions -fno-strict-aliasing -MT pml_ob1_isend.lo -MD -MP -MF .deps/pml_ob1_isend.Tpo -c pml_ob1_isend.c  -fPIC -DPIC -o .libs/pml_ob1_isend.o
Calling signal handler...
1586-494 (U) INTERNAL COMPILER ERROR: Signal 11.
/opt/ibm/xlC/13.1.5/bin/.orig/xlc_r: error: 1501-230 Internal compiler error; please contact your Service Representative. For more information visit:
http://www.ibm.com/support/docview.wss?uid=swg21110810
make: *** [pml_ob1_isend.lo] Error 1

jsquyres

In general, this looks pretty great. I have a bunch of nit-picky comments, and I also have some slightly larger comments.

Additionally, there should likely be some form of documentation with this. E.g., a man page or something.

jsquyres · 2018-03-19T23:08:08Z

examples/spc_example.c

+    MPI_Comm_size(MPI_COMM_WORLD, &size);
+    if(size != 2) {
+        fprintf(stderr, "ERROR: This test should be run with two MPI processes.\n");
+        return -1;


Maybe better to MPI_ABORT here.

jsquyres · 2018-03-19T23:08:26Z

examples/spc_example.c

+    /* Make sure we found the counters */
+    if(index == -1) {
+        fprintf(stderr, "ERROR: Couldn't find the appropriate SPC counter in the MPI_T pvars.\n");
+        return -1;


Same here: MPI_ABORT.

jsquyres · 2018-03-19T23:09:24Z

examples/spc_example.c

+    /* Counter names to be read by ranks 0 and 1 */
+    char counter_names[2][40];
+    sprintf(counter_names[0], "runtime_spc_OMPI_BYTES_SENT_USER");
+    sprintf(counter_names[1], "runtime_spc_OMPI_BYTES_RECEIVED_USER");


Would be ok to use string constants here with C99 initialization (vs. using sprintf -- some compilers warn about using sprintf).

jsquyres · 2018-03-19T23:10:11Z

ompi/mca/pml/ob1/pml_ob1.c

@@ -36,6 +36,7 @@
 #include "opal_stdint.h"
 #include "opal/mca/btl/btl.h"
 #include "opal/mca/btl/base/base.h"
+#include "ompi/runtime/ompi_spc.h"


This doesn't appear to be used.

jsquyres · 2018-03-19T23:11:01Z

ompi/mca/pml/ob1/pml_ob1_recvfrag.c

        return remove_head_from_ordered_list(&proc->frags_cant_match);
-    }


Nit: this appears to be unrelated to SPC. Might want to exclude this change from this PR.

jsquyres · 2018-03-20T13:18:37Z

ompi/runtime/ompi_spc.c

+    int i, j, rank, world_size, offset, err;
+    long long *recv_buffer, *send_buffer;
+
+    ompi_communicator_t *comm = &ompi_mpi_comm_world.comm;


It may not be safe to use MPI_COMM_WORLD here. I think you might want to dup COMM_WORLD during your startup and then use that here in the dump (and then be sure to free that communicator when you're done with it).

jsquyres · 2018-03-20T13:19:33Z

ompi/runtime/ompi_spc.c

+    }
+
+    /* Aggregate all of the information on rank 0 using MPI_Gather on MPI_COMM_WORLD */
+    if(rank == 0) {


The only difference between these two blocks is the allocation of recv_buffer, right? Might want to just make that the only thing dependent upon rank == 0.

jsquyres · 2018-03-20T13:20:20Z

ompi/runtime/ompi_spc.c

+
+    /* Once rank 0 has all of the information, print the aggregated counter values for each rank in order */
+    if(rank == 0) {
+        fprintf(stdout, "OMPI Software Counters:\n");


Should we use fprintf or opal_output here?

Also, we use the name "Open MPI" publicly; we rarely use the name OMPI in help/output messages.

jsquyres · 2018-03-20T13:20:38Z

ompi/runtime/ompi_spc.c

+        fprintf(stdout, "OMPI Software Counters:\n");
+        offset = 0; /* Offset into the recv_buffer for each rank */
+        for(j = 0; j < world_size; j++) {
+            fprintf(stdout, "World Rank %d:\n", j);


I think you mean "MPI_COMM_WORLD rank".

jsquyres · 2018-03-20T13:21:22Z

ompi/runtime/ompi_spc.c

+     * This is denoted unlikely because the counters will often be turned off.
+     */
+    if(OPAL_UNLIKELY(attached_event[event_id] == 1 && *cycles == 0)) {
+        *cycles = opal_timer_base_get_cycles();


See above -- should this just use the high-precision timer.

thananon · 2018-03-20T21:51:32Z

Per discussion at face to face meeting on 03/20/18:

@davideberius will have to follow up with the reviews.
SPC is bound for 4.0.0 release. No need to PR over to 3.0.x or 3.1.x.
Have to get in touch with compiler guy from IBM to see whats wrong with XLC.
"counter_names" and "counter_descriptions" might not be a good name for a symbol due to possible name collision. We might have to raise this problem with PAPI team.

ibm-ompi · 2018-03-21T05:03:49Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/154b75812fe732a7906766782994adb0

ibm-ompi · 2018-03-21T05:27:32Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/9c0c0618be5243ba277701cda384f8ef

ibm-ompi · 2018-03-21T14:45:45Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/065cc2e477d9417e7d6e1987ee5dc903

bosilca · 2018-03-21T15:19:14Z

The XLC compiler seems puzzled by the existence of the following line of code "do {} while (0);".

ibm-ompi · 2018-03-21T15:29:38Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/504e194c3b351871519716e8e85541b6

ibm-ompi · 2018-03-21T21:17:23Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/f6d621757ccb5ea57cfe94d7251b8f93

jjhursey · 2018-03-22T16:21:10Z

ompi/runtime/ompi_spc.h

+#else /* SPCs are not enabled */
+
+#define SPC_INIT()  \
+    do {} while (0)


If this is the problem with XL, why don't you just have it:

#define SPC_INIT() ;

I was under the impression that some compilers do not like empty statements such as ";;".

hopefully this line is our problem.
We can test this hypothesis by putting something random there right?

ibm-ompi · 2018-03-22T20:40:42Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/fc7047f0071fd59b8deb07430b64b532

thananon · 2018-03-22T22:34:19Z

Seems like we have a winner!

jsquyres · 2018-03-23T19:11:51Z

Hmm. There's quite a few of my comments that are hidden by github in "outdated" blocks, but they weren't discussed / addressed. E.g., why invent your own bitmap handling instead of using opal_bitmap_t?

bosilca · 2018-03-23T22:00:25Z

opal_bitmap_t is an overkill for our needs and in this context the emphasis is on performance. This answer also applies to your question about frequencies, we cannot afford to call an expensive timer as we are in the critical path of the MPI library (as an example in the matching).

thananon · 2018-04-18T19:13:05Z

I just want to follow up on this PR. It has been a while.

Right now the code is functional. We just have some suggestions from @jsquyres that @bosilca does not agree with. (Regarding the CPU frequency changing dynamically, etc) So I request @ggouaillardet to pitch in.

If there is no discussion anymore, I suggest that we should merge this before it goes to the void.

thananon · 2018-05-08T20:35:08Z

Seems like no response will be made. @jsquyres can we agree and merge this?

jsquyres

I'm still quite hesitant about the variable frequency issue. I can be out-voted here, but Intel chips can (and do) change frequency depending on load. For a well-behaving HPC application (i.e., something that runs the CPU at 100%), the frequency likely won't change. But that may not be true in the general case (i.e., for all the plebian HPC apps out there).

jsquyres · 2018-05-08T21:24:42Z

examples/spc_example.c

+    char name[256], description[256];
+
+    /* Counter names to be read by ranks 0 and 1 */
+    char counter_names[2][40] = { "runtime_spc_OMPI_BYTES_SENT_USER",


Should probably be char *counter_names[] = { "...

jsquyres · 2018-05-08T21:25:40Z

test/spc/spc_test.c

+    char name[256], description[256];
+
+    /* Counter names to be read by ranks 0 and 1 */
+    char counter_names[2][40] = { "runtime_spc_OMPI_BYTES_SENT_USER",


Same as before: char *counter_names[] = ...

jsquyres · 2018-05-08T21:27:06Z

ompi/runtime/ompi_spc.h

+ */
+
+/* This enumeration serves as event ids for the various events */
+enum ompi_spc_counters_t {


My suggestion earlier was in shorthand. This should probably be:

typedef enum ompi_spc_counters { ... } ompi_spec_counters_t

I think this will make it like many other declarations in OMPI.

jsquyres · 2018-05-08T21:29:21Z

ompi/runtime/ompi_spc.h

+    OMPI_OOS_IN_QUEUE,
+    OMPI_MAX_UNEXPECTED_IN_QUEUE,
+    OMPI_MAX_OOS_IN_QUEUE,
+    OMPI_SPC_NUM_COUNTERS /* This serves as the number of counters.  It must be last. */


My prior comment actually intended you to put SPC_ in all of these enums. OMPI_SPC_NUM_COUNTERS was a single example, but it also applies to OMPI_SPC_MAX_OOS_IN_QUEUE, OMPI_SPC_MAX_UNEXPECTED_IN_QUEUE, ...etc. I know this is a PITA, but this is a limitation of C -- we need to have decent prefixes to our values as a primitive namespace. Sorry. 😦

jsquyres · 2018-05-08T21:29:53Z

ompi/runtime/ompi_spc.c

+ */
+static int ompi_spc_get_count(const struct mca_base_pvar_t *pvar, void *value, void *obj_handle)
+{
+    (void) obj_handle;


As I mentioned twice, please use __opal_attribute_unused__.

jsquyres · 2018-05-08T21:32:57Z

ompi/runtime/ompi_spc.c

+        *counter_value /= sys_clock_freq_mhz;
+    }
+    /* If this is a high watermark counter, reset it after it has been read */
+    if(index == OMPI_MAX_UNEXPECTED_IN_QUEUE || index == OMPI_MAX_OOS_IN_QUEUE) {


Per my previous comment:

It might be better to have some kind of attribute somewhere that indicates that a particular SPC counter needs to be reset after being read (Vs. having a hard-coded list of specific enums here in the code).

jsquyres · 2018-05-08T21:34:51Z

ompi/runtime/ompi_spc.c

+    }
+
+    /* Aggregate all of the information on rank 0 using MPI_Gather on MPI_COMM_WORLD */
+    send_buffer = (long long*)malloc(OMPI_SPC_NUM_COUNTERS * sizeof(long long));


Per my previous comment, please check for malloc failure.

This will be fixed this evening.

Actually, this was already fixed, but this line didn't change so the comment still shows.

jsquyres · 2018-05-08T21:36:09Z

ompi/runtime/ompi_spc.h

+} ompi_spc_t;
+
+/* Events data structure initialization function */
+void events_init(void);


This function name needs to be prefixed with ompi_spc_.

jsquyres · 2018-05-08T21:43:35Z

ompi/runtime/ompi_spc.c

+    int i, j, world_size, offset;
+    long long *recv_buffer = NULL, *send_buffer;
+
+    ompi_communicator_t *comm = &ompi_mpi_comm_world.comm;


Per my prior comment:

It may not be safe to use MPI_COMM_WORLD here. I think you might want to dup COMM_WORLD during your startup and then use that here in the dump (and then be sure to free that communicator when you're done with it).

I'll fix this this evening.

bosilca · 2018-05-09T15:48:18Z

@jsquyres what we are measuring are very short time intervals (such as the matching time). The change for a clock frequency change during such measurements is minimal (frankly non-existent) so I would not be concerned about the lack of accuracy. Moreover, we never return to the user absolute timing, so even if there is a drift in frequency during the execution the impact on the value returned will be minimal.

jsquyres · 2018-05-11T15:49:52Z

@bosilca I hear what you're saying. And I agree: it'll be pretty rare for the frequency to change while you're making the measurement. But:

It can still happen (e.g., if an app idles for a while before a message is received, e.g., while (!done) { MPI_Test(...); if (flag) break; sleep(1); }. Yes, we've all seen apps that do this. 😉 ).
The frequency is measured at the beginning of the application -- not during each measurement. The chance of the frequency being different at the beginning of the application vs. the time of measurement is actually not small.

Here's an example:

An always-full HPC cluster finishes one job.
The job scheduler does all of its cleanup stuff at the end of the job, and resets the nodes for the next job.
During this time -- which may take a minute or three -- all but one of the cores spin down to a lower frequency.
The job scheduler finally gets the next job and starts it up, one process per core.
Each process calls MPI_INIT right away, which samples the frequency.
The process on the first core may will get a high frequency (because it's where the job scheduler was running and has been busy the whole time). But all the other cores may be in a lower frequency because they were more-or-less idle for a little while.
After a short while running the MPI apps, all the cores are back up to their full frequency, but the damage was already done: they measured their frequency during MPI_INIT as X, but now they're running at frequency Y (where X<Y).

That can definitely happen.

I'm also curious: what is the usefulness of having fast-but-inaccurate timings? If x% of your timings could be incorrect, how is that data useful?

Maybe it would be enough to have 2 different methods of timing: one based on your current frequency method, and another based on slower/more-accurate methods (e.g., clock_gettime()), and have an MCA param to switch between the two. There are tradeoffs to both methods, of course:

Fast but possibly inaccurate.
Slow by possibly affects the overall runtime (because it's adding "slow" clock checks in performance-critical code paths).

jsquyres · 2018-05-11T15:53:02Z

Right after I hit submit, I thought of a better example where the frequency can change: while blocking for disk I/O. E.g.:

while (!done) {
    MPI_Test(...);
    if (flag) break;
    read(fd, large_chunk_of_disk_data, ...);
}

Given that the read() is blocking, it could actually swap out for a little while (especially if it's waiting on a device or the network), and could actually affect the core frequency.

(this is somewhat moot because the frequency is only measured at the beginning of the app, but it's just better example than the sleep() example I listed above)

bosilca · 2018-05-14T14:29:25Z

@jsquyres :

if the cost of taking the measure significantly impacts the process itself, it is not worth measuring.
we never measure things across MPI calls (it is important because)
current processors do not exhibit the problem you are describing anymore.

The example you provided will work just fine with modern processors. For older processors the results might be off if there are frequency changes between MPI calls, but there I am not sure that people really care about measuring matching time.

davideberius · 2018-06-06T04:49:30Z

I checked to make sure that we are verifying that the timers are monotonic. The following code shows how this is done right now. Should I add an SPC specific warning message here?

int opal_timer_linux_open(void)
{
    int ret = OPAL_SUCCESS;

    if (mca_timer_base_monotonic && !opal_sys_timer_is_monotonic ()) {
#if OPAL_HAVE_CLOCK_GETTIME && (0 == OPAL_TIMER_MONOTONIC)
        struct timespec res;
        if( 0 == clock_getres(CLOCK_MONOTONIC, &res)) {
            opal_timer_linux_freq = 1.e3;
            opal_timer_base_get_cycles = opal_timer_linux_get_cycles_clock_gettime;
            opal_timer_base_get_usec = opal_timer_linux_get_usec_clock_gettime;
            return ret;
        }
#else
        /* Monotonic time requested but cannot be found. Complain! */
        opal_show_help("help-opal-timer-linux.txt", "monotonic not supported", true);
#endif  /* OPAL_HAVE_CLOCK_GETTIME && (0 == OPAL_TIMER_MONOTONIC) */
    }
    ret = opal_timer_linux_find_freq();
    opal_timer_base_get_cycles = opal_timer_linux_get_cycles_sys_timer;
    opal_timer_base_get_usec = opal_timer_linux_get_usec_sys_timer;
    return ret;
}

thananon · 2018-06-06T15:16:38Z

We would like to get this into 4.0. The fork is this Friday IIRC. We should reach agreement on that timer/tick rate or actually remove it completely. The new matching is fast anyway.

bosilca · 2018-06-06T16:10:28Z

@jsquyres poke

davideberius · 2018-06-06T16:13:51Z

ompi/runtime/ompi_spc.c

+void ompi_spc_fini(void)
+{
+#if SPC_ENABLE == 1
+    ompi_spc_dump();


It looks like I forgot to add the check for whether dumping is enabled. I'll add this when I get home this evening.

davideberius · 2018-06-07T02:46:30Z

Ok, I've made all of the changes requested. What is the status of the frequency issue?

bosilca · 2018-06-09T14:04:18Z

I don't think there is an issue anymore. We discussed it with Jeff privately, and I think we agreed that as long as there is a warning we are good to go. But I hope @jsquyres will confirm and then approve this PR so that we can merge.

jsquyres · 2018-06-10T15:00:52Z

I just filed a quick PR (davideberius#1 -- I couldn't seem to push to @davideberius's PR branch for some reason) with some minor suggestions.

In terms of the frequency issue, there's two points:

George pointed out that we're using the invariant frequency, so the change-the-frequency issue shouldn't be an issue. Ok, I'm good with that.
However, that's still only relevant to an individual core. I still wouldn't mind a warning of some kind if we know that you may well get some level of non-determinism in the results (e.g., if your process is not bound to core, and therefore you may see the cycle count from different cores upon subsequent sampling).

bosilca · 2018-06-10T22:04:23Z

@davideberius highlighted that we have the same level of protection for timers as MPI_Wtime. And we inherit the same level of warning.

thananon

Looking great. Squash and go.

davideberius · 2018-06-11T18:14:41Z

Ok, I'll squash this when I get back home tonight. Should this all be one commit?

bosilca · 2018-06-11T20:31:27Z

Technically there is no strict rule on the number of commits (the details are left to the PR author(s)). In this particular case, due to a significant number of updates, I would indeed squash down to 1.

…ral counters. This code is the implementation of Software-base Performance Counters as described in the paper 'Using Software-Base Performance Counters to Expose Low-Level Open MPI Performance Information' in EuroMPI/USA '17 (http://icl.cs.utk.edu/news_pub/submissions/software-performance-counters.pdf). More practical usage information can be found here: https://github.com/davideberius/ompi/wiki/How-to-Use-Software-Based-Performance-Counters-(SPCs)-in-Open-MPI. All software events functions are put in macros that become no-ops when SOFTWARE_EVENTS_ENABLE is not defined. The internal timer units have been changed to cycles to avoid division operations which was a large source of overhead as discussed in the paper. Added a --with-spc configure option to enable SPCs in the Open MPI build. This defines SOFTWARE_EVENTS_ENABLE. Added an MCA parameter, mpi_spc_enable, for turning on specific counters. Added an MCA parameter, mpi_spc_dump_enabled, for turning on and off dumping SPC counters in MPI_Finalize. Added an SPC test and example. Signed-off-by: David Eberius <[email protected]>

davideberius · 2018-06-12T02:53:02Z

I have squashed all of the commits into a single commit.

jsquyres · 2018-06-12T14:06:56Z

bot:mellanox:retest
bot:ompi:retest

Looks like the Jenkins/Github git clone bug again. 😦

jsquyres

@bosilca Ok, fair enough that we're in the same category as MPI_WTICK/MPI_WTIME, which means that my concerns are not specific to this PR (i.e., we shouldn't hold up this PR any further, and should instead address that issue outside of this PR). I would still like to see some kind of disable-able warning about this ("your time may not be constant!"), but that will be outside of this PR.

bosilca · 2018-06-12T14:17:08Z

More than what @davideberius pinpointed to few comments above ?

opal_show_help("help-opal-timer-linux.txt", "monotonic not supported", true);

jsquyres · 2018-06-12T14:22:32Z

@bosilca Yes, more than that (but perhaps similar?) -- opal_sys_timer_is_monotonic() only returns if the system clock supports monotonic time or not. It does not take into effect process binding (which will be the more likely case: your system supports monotonic time, but your process isn't bound to where it will consistently get the same clock).

Dumb question: is the OMPI time-invariant clock-reading-code getting its values from a core or from a package? (I assume core, but I don't know that for a fact) I.e., if a process is bound to a socket/package, is it still going to always get consistent time?

bosilca · 2018-06-12T15:51:06Z

My understanding is that it depends on the processor, but I have no idea how to check what processor has what type of monotonic hwclock. I agree that from a user perspective it could be nice to be extremely attentive, but realistically it's a lot of work for little benefit.

thananon assigned bosilca Mar 7, 2018

thananon requested a review from bosilca March 7, 2018 21:31

bosilca approved these changes Mar 7, 2018

View reviewed changes

davideberius force-pushed the spc_pr branch from 3713710 to 2a3d40d Compare March 7, 2018 21:32

thananon added enhancement Target: main labels Mar 7, 2018

davideberius force-pushed the spc_pr branch from 2a3d40d to 5b36aa7 Compare March 7, 2018 21:36

davideberius force-pushed the spc_pr branch from 5b36aa7 to 5011683 Compare March 7, 2018 21:58

jsquyres requested changes Mar 20, 2018

View reviewed changes

bosilca force-pushed the spc_pr branch from cb64758 to ff20028 Compare March 21, 2018 05:13

bosilca force-pushed the spc_pr branch from ff20028 to 0d9278e Compare March 21, 2018 14:31

jjhursey reviewed Mar 22, 2018

View reviewed changes

thananon assigned ggouaillardet Apr 6, 2018

jsquyres requested changes May 8, 2018

View reviewed changes

davideberius commented Jun 6, 2018

View reviewed changes

thananon approved these changes Jun 11, 2018

View reviewed changes

davideberius force-pushed the spc_pr branch from 772ac01 to d377a6b Compare June 12, 2018 02:51

jsquyres approved these changes Jun 12, 2018

View reviewed changes

thananon merged commit 390d72a into open-mpi:master Jun 12, 2018

		return remove_head_from_ordered_list(&proc->frags_cant_match);
		}

Initial Software-based Performance Counters PR #4885

Initial Software-based Performance Counters PR #4885

Conversation

davideberius commented Mar 7, 2018 • edited by jsquyres Loading

ompiteam-bot commented Mar 7, 2018

thananon commented Mar 7, 2018

jsquyres commented Mar 7, 2018

ibm-ompi commented Mar 7, 2018

ibm-ompi commented Mar 7, 2018

ggouaillardet commented Mar 8, 2018

ibm-ompi commented Mar 8, 2018

thananon commented Mar 8, 2018

jjhursey commented Mar 8, 2018

jsquyres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thananon commented Mar 20, 2018

ibm-ompi commented Mar 21, 2018

ibm-ompi commented Mar 21, 2018

ibm-ompi commented Mar 21, 2018

bosilca commented Mar 21, 2018

ibm-ompi commented Mar 21, 2018

ibm-ompi commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibm-ompi commented Mar 22, 2018

thananon commented Mar 22, 2018

jsquyres commented Mar 23, 2018

bosilca commented Mar 23, 2018

thananon commented Apr 18, 2018

thananon commented May 8, 2018

jsquyres left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bosilca commented May 9, 2018

jsquyres commented May 11, 2018

jsquyres commented May 11, 2018

bosilca commented May 14, 2018

davideberius commented Jun 6, 2018 • edited Loading

thananon commented Jun 6, 2018

bosilca commented Jun 6, 2018

Choose a reason for hiding this comment

davideberius commented Jun 7, 2018

bosilca commented Jun 9, 2018

jsquyres commented Jun 10, 2018

bosilca commented Jun 10, 2018

thananon left a comment

Choose a reason for hiding this comment

davideberius commented Jun 11, 2018

bosilca commented Jun 11, 2018

davideberius commented Jun 12, 2018

jsquyres commented Jun 12, 2018

jsquyres left a comment

Choose a reason for hiding this comment

bosilca commented Jun 12, 2018

jsquyres commented Jun 12, 2018

bosilca commented Jun 12, 2018

davideberius commented Mar 7, 2018 •

edited by jsquyres

Loading

davideberius commented Jun 6, 2018 •

edited

Loading