commit 660bfd20bc89a26629e99de958d38b031db4250d
Author: William Cohen <wcohen@redhat.com>
Date: Thu Oct 31 15:30:00 2019 -0400
This code is a modification of krentel_pthreads.c, to better test
some race conditions. It is not included in the standard tests;
it is a diagnostic that should be run with "valgrind --tool=helgrind".
Signed-off-by: Anthony Castaldo <TonyCastaldo@icl.utk.edu>
diff --git a/src/ctests/krentel_pthreads_race.c b/src/ctests/krentel_pthreads_race.c
new file mode 100644
index 000000000..0ebfb5056
--- /dev/null
+++ b/src/ctests/krentel_pthreads_race.c
@@ -0,0 +1,236 @@
+/*
+ * Test PAPI with multiple threads.
+ * This code is a modification of krentel_pthreads.c by William Cohen
+ * <wcohen@redhat.com>, on Sep 10 2019, to exercise and test for the race
+ * condition in papi_internal.c involving the formerly static variables
+ * papi_event_code and papi_event_code_changed. This code should be run with
+ * "valgrind --tool=helgrind" to show any data races. If run with:
+ * "valgrind --tool=helgrind --log-file=helgrind_out.txt"
+ * The output will be captured in helgrind_out.txt and can then be processed
+ * with the program filter_helgrind.c; see commentary at the top of that file.
+ */
+
+#define MAX_THREADS 256
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <sys/time.h>
+
+#include "papi.h"
+#include "papi_test.h"
+
+#define EVENT PAPI_TOT_CYC
+
+static int program_time = 5;
+static int threshold = 20000000;
+static int num_threads = 3;
+
+static long count[MAX_THREADS];
+static long iter[MAX_THREADS];
+static struct timeval last[MAX_THREADS];
+
+static pthread_key_t key;
+
+static struct timeval start;
+
+static void
+my_handler( int EventSet, void *pc, long long ovec, void *context )
+{
+ ( void ) EventSet;
+ ( void ) pc;
+ ( void ) ovec;
+ ( void ) context;
+
+ long num = ( long ) pthread_getspecific( key );
+
+ if ( num < 0 || num > num_threads )
+ test_fail( __FILE__, __LINE__, "getspecific failed", 1 );
+ count[num]++;
+}
+
+static void
+print_rate( long num )
+{
+ struct timeval now;
+ long st_secs;
+ double last_secs;
+
+ gettimeofday( &now, NULL );
+ st_secs = now.tv_sec - start.tv_sec;
+ last_secs = ( double ) ( now.tv_sec - last[num].tv_sec )
+ + ( ( double ) ( now.tv_usec - last[num].tv_usec ) ) / 1000000.0;
+ if ( last_secs <= 0.001 )
+ last_secs = 0.001;
+
+ if (!TESTS_QUIET) {
+ printf( "[%ld] time = %ld, count = %ld, iter = %ld, "
+ "rate = %.1f/Kiter\n",
+ num, st_secs, count[num], iter[num],
+ ( 1000.0 * ( double ) count[num] ) / ( double ) iter[num] );
+ }
+
+ count[num] = 0;
+ iter[num] = 0;
+ last[num] = now;
+}
+
+static void
+do_cycles( long num, int len )
+{
+ struct timeval start, now;
+ double x, sum;
+
+ gettimeofday( &start, NULL );
+
+ for ( ;; ) {
+ sum = 1.0;
+ for ( x = 1.0; x < 250000.0; x += 1.0 )
+ sum += x;
+ if ( sum < 0.0 )
+ printf( "==>> SUM IS NEGATIVE !! <<==\n" );
+
+ iter[num]++;
+
+ gettimeofday( &now, NULL );
+ if ( now.tv_sec >= start.tv_sec + len )
+ break;
+ }
+}
+
+static void *
+my_thread( void *v )
+{
+ long num = ( long ) v;
+ int n;
+ int EventSet = PAPI_NULL;
+ int event_code;
+ long long value;
+
+ int retval;
+
+ retval = PAPI_register_thread( );
+ if ( retval != PAPI_OK ) {
+ test_fail( __FILE__, __LINE__, "PAPI_register_thread", retval );
+ }
+ pthread_setspecific( key, v );
+
+ count[num] = 0;
+ iter[num] = 0;
+ last[num] = start;
+
+ retval = PAPI_create_eventset( &EventSet );
+ if ( retval != PAPI_OK ) {
+ test_fail( __FILE__, __LINE__, "PAPI_create_eventset failed", retval );
+ }
+
+ retval = PAPI_event_name_to_code("PAPI_TOT_CYC", &event_code);
+ if (retval != PAPI_OK ) {
+ if (!TESTS_QUIET) printf("Trouble creating event name\n");
+ test_fail( __FILE__, __LINE__, "PAPI_event_name_to_code failed", retval );
+ }
+
+ retval = PAPI_add_event( EventSet, EVENT );
+ if (retval != PAPI_OK ) {
+ if (!TESTS_QUIET) printf("Trouble adding event\n");
+ test_fail( __FILE__, __LINE__, "PAPI_add_event failed", retval );
+ }
+
+ if ( PAPI_overflow( EventSet, EVENT, threshold, 0, my_handler ) != PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_overflow failed", 1 );
+
+ if ( PAPI_start( EventSet ) != PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_start failed", 1 );
+
+ if (!TESTS_QUIET) printf( "launched timer in thread %ld\n", num );
+
+ for ( n = 1; n <= program_time; n++ ) {
+ do_cycles( num, 1 );
+ print_rate( num );
+ }
+
+ PAPI_stop( EventSet, &value );
+
+ retval = PAPI_overflow( EventSet, EVENT, 0, 0, my_handler);
+ if ( retval != PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_overflow failed to reset the overflow handler", retval );
+
+ if ( PAPI_remove_event( EventSet, EVENT ) != PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_remove_event", 1 );
+
+ if ( PAPI_destroy_eventset( &EventSet ) != PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_destroy_eventset", 1 );
+
+ if ( PAPI_unregister_thread( ) != PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_unregister_thread", 1 );
+
+ return ( NULL );
+}
+
+int
+main( int argc, char **argv )
+{
+ pthread_t *td = NULL;
+ long n;
+ int quiet,retval;
+
+ /* Set TESTS_QUIET variable */
+ quiet=tests_quiet( argc, argv );
+
+ if ( argc < 2 || sscanf( argv[1], "%d", &program_time ) < 1 )
+ program_time = 6;
+ if ( argc < 3 || sscanf( argv[2], "%d", &threshold ) < 1 )
+ threshold = 20000000;
+ if ( argc < 4 || sscanf( argv[3], "%d", &num_threads ) < 1 )
+ num_threads = 32;
+
+ td = malloc((num_threads+1) * sizeof(pthread_t));
+ if (!td) {
+ test_fail( __FILE__, __LINE__, "td malloc failed", 1 );
+ }
+
+ if (!quiet) {
+ printf( "program_time = %d, threshold = %d, num_threads = %d\n\n",
+ program_time, threshold, num_threads );
+ }
+
+ if ( PAPI_library_init( PAPI_VER_CURRENT ) != PAPI_VER_CURRENT )
+ test_fail( __FILE__, __LINE__, "PAPI_library_init failed", 1 );
+
+ /* Test to be sure we can add events */
+ retval = PAPI_query_event( EVENT );
+ if (retval!=PAPI_OK) {
+ if (!quiet) printf("Trouble finding event\n");
+ test_skip(__FILE__,__LINE__,"Event not available",1);
+ }
+
+ if ( PAPI_thread_init( ( unsigned long ( * )( void ) ) ( pthread_self ) ) !=
+ PAPI_OK )
+ test_fail( __FILE__, __LINE__, "PAPI_thread_init failed", 1 );
+
+ if ( pthread_key_create( &key, NULL ) != 0 )
+ test_fail( __FILE__, __LINE__, "pthread key create failed", 1 );
+
+ gettimeofday( &start, NULL );
+
+ for ( n = 1; n <= num_threads; n++ ) {
+ if ( pthread_create( &(td[n]), NULL, my_thread, ( void * ) n ) != 0 )
+ test_fail( __FILE__, __LINE__, "pthread create failed", 1 );
+ }
+
+ my_thread( ( void * ) 0 );
+
+ /* wait for all the threads */
+ for ( n = 1; n <= num_threads; n++ ) {
+ if ( pthread_join( td[n], NULL))
+ test_fail( __FILE__, __LINE__, "pthread join failed", 1 );
+ }
+
+ free(td);
+
+ if (!quiet) printf( "done\n" );
+
+ test_pass( __FILE__ );
+
+ return 0;
+}
commit 979e80136fd5e0ee2fb26f7374b36a8433147a68
Author: Anthony Castaldo <TonyCastaldo@icl.utk.edu>
Date: Thu Oct 31 15:56:55 2019 -0400
The changes to papi.c, papi_internal.c, threads.h and threads.c
correct a race condition that was the result of all threads using
the same two static variables (papi_event_code and papi_event_code_changed)
to temporarily record a state of operation. The solution was to
make these variables unique per thread, using the ThreadInfo_t
structure already provided in PAPI for such purposes. The file
krentel_pthread_race.c is a stress test to produce race conditions.
filter_helgrind.c reduces the volume of --tool-helgrind output to
a more manageable summary. Both are added to Makefile.recipies.
diff --git a/src/ctests/Makefile.recipies b/src/ctests/Makefile.recipies
index 87340831d..b7c1963d7 100644
--- a/src/ctests/Makefile.recipies
+++ b/src/ctests/Makefile.recipies
@@ -161,6 +161,12 @@ locks_pthreads: locks_pthreads.c $(TESTLIB) $(PAPILIB)
krentel_pthreads: krentel_pthreads.c $(TESTLIB) $(PAPILIB)
$(CC_R) $(INCLUDE) $(CFLAGS) $(TOPTFLAGS) krentel_pthreads.c $(TESTLIB) $(PAPILIB) $(LDFLAGS) -o krentel_pthreads -lpthread
+# krentel_pthreads_race is not included with the standard tests;
+# it is a modification of krentel_pthreads intended to be run with
+# "valgrind --tool=helgrind" to test for race conditions.
+krentel_pthreads_race: krentel_pthreads_race.c $(TESTLIB) $(PAPILIB)
+ $(CC_R) $(INCLUDE) $(CFLAGS) $(TOPTFLAGS) krentel_pthreads_race.c $(TESTLIB) $(PAPILIB) $(LDFLAGS) -o krentel_pthreads_race -lpthread
+
overflow_pthreads: overflow_pthreads.c $(TESTLIB) $(DOLOOPS) $(PAPILIB)
$(CC_R) $(INCLUDE) $(CFLAGS) $(TOPTFLAGS) overflow_pthreads.c $(TESTLIB) $(DOLOOPS) $(PAPILIB) $(LDFLAGS) -o overflow_pthreads -lpthread
@@ -434,6 +440,9 @@ forkexec4: forkexec4.c $(TESTLIB) $(PAPILIB)
prof_utils.o: prof_utils.c $(testlibdir)/papi_test.h prof_utils.h
$(CC) $(INCLUDE) $(CFLAGS) $(TOPTFLAGS) -c prof_utils.c
+filter_helgrind: filter_helgrind.c $(TESTLIB) $(PAPILIB)
+ -$(CC) $(INCLUDE) $(CFLAGS) $(TOPTFLAGS) filter_helgrind.c $(TESTLIB) $(PAPILIB) $(LDFLAGS) -o filter_helgrind
+
.PHONY : all default ctests ctest clean
clean:
diff --git a/src/ctests/filter_helgrind.c b/src/ctests/filter_helgrind.c
new file mode 100644
index 000000000..d918a789e
--- /dev/null
+++ b/src/ctests/filter_helgrind.c
@@ -0,0 +1,170 @@
+/*
+ * This code is a simple filter for the helgrind_out.txt file
+ * produced by:
+ * "valgrind --tool=helgrind --log-file=helgrind_out.txt someProgram"
+ *
+ * This is useful because the tool does not recognize PAPI locks,
+ * thus reports as possible race conditions reads/writes by
+ * different threads that are actually fine (surrounded by locks).
+ *
+ * This was written particularly for krentel_pthreads_race.c
+ * when processed by the above valgrind. We produce a line per
+ * condition, in the form:
+ * OP@file:line OP@file:line
+ * where OP is R or W. The first file:line code occurred
+ * after the second file:line code, and on a different thread.
+ *
+ * We print the results to stdout. It is useful to filter this
+ * through the standard utility 'uniq', each occurrence only
+ * needs to be investigated once. Just insure there are
+ * MATCHING locks around each operation within the code.
+ *
+ * An example run (using uniq): The options -uc will print
+ * only unique lines, preceeded by a count of how many times
+ * it occurs.
+ *
+ * ./filter_helgrind | uniq -uc
+ *
+ * An example output line (piped through uniq as above):
+ * 1 R@threads.c:190 W@threads.c:206
+ * An investigation shows threads.c:190 is protected by
+ * _papi_hwi_lock(THREADS_LOCK); and threads.c:206 is
+ * protected by the same lock. Thus no data race can
+ * occur for this instance.
+ *
+ * Compilation within the papi/src/ctests directory:
+ * make filter_helgrind
+ *
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+int main(int argc, char** args) {
+ (void) argc;
+ (void) args;
+
+ char myLine[16384];
+ int state, size;
+ char type1, type2;
+ char fname1[256], fname2[256];
+ char *paren1, *paren2;
+
+ FILE *HELOUT = fopen("helgrind_out.txt", "r"); // Read the file.
+ if (HELOUT == NULL) {
+ fprintf(stderr, "Could not open helgrind_out.txt.\n");
+ exit(-1);
+ }
+
+ char PDRR[]="Possible data race during read";
+ char PDRW[]="Possible data race during write";
+ char TCWW[]="This conflicts with a previous write";
+ char TCWR[]="This conflicts with a previous read";
+ char atSTR[]=" at ";
+
+ // State machine:
+ // State 0: We are looking for a line with PDRR or PDRW.
+ // We don't exit until we find it, or run out of lines.
+ // if we find it, we remember which and go to state 1.
+ // State 1: Looking for " at " in column 11.
+ // When found, we extract the string betweeen '(' and ')'
+ // which is program name:line. go to state 2.
+ // State 2: We are searching for TCWW, TCWR, PDRW, PDRR.
+ // If we find the first two:
+ // Remember which, and go to state 3.
+ // If we find either of the second two, go back to State 1.
+ // State 3: Looking for " at " in column 11.
+ // When found, extract the string betweeen '(' and ')',
+ // which is program name:line.
+ // OUTPUT LINE for an investigation.
+ // Go to State 0.
+
+ state = 0; // looking for PDRR, PDRW.
+ while (fgets(myLine, 16384, HELOUT) != NULL) {
+ if (strlen(myLine) < 20) continue;
+ switch (state) {
+ case 0: // Looking for PDRR or PRDW.
+ if (strstr(myLine, PDRR) != NULL) {
+ type1='R';
+ state=1;
+ continue;
+ }
+
+ if (strstr(myLine, PDRW) != NULL) {
+ type1='W';
+ state=1;
+ continue;
+ }
+
+ continue;
+ break;
+
+ case 1: // Looking for atSTR in column 11.
+ if (strncmp(myLine+10, atSTR, 6) != 0) continue;
+ paren1=strchr(myLine, '(');
+ paren2=strchr(myLine, ')');
+ if (paren1 == NULL || paren2 == NULL ||
+ paren1 > paren2) {
+ state=0; // Abort, found something I don't understand.
+ continue;
+ }
+
+ size = paren2-paren1-1; // compute length of name.
+ strncpy(fname1, paren1+1, size); // Copy the name.
+ fname1[size]=0; // install z-terminator.
+ state=2;
+ continue;
+ break;
+
+ case 2: // Looking for TCWW, TCWR, PDRR, PDRW.
+ if (strstr(myLine, TCWR) != NULL) {
+ type2='R';
+ state=3;
+ continue;
+ }
+
+ if (strstr(myLine, TCWW) != NULL) {
+ type2='W';
+ state=3;
+ continue;
+ }
+
+ if (strstr(myLine, PDRR) != NULL) {
+ type1='R';
+ state=1;
+ continue;
+ }
+
+ if (strstr(myLine, PDRW) != NULL) {
+ type1='W';
+ state=1;
+ continue;
+ }
+
+ continue;
+ break;
+
+ case 3: // Looking for atSTR in column 11.
+ if (strncmp(myLine+10, atSTR, 6) != 0) continue;
+ paren1=strchr(myLine, '(');
+ paren2=strchr(myLine, ')');
+ if (paren1 == NULL || paren2 == NULL ||
+ paren1 > paren2) {
+ state=0; // Abort, found something I don't understand.
+ continue;
+ }
+
+ size = paren2-paren1-1; // compute length of name.
+ strncpy(fname2, paren1+1, size); // Copy the name.
+ fname2[size]=0; // install z-terminator.
+ fprintf(stdout, "%c@%-32s %c@%-32s\n", type1, fname1, type2, fname2);
+ state=0;
+ continue;
+ break;
+ } // end switch.
+ } // end while.
+
+ fclose(HELOUT);
+ exit(0);
+}
diff --git a/src/papi.c b/src/papi.c
index 4e08dc840..070e3f8c6 100644
--- a/src/papi.c
+++ b/src/papi.c
@@ -608,32 +608,26 @@ PAPI_library_init( int version )
papi_return( init_retval );
}
- /* Initialize component globals */
+ /* Initialize thread globals, including the main threads */
- tmp = _papi_hwi_init_global( );
+ tmp = _papi_hwi_init_global_threads( );
if ( tmp ) {
init_retval = tmp;
_papi_hwi_shutdown_global_internal( );
- _in_papi_library_init_cnt--;
+ _in_papi_library_init_cnt--;
papi_return( init_retval );
}
-
- /* Initialize thread globals, including the main threads */
- tmp = _papi_hwi_init_global_threads( );
+ /* Initialize component globals */
+
+ tmp = _papi_hwi_init_global( );
if ( tmp ) {
- int i;
init_retval = tmp;
_papi_hwi_shutdown_global_internal( );
- for ( i = 0; i < papi_num_components; i++ ) {
- if (!_papi_hwd[i]->cmp_info.disabled) {
- _papi_hwd[i]->shutdown_component( );
- }
- }
_in_papi_library_init_cnt--;
papi_return( init_retval );
}
-
+
init_level = PAPI_LOW_LEVEL_INITED;
_in_papi_library_init_cnt--;
diff --git a/src/papi_internal.c b/src/papi_internal.c
index 2412eca63..f0e457bf7 100644
--- a/src/papi_internal.c
+++ b/src/papi_internal.c
@@ -111,31 +111,28 @@ _papi_hwi_free_papi_event_string() {
}
return;
}
-// A place to keep the current papi event code so some component functions can fetch its value
-// The current event code can be stored here prior to component calls and cleared after the component returns
-static unsigned int papi_event_code = -1;
-static int papi_event_code_changed = -1;
+
void
_papi_hwi_set_papi_event_code (unsigned int event_code, int update_flag) {
INTDBG("new event_code: %#x, update_flag: %d, previous event_code: %#x\n", event_code, update_flag, papi_event_code);
// if call is just to reset and start over, set both flags to show nothing saved yet
if (update_flag < 0) {
- papi_event_code_changed = -1;
- papi_event_code = -1;
+ _papi_hwi_my_thread->tls_papi_event_code_changed = -1;
+ _papi_hwi_my_thread->tls_papi_event_code = -1;
return;
}
// if 0, it is being set prior to calling a component, if >0 it is being changed by the component
- papi_event_code_changed = update_flag;
+ _papi_hwi_my_thread->tls_papi_event_code_changed = update_flag;
// save the event code passed in
- papi_event_code = event_code;
+ _papi_hwi_my_thread->tls_papi_event_code = event_code;
return;
}
unsigned int
_papi_hwi_get_papi_event_code () {
INTDBG("papi_event_code: %#x\n", papi_event_code);
- return papi_event_code;
+ return _papi_hwi_my_thread->tls_papi_event_code;
}
/* Get the index into the ESI->NativeInfoArray for the current PAPI event code */
int
@@ -560,7 +557,7 @@ _papi_hwi_native_to_eventcode(int cidx, int event_code, int ntv_idx, const char
int result;
- if (papi_event_code_changed > 0) {
+ if (_papi_hwi_my_thread->tls_papi_event_code_changed > 0) {
result = _papi_hwi_get_papi_event_code();
INTDBG("EXIT: papi_event_code: %#x set by the component\n", result);
return result;
diff --git a/src/threads.c b/src/threads.c
index 4dd0cf4e3..9f586c415 100644
--- a/src/threads.c
+++ b/src/threads.c
@@ -286,6 +286,10 @@ _papi_hwi_initialize_thread( ThreadInfo_t ** dest, int tid )
return PAPI_ENOMEM;
}
+ /* init event memory variables, used by papi_internal.c */
+ thread->tls_papi_event_code = -1;
+ thread->tls_papi_event_code_changed = -1;
+
/* Call the component to fill in anything special. */
for ( i = 0; i < papi_num_components; i++ ) {
@@ -421,6 +425,11 @@ _papi_hwi_shutdown_thread( ThreadInfo_t * thread, int force_shutdown )
unsigned long tid;
int i, failure = 0;
+ /* Clear event memory variables */
+ thread->tls_papi_event_code = -1;
+ thread->tls_papi_event_code_changed = -1;
+
+ /* Get thread id */
if ( _papi_hwi_thread_id_fn )
tid = ( *_papi_hwi_thread_id_fn ) ( );
else
diff --git a/src/threads.h b/src/threads.h
index cd3369068..264d9f3a6 100644
--- a/src/threads.h
+++ b/src/threads.h
@@ -30,6 +30,11 @@ typedef struct _ThreadInfo
EventSetInfo_t **running_eventset;
EventSetInfo_t *from_esi; /* ESI used for last update this control state */
int wants_signal;
+
+ // The current event code can be stored here prior to
+ // component calls and cleared after the component returns.
+ unsigned int tls_papi_event_code;
+ int tls_papi_event_code_changed;
} ThreadInfo_t;
/** The list of threads, gets initialized to master process with TID of getpid()