port r11138 from 1.1: Do not lie to helgrind about having taken a mutex, it produces erroneous races inside mutexes. Rather make helgrind ignore the precise memory accesses we know we do not need to care about

src/common/utils.h

@@ -30,18 +30,6 @@
 #include <valgrind/helgrind.h>
 #endif
 
-#ifndef VALGRIND_HG_MUTEX_LOCK_PRE
-#define VALGRIND_HG_MUTEX_LOCK_PRE(mutex, istrylock) ((void)0)
-#endif
-#ifndef VALGRIND_HG_MUTEX_LOCK_POST
-#define VALGRIND_HG_MUTEX_LOCK_POST(mutex) ((void)0)
-#endif
-#ifndef VALGRIND_HG_MUTEX_UNLOCK_PRE
-#define VALGRIND_HG_MUTEX_UNLOCK_PRE(mutex) ((void)0)
-#endif
-#ifndef VALGRIND_HG_MUTEX_UNLOCK_POST
-#define VALGRIND_HG_MUTEX_UNLOCK_POST(mutex) ((void)0)
-#endif
 #ifndef DO_CREQ_v_WW
 #define DO_CREQ_v_WW(_creqF, _ty1F, _arg1F, _ty2F, _arg2F) ((void)0)
 #endif
@@ -54,12 +42,14 @@
 #ifndef ANNOTATE_HAPPENS_AFTER
 #define ANNOTATE_HAPPENS_AFTER(obj) ((void)0)
 #endif
-#ifndef ANNOTATE_RWLOCK_ACQUIRED
-#define ANNOTATE_RWLOCK_ACQUIRED(lock, is_w) ((void)0)
+#ifndef VALGRIND_HG_DISABLE_CHECKING
+#define VALGRIND_HG_DISABLE_CHECKING(start, len) ((void)0)
 #endif
-#ifndef ANNOTATE_RWLOCK_RELEASED
-#define ANNOTATE_RWLOCK_RELEASED(lock, is_w) ((void)0)
+#ifndef VALGRIND_HG_ENABLE_CHECKING
+#define VALGRIND_HG_ENABLE_CHECKING(start, len) ((void)0)
 #endif
+#define STARPU_HG_DISABLE_CHECKING(variable) VALGRIND_HG_DISABLE_CHECKING(&(variable), sizeof(variable))
+#define STARPU_HG_ENABLE_CHECKING(variable)  VALGRIND_HG_ENABLE_CHECKING(&(variable), sizeof(variable))
 
 #define _STARPU_VALGRIND_HG_SPIN_LOCK_PRE(lock) \
 	DO_CREQ_v_WW(_VG_USERREQ__HG_PTHREAD_SPIN_LOCK_PRE, \

src/core/dependencies/implicit_data_deps.c

@@ -481,11 +481,12 @@ void _starpu_unlock_post_sync_tasks(starpu_data_handle_t handle)
 	struct _starpu_task_wrapper_list *post_sync_tasks = NULL;
 	unsigned do_submit_tasks = 0;
 
-	VALGRIND_HG_MUTEX_LOCK_PRE(&handle->sequential_consistency_mutex, 0);
-	VALGRIND_HG_MUTEX_LOCK_POST(&handle->sequential_consistency_mutex);
+	/* Tell helgrind we are fine with getting outdated values: count can
+	 * only be zero if we don't have to care about post_sync_tasks_cnt at
+	 * all */
+	STARPU_HG_DISABLE_CHECKING(handle->post_sync_tasks_cnt);
 	unsigned count = handle->post_sync_tasks_cnt;
-	VALGRIND_HG_MUTEX_UNLOCK_PRE(&handle->sequential_consistency_mutex);
-	VALGRIND_HG_MUTEX_UNLOCK_POST(&handle->sequential_consistency_mutex);
+	STARPU_HG_ENABLE_CHECKING(handle->post_sync_tasks_cnt);
 
 	if (count)
 	{

src/core/perfmodel/perfmodel_history.c

@@ -1114,11 +1114,24 @@ double _starpu_non_linear_regression_based_job_expected_perf(struct starpu_perfm
 
 		/* We do not care about racing access to the mean, we only want a
 		 * good-enough estimation, thus simulate taking the rdlock */
-		ANNOTATE_RWLOCK_ACQUIRED(&model->model_rwlock, 0);
 
-		if (entry && entry->history_entry && entry->history_entry->nsample >= _STARPU_CALIBRATION_MINIMUM)
-			exp = entry->history_entry->mean;
-		else if (!model->benchmarking)
+		if (entry) {
+			STARPU_HG_DISABLE_CHECKING(entry->history_entry);
+			if (entry->history_entry) {
+				STARPU_HG_DISABLE_CHECKING(entry->history_entry->nsample);
+				if (entry->history_entry->nsample >= _STARPU_CALIBRATION_MINIMUM)
+				{
+					STARPU_HG_DISABLE_CHECKING(entry->history_entry->mean);
+					exp = entry->history_entry->mean;
+					STARPU_HG_ENABLE_CHECKING(entry->history_entry->mean);
+				}
+				STARPU_HG_ENABLE_CHECKING(entry->history_entry->nsample);
+			}
+			STARPU_HG_ENABLE_CHECKING(entry->history_entry);
+		}
+
+		STARPU_HG_DISABLE_CHECKING(model->benchmarking);
+		if (isnan(exp) && !model->benchmarking)
 		{
 			char archname[32];
 
@@ -1127,7 +1140,7 @@ double _starpu_non_linear_regression_based_job_expected_perf(struct starpu_perfm
 			_starpu_set_calibrate_flag(1);
 			model->benchmarking = 1;
 		}
-		ANNOTATE_RWLOCK_RELEASED(&model->model_rwlock, 0);
+		STARPU_HG_ENABLE_CHECKING(model->benchmarking);
 	}
 
 	return exp;
@@ -1135,7 +1148,7 @@ double _starpu_non_linear_regression_based_job_expected_perf(struct starpu_perfm
 
 double _starpu_history_based_job_expected_perf(struct starpu_perfmodel *model, enum starpu_perfmodel_archtype arch, struct _starpu_job *j,unsigned nimpl)
 {
-	double exp;
+	double exp = NAN;
 	struct starpu_perfmodel_per_arch *per_arch_model;
 	struct starpu_perfmodel_history_entry *entry;
 	struct starpu_perfmodel_history_table *history, *elt;
@@ -1152,16 +1165,22 @@ double _starpu_history_based_job_expected_perf(struct starpu_perfmodel *model, e
 
 	/* We do not care about racing access to the mean, we only want a
 	 * good-enough estimation, thus simulate taking the rdlock */
-	ANNOTATE_RWLOCK_ACQUIRED(&model->model_rwlock, 0);
 
-	exp = entry?entry->mean:NAN;
-
-	if (entry && entry->nsample < _STARPU_CALIBRATION_MINIMUM)
+	if (entry) {
+		STARPU_HG_DISABLE_CHECKING(entry->nsample);
 		/* TODO: report differently if we've scheduled really enough
 		 * of that task and the scheduler should perhaps put it aside */
-		/* Not calibrated enough */
-		exp = NAN;
+		if (entry->nsample >= _STARPU_CALIBRATION_MINIMUM)
+		{
+			/* Calibrated enough */
+			STARPU_HG_DISABLE_CHECKING(entry->mean);
+			exp = entry->mean;
+			STARPU_HG_ENABLE_CHECKING(entry->mean);
+		}
+		STARPU_HG_ENABLE_CHECKING(entry->nsample);
+	}
 
+	STARPU_HG_DISABLE_CHECKING(model->benchmarking);
 	if (isnan(exp) && !model->benchmarking)
 	{
 		char archname[32];
@@ -1171,8 +1190,7 @@ double _starpu_history_based_job_expected_perf(struct starpu_perfmodel *model, e
 		_starpu_set_calibrate_flag(1);
 		model->benchmarking = 1;
 	}
-
-	ANNOTATE_RWLOCK_RELEASED(&model->model_rwlock, 0);
+	STARPU_HG_ENABLE_CHECKING(model->benchmarking);
 
 	return exp;
 }

src/datawizard/data_request.c

@@ -398,17 +398,15 @@ void _starpu_handle_node_data_requests(unsigned src_node, unsigned may_alloc)
 	struct _starpu_data_request_list *new_data_requests;
 
 	/* Note: we here tell valgrind that list_empty (reading a pointer) is
-	 * as safe as if we had the lock held */
-	VALGRIND_HG_MUTEX_LOCK_PRE(&data_requests_list_mutex[src_node], 0);
-	VALGRIND_HG_MUTEX_LOCK_POST(&data_requests_list_mutex[src_node]);
+	 * as safe as if we had the lock held, and we don't care about missing
+	 * an entry, we will get called again sooner or later. */
+	STARPU_HG_DISABLE_CHECKING(*data_requests[src_node]);
 	if (_starpu_data_request_list_empty(data_requests[src_node]))
 	{
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(&data_requests_list_mutex[src_node]);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(&data_requests_list_mutex[src_node]);
+		STARPU_HG_ENABLE_CHECKING(*data_requests[src_node]);
 		return;
 	}
-	VALGRIND_HG_MUTEX_UNLOCK_PRE(&data_requests_list_mutex[src_node]);
-	VALGRIND_HG_MUTEX_UNLOCK_POST(&data_requests_list_mutex[src_node]);
+	STARPU_HG_ENABLE_CHECKING(*data_requests[src_node]);
 
 	/* take all the entries from the request list */
         STARPU_PTHREAD_MUTEX_LOCK(&data_requests_list_mutex[src_node]);

src/sched_policies/eager_central_policy.c

@@ -119,17 +119,14 @@ static struct starpu_task *pop_task_eager_policy(unsigned sched_ctx_id)
 
 	/* Tell helgrind that it's fine to check for empty fifo without actual
 	 * mutex (it's just a pointer) */
-	VALGRIND_HG_MUTEX_LOCK_PRE(&data->policy_mutex, 0);
-	VALGRIND_HG_MUTEX_LOCK_POST(&data->policy_mutex);
+	STARPU_HG_DISABLE_CHECKING(*data->fifo);
 	/* block until some event happens */
 	if (_starpu_fifo_empty(data->fifo))
 	{
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(&data->policy_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(&data->policy_mutex);
+		STARPU_HG_ENABLE_CHECKING(*data->fifo);
 		return NULL;
 	}
-	VALGRIND_HG_MUTEX_UNLOCK_PRE(&data->policy_mutex);
-	VALGRIND_HG_MUTEX_UNLOCK_POST(&data->policy_mutex);
+	STARPU_HG_ENABLE_CHECKING(*data->fifo);
 
 	STARPU_PTHREAD_MUTEX_LOCK(&data->policy_mutex);
 	task = _starpu_fifo_pop_task(data->fifo, workerid);

src/sched_policies/eager_central_priority_policy.c

@@ -156,17 +156,14 @@ static struct starpu_task *_starpu_priority_pop_task(unsigned sched_ctx_id)
 
 	/* Tell helgrind that it's fine to check for empty fifo without actual
 	 * mutex (it's just a pointer) */
-	VALGRIND_HG_MUTEX_LOCK_PRE(&data->policy_mutex, 0);
-	VALGRIND_HG_MUTEX_LOCK_POST(&data->policy_mutex);
+	STARPU_HG_DISABLE_CHECKING(taskq->total_ntasks);
 	/* block until some event happens */
 	if (taskq->total_ntasks == 0)
 	{
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(&data->policy_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(&data->policy_mutex);
+		STARPU_HG_ENABLE_CHECKING(taskq->total_ntasks);
 		return NULL;
 	}
-	VALGRIND_HG_MUTEX_UNLOCK_PRE(&data->policy_mutex);
-	VALGRIND_HG_MUTEX_UNLOCK_POST(&data->policy_mutex);
+	STARPU_HG_ENABLE_CHECKING(taskq->total_ntasks);
 
 	/* release this mutex before trying to wake up other workers */
 	starpu_pthread_mutex_t *curr_sched_mutex;

src/sched_policies/parallel_heft.c

@@ -196,11 +196,12 @@ static double compute_expected_end(int workerid, double length)
 		double res;
 		/* This is a basic worker */
 
-		VALGRIND_HG_MUTEX_LOCK_PRE(sched_mutex, 0);
-		VALGRIND_HG_MUTEX_LOCK_POST(sched_mutex);
+		/* Tell helgrid that we are fine with getting outdated values, this is just an estimation */
+		STARPU_HG_DISABLE_CHECKING(worker_exp_start[workerid]);
+		STARPU_HG_DISABLE_CHECKING(worker_exp_len[workerid]);
 		res = worker_exp_start[workerid] + worker_exp_len[workerid] + length;
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(sched_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(sched_mutex);
+		STARPU_HG_ENABLE_CHECKING(worker_exp_len[workerid]);
+		STARPU_HG_ENABLE_CHECKING(worker_exp_start[workerid]);
 
 		return res;
 	}
@@ -213,8 +214,9 @@ static double compute_expected_end(int workerid, double length)
 
 		double exp_end = DBL_MIN;
 
-		VALGRIND_HG_MUTEX_LOCK_PRE(sched_mutex, 0);
-		VALGRIND_HG_MUTEX_LOCK_POST(sched_mutex);
+		/* Tell helgrid that we are fine with getting outdated values, this is just an estimation */
+		STARPU_HG_DISABLE_CHECKING(worker_exp_start);
+		STARPU_HG_DISABLE_CHECKING(worker_exp_len);
 
 		int i;
 		for (i = 0; i < worker_size; i++)
@@ -225,8 +227,8 @@ static double compute_expected_end(int workerid, double length)
 			exp_end = STARPU_MAX(exp_end, local_exp_end);
 		}
 
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(sched_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(sched_mutex);
+		STARPU_HG_ENABLE_CHECKING(worker_exp_len);
+		STARPU_HG_ENABLE_CHECKING(worker_exp_start);
 
 		return exp_end;
 	}
@@ -245,11 +247,10 @@ static double compute_ntasks_end(int workerid)
 		double res;
 		/* This is a basic worker */
 
-		VALGRIND_HG_MUTEX_LOCK_PRE(sched_mutex, 0);
-		VALGRIND_HG_MUTEX_LOCK_POST(sched_mutex);
+		/* Tell helgrid that we are fine with getting outdated values, this is just an estimation */
+		STARPU_HG_DISABLE_CHECKING(ntasks[workerid]);
 		res = ntasks[workerid] / starpu_worker_get_relative_speedup(perf_arch);
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(sched_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(sched_mutex);
+		STARPU_HG_ENABLE_CHECKING(ntasks[workerid]);
 
 		return res;
 	}
@@ -262,8 +263,8 @@ static double compute_ntasks_end(int workerid)
 
 		int ntasks_end=0;
 
-		VALGRIND_HG_MUTEX_LOCK_PRE(sched_mutex, 0);
-		VALGRIND_HG_MUTEX_LOCK_POST(sched_mutex);
+		/* Tell helgrid that we are fine with getting outdated values, this is just an estimation */
+		STARPU_HG_DISABLE_CHECKING(ntasks);
 
 		int i;
 		for (i = 0; i < worker_size; i++)
@@ -272,8 +273,7 @@ static double compute_ntasks_end(int workerid)
 			ntasks_end = STARPU_MAX(ntasks_end, (int) ((double) ntasks[combined_workerid[i]] / starpu_worker_get_relative_speedup(perf_arch)));
 		}
 
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(sched_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(sched_mutex);
+		STARPU_HG_ENABLE_CHECKING(ntasks);
 
 		return ntasks_end;
 	}

src/sched_policies/work_stealing_policy.c

@@ -73,11 +73,10 @@ static unsigned select_victim_round_robin(unsigned sched_ctx_id)
 		unsigned njobs;
 
 		starpu_worker_get_sched_condition(worker, &victim_sched_mutex, &victim_sched_cond);
-		VALGRIND_HG_MUTEX_LOCK_PRE(victim_sched_mutex, 0);
-		VALGRIND_HG_MUTEX_LOCK_POST(victim_sched_mutex);
+		/* Tell helgrid that we are fine with getting outdated values, this is just an estimation */
+		STARPU_HG_DISABLE_CHECKING(ws->queue_array[worker]->njobs);
 		njobs = ws->queue_array[worker]->njobs;
-		VALGRIND_HG_MUTEX_UNLOCK_PRE(victim_sched_mutex);
-		VALGRIND_HG_MUTEX_UNLOCK_POST(victim_sched_mutex);
+		STARPU_HG_ENABLE_CHECKING(ws->queue_array[worker]->njobs);
 
 		if (njobs)
 			break;