Browse Source

backport r11155 from 1.1: Disabling access to a variable locally does not actually seem to prevent helgrind from raising the race issues, on the contrary it prevents it from recording where the race comes from... Simply just disable checking these variables completely.

Samuel Thibault 11 years ago
parent
commit
0d1b0375b3

+ 3 - 5
src/core/dependencies/implicit_data_deps.c

@@ -481,12 +481,10 @@ void _starpu_unlock_post_sync_tasks(starpu_data_handle_t handle)
 	struct _starpu_task_wrapper_list *post_sync_tasks = NULL;
 	struct _starpu_task_wrapper_list *post_sync_tasks = NULL;
 	unsigned do_submit_tasks = 0;
 	unsigned do_submit_tasks = 0;
 
 
-	/* 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);
+	/* Here helgrind would shout that this is an unprotected access, but
+	 * count can only be zero if we don't have to care about
+	 * post_sync_tasks_cnt at all.  */
 	unsigned count = handle->post_sync_tasks_cnt;
 	unsigned count = handle->post_sync_tasks_cnt;
-	STARPU_HG_ENABLE_CHECKING(handle->post_sync_tasks_cnt);
 
 
 	if (count)
 	if (count)
 	{
 	{

+ 13 - 28
src/core/perfmodel/perfmodel_history.c

@@ -1113,22 +1113,10 @@ double _starpu_non_linear_regression_based_job_expected_perf(struct starpu_perfm
 		STARPU_PTHREAD_RWLOCK_UNLOCK(&model->model_rwlock);
 		STARPU_PTHREAD_RWLOCK_UNLOCK(&model->model_rwlock);
 
 
 		/* We do not care about racing access to the mean, we only want a
 		/* We do not care about racing access to the mean, we only want a
-		 * good-enough estimation, thus simulate taking the rdlock */
+		 * good-enough estimation */
 
 
-		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);
-		}
+		if (entry && entry->history_entry && entry->history_entry->nsample >= _STARPU_CALIBRATION_MINIMUM)
+			exp = entry->history_entry->mean;
 
 
 		STARPU_HG_DISABLE_CHECKING(model->benchmarking);
 		STARPU_HG_DISABLE_CHECKING(model->benchmarking);
 		if (isnan(exp) && !model->benchmarking)
 		if (isnan(exp) && !model->benchmarking)
@@ -1140,7 +1128,6 @@ double _starpu_non_linear_regression_based_job_expected_perf(struct starpu_perfm
 			_starpu_set_calibrate_flag(1);
 			_starpu_set_calibrate_flag(1);
 			model->benchmarking = 1;
 			model->benchmarking = 1;
 		}
 		}
-		STARPU_HG_ENABLE_CHECKING(model->benchmarking);
 	}
 	}
 
 
 	return exp;
 	return exp;
@@ -1166,19 +1153,11 @@ 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
 	/* We do not care about racing access to the mean, we only want a
 	 * good-enough estimation, thus simulate taking the rdlock */
 	 * good-enough estimation, thus simulate taking the rdlock */
 
 
-	if (entry) {
-		STARPU_HG_DISABLE_CHECKING(entry->nsample);
+	if (entry && entry->nsample >= _STARPU_CALIBRATION_MINIMUM)
 		/* TODO: report differently if we've scheduled really enough
 		/* TODO: report differently if we've scheduled really enough
 		 * of that task and the scheduler should perhaps put it aside */
 		 * of that task and the scheduler should perhaps put it aside */
-		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);
-	}
+		/* Calibrated enough */
+		exp = entry->mean;
 
 
 	STARPU_HG_DISABLE_CHECKING(model->benchmarking);
 	STARPU_HG_DISABLE_CHECKING(model->benchmarking);
 	if (isnan(exp) && !model->benchmarking)
 	if (isnan(exp) && !model->benchmarking)
@@ -1190,7 +1169,6 @@ double _starpu_history_based_job_expected_perf(struct starpu_perfmodel *model, e
 		_starpu_set_calibrate_flag(1);
 		_starpu_set_calibrate_flag(1);
 		model->benchmarking = 1;
 		model->benchmarking = 1;
 	}
 	}
-	STARPU_HG_ENABLE_CHECKING(model->benchmarking);
 
 
 	return exp;
 	return exp;
 }
 }
@@ -1230,6 +1208,13 @@ void _starpu_update_perfmodel_history(struct _starpu_job *j, struct starpu_perfm
 				/* this is the first entry with such a footprint */
 				/* this is the first entry with such a footprint */
 				entry = (struct starpu_perfmodel_history_entry *) malloc(sizeof(struct starpu_perfmodel_history_entry));
 				entry = (struct starpu_perfmodel_history_entry *) malloc(sizeof(struct starpu_perfmodel_history_entry));
 				STARPU_ASSERT(entry);
 				STARPU_ASSERT(entry);
+
+				/* Tell  helgrind that we do not care about
+				 * racing access to the sampling, we only want a
+				 * good-enough estimation */
+				STARPU_HG_DISABLE_CHECKING(entry->nsample);
+				STARPU_HG_DISABLE_CHECKING(entry->mean);
+
 				entry->mean = measured;
 				entry->mean = measured;
 				entry->sum = measured;
 				entry->sum = measured;
 
 

+ 6 - 8
src/datawizard/data_request.c

@@ -36,6 +36,12 @@ void _starpu_init_data_request_lists(void)
 	{
 	{
 		prefetch_requests[i] = _starpu_data_request_list_new();
 		prefetch_requests[i] = _starpu_data_request_list_new();
 		data_requests[i] = _starpu_data_request_list_new();
 		data_requests[i] = _starpu_data_request_list_new();
+
+		/* Tell helgrind that we are fine with checking for list_empty
+		 * in _starpu_handle_node_data_requests, we will call it
+		 * periodically anyway */
+		STARPU_HG_DISABLE_CHECKING(data_requests[i]->_head);
+
 		STARPU_PTHREAD_MUTEX_INIT(&data_requests_list_mutex[i], NULL);
 		STARPU_PTHREAD_MUTEX_INIT(&data_requests_list_mutex[i], NULL);
 
 
 		data_requests_pending[i] = _starpu_data_request_list_new();
 		data_requests_pending[i] = _starpu_data_request_list_new();
@@ -397,16 +403,8 @@ void _starpu_handle_node_data_requests(unsigned src_node, unsigned may_alloc)
 	struct _starpu_data_request *r;
 	struct _starpu_data_request *r;
 	struct _starpu_data_request_list *new_data_requests;
 	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, 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]))
 	if (_starpu_data_request_list_empty(data_requests[src_node]))
-	{
-		STARPU_HG_ENABLE_CHECKING(*data_requests[src_node]);
 		return;
 		return;
-	}
-	STARPU_HG_ENABLE_CHECKING(*data_requests[src_node]);
 
 
 	/* take all the entries from the request list */
 	/* take all the entries from the request list */
         STARPU_PTHREAD_MUTEX_LOCK(&data_requests_list_mutex[src_node]);
         STARPU_PTHREAD_MUTEX_LOCK(&data_requests_list_mutex[src_node]);

+ 2 - 0
src/datawizard/filters.c

@@ -191,6 +191,8 @@ void starpu_data_partition(starpu_data_handle_t initial_handle, struct starpu_da
 		child->last_sync_task = NULL;
 		child->last_sync_task = NULL;
 		child->last_submitted_accessors = NULL;
 		child->last_submitted_accessors = NULL;
 		child->post_sync_tasks = NULL;
 		child->post_sync_tasks = NULL;
+		/* Tell helgrind that the race in _starpu_unlock_post_sync_tasks is fine */
+		STARPU_HG_DISABLE_CHECKING(child->post_sync_tasks_cnt);
 		child->post_sync_tasks_cnt = 0;
 		child->post_sync_tasks_cnt = 0;
 
 
 		/* The methods used for reduction are propagated to the
 		/* The methods used for reduction are propagated to the

+ 3 - 0
src/datawizard/interfaces/data_interface.c

@@ -200,6 +200,9 @@ static void _starpu_register_new_data(starpu_data_handle_t handle,
 	handle->last_sync_task = NULL;
 	handle->last_sync_task = NULL;
 	handle->last_submitted_accessors = NULL;
 	handle->last_submitted_accessors = NULL;
 	handle->post_sync_tasks = NULL;
 	handle->post_sync_tasks = NULL;
+
+	/* Tell helgrind that the race in _starpu_unlock_post_sync_tasks is fine */
+	STARPU_HG_DISABLE_CHECKING(handle->post_sync_tasks_cnt);
 	handle->post_sync_tasks_cnt = 0;
 	handle->post_sync_tasks_cnt = 0;
 
 
 	/* By default, there are no methods available to perform a reduction */
 	/* By default, there are no methods available to perform a reduction */

+ 5 - 7
src/sched_policies/eager_central_policy.c

@@ -42,6 +42,11 @@ static void initialize_eager_center_policy(unsigned sched_ctx_id)
 	/* there is only a single queue in that trivial design */
 	/* there is only a single queue in that trivial design */
 	data->fifo =  _starpu_create_fifo();
 	data->fifo =  _starpu_create_fifo();
 
 
+	 /* Tell helgrind that it's fine to check for empty fifo in
+	  * pop_task_eager_policy without actual mutex (it's just an integer)
+	  */
+	STARPU_HG_DISABLE_CHECKING(data->fifo->ntasks);
+
 	starpu_sched_ctx_set_policy_data(sched_ctx_id, (void*)data);
 	starpu_sched_ctx_set_policy_data(sched_ctx_id, (void*)data);
 	STARPU_PTHREAD_MUTEX_INIT(&data->policy_mutex, NULL);
 	STARPU_PTHREAD_MUTEX_INIT(&data->policy_mutex, NULL);
 }
 }
@@ -117,16 +122,9 @@ static struct starpu_task *pop_task_eager_policy(unsigned sched_ctx_id)
 
 
 	struct starpu_task *task = NULL;
 	struct starpu_task *task = NULL;
 
 
-	/* Tell helgrind that it's fine to check for empty fifo without actual
-	 * mutex (it's just a pointer) */
-	STARPU_HG_DISABLE_CHECKING(*data->fifo);
 	/* block until some event happens */
 	/* block until some event happens */
 	if (_starpu_fifo_empty(data->fifo))
 	if (_starpu_fifo_empty(data->fifo))
-	{
-		STARPU_HG_ENABLE_CHECKING(*data->fifo);
 		return NULL;
 		return NULL;
-	}
-	STARPU_HG_ENABLE_CHECKING(*data->fifo);
 
 
 	STARPU_PTHREAD_MUTEX_LOCK(&data->policy_mutex);
 	STARPU_PTHREAD_MUTEX_LOCK(&data->policy_mutex);
 	task = _starpu_fifo_pop_task(data->fifo, workerid);
 	task = _starpu_fifo_pop_task(data->fifo, workerid);

+ 5 - 7
src/sched_policies/eager_central_priority_policy.c

@@ -84,6 +84,11 @@ static void initialize_eager_center_priority_policy(unsigned sched_ctx_id)
 
 
 	/* only a single queue (even though there are several internaly) */
 	/* only a single queue (even though there are several internaly) */
 	data->taskq = _starpu_create_priority_taskq();
 	data->taskq = _starpu_create_priority_taskq();
+
+	/* Tell helgrind that it's fine to check for empty fifo in
+	 * _starpu_priority_pop_task without actual mutex (it's just an
+	 * integer) */
+	STARPU_HG_DISABLE_CHECKING(data->taskq->total_ntasks);
 	starpu_sched_ctx_set_policy_data(sched_ctx_id, (void*)data);
 	starpu_sched_ctx_set_policy_data(sched_ctx_id, (void*)data);
 	STARPU_PTHREAD_MUTEX_INIT(&data->policy_mutex, NULL);
 	STARPU_PTHREAD_MUTEX_INIT(&data->policy_mutex, NULL);
 
 
@@ -154,16 +159,9 @@ static struct starpu_task *_starpu_priority_pop_task(unsigned sched_ctx_id)
 
 
 	struct _starpu_priority_taskq *taskq = data->taskq;
 	struct _starpu_priority_taskq *taskq = data->taskq;
 
 
-	/* Tell helgrind that it's fine to check for empty fifo without actual
-	 * mutex (it's just a pointer) */
-	STARPU_HG_DISABLE_CHECKING(taskq->total_ntasks);
 	/* block until some event happens */
 	/* block until some event happens */
 	if (taskq->total_ntasks == 0)
 	if (taskq->total_ntasks == 0)
-	{
-		STARPU_HG_ENABLE_CHECKING(taskq->total_ntasks);
 		return NULL;
 		return NULL;
-	}
-	STARPU_HG_ENABLE_CHECKING(taskq->total_ntasks);
 
 
 	/* release this mutex before trying to wake up other workers */
 	/* release this mutex before trying to wake up other workers */
 	starpu_pthread_mutex_t *curr_sched_mutex;
 	starpu_pthread_mutex_t *curr_sched_mutex;

+ 5 - 20
src/sched_policies/parallel_heft.c

@@ -196,12 +196,7 @@ static double compute_expected_end(int workerid, double length)
 		double res;
 		double res;
 		/* This is a basic worker */
 		/* This is a basic worker */
 
 
-		/* 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;
 		res = worker_exp_start[workerid] + worker_exp_len[workerid] + length;
-		STARPU_HG_ENABLE_CHECKING(worker_exp_len[workerid]);
-		STARPU_HG_ENABLE_CHECKING(worker_exp_start[workerid]);
 
 
 		return res;
 		return res;
 	}
 	}
@@ -214,10 +209,6 @@ static double compute_expected_end(int workerid, double length)
 
 
 		double exp_end = DBL_MIN;
 		double exp_end = DBL_MIN;
 
 
-		/* 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;
 		int i;
 		for (i = 0; i < worker_size; i++)
 		for (i = 0; i < worker_size; i++)
 		{
 		{
@@ -227,9 +218,6 @@ static double compute_expected_end(int workerid, double length)
 			exp_end = STARPU_MAX(exp_end, local_exp_end);
 			exp_end = STARPU_MAX(exp_end, local_exp_end);
 		}
 		}
 
 
-		STARPU_HG_ENABLE_CHECKING(worker_exp_len);
-		STARPU_HG_ENABLE_CHECKING(worker_exp_start);
-
 		return exp_end;
 		return exp_end;
 	}
 	}
 }
 }
@@ -247,10 +235,7 @@ static double compute_ntasks_end(int workerid)
 		double res;
 		double res;
 		/* This is a basic worker */
 		/* This is a basic worker */
 
 
-		/* 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);
 		res = ntasks[workerid] / starpu_worker_get_relative_speedup(perf_arch);
-		STARPU_HG_ENABLE_CHECKING(ntasks[workerid]);
 
 
 		return res;
 		return res;
 	}
 	}
@@ -263,9 +248,6 @@ static double compute_ntasks_end(int workerid)
 
 
 		int ntasks_end=0;
 		int ntasks_end=0;
 
 
-		/* Tell helgrid that we are fine with getting outdated values, this is just an estimation */
-		STARPU_HG_DISABLE_CHECKING(ntasks);
-
 		int i;
 		int i;
 		for (i = 0; i < worker_size; i++)
 		for (i = 0; i < worker_size; i++)
 		{
 		{
@@ -273,8 +255,6 @@ 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)));
 			ntasks_end = STARPU_MAX(ntasks_end, (int) ((double) ntasks[combined_workerid[i]] / starpu_worker_get_relative_speedup(perf_arch)));
 		}
 		}
 
 
-		STARPU_HG_ENABLE_CHECKING(ntasks);
-
 		return ntasks_end;
 		return ntasks_end;
 	}
 	}
 }
 }
@@ -580,6 +560,11 @@ static void initialize_parallel_heft_policy(unsigned sched_ctx_id)
 
 
 	STARPU_PTHREAD_MUTEX_INIT(&hd->global_push_mutex, NULL);
 	STARPU_PTHREAD_MUTEX_INIT(&hd->global_push_mutex, NULL);
 
 
+	/* Tell helgrind that we are fine with getting outdated values when
+	 * estimating schedules */
+	STARPU_HG_DISABLE_CHECKING(worker_exp_start);
+	STARPU_HG_DISABLE_CHECKING(worker_exp_len);
+	STARPU_HG_DISABLE_CHECKING(ntasks);
 }
 }
 
 
 static void parallel_heft_deinit(unsigned sched_ctx_id)
 static void parallel_heft_deinit(unsigned sched_ctx_id)

+ 5 - 3
src/sched_policies/work_stealing_policy.c

@@ -73,10 +73,7 @@ static unsigned select_victim_round_robin(unsigned sched_ctx_id)
 		unsigned njobs;
 		unsigned njobs;
 
 
 		starpu_worker_get_sched_condition(worker, &victim_sched_mutex, &victim_sched_cond);
 		starpu_worker_get_sched_condition(worker, &victim_sched_mutex, &victim_sched_cond);
-		/* 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;
 		njobs = ws->queue_array[worker]->njobs;
-		STARPU_HG_ENABLE_CHECKING(ws->queue_array[worker]->njobs);
 
 
 		if (njobs)
 		if (njobs)
 			break;
 			break;
@@ -401,6 +398,11 @@ static void ws_add_workers(unsigned sched_ctx_id, int *workerids,unsigned nworke
 		workerid = workerids[i];
 		workerid = workerids[i];
 		starpu_sched_ctx_worker_shares_tasks_lists(workerid, sched_ctx_id);
 		starpu_sched_ctx_worker_shares_tasks_lists(workerid, sched_ctx_id);
 		ws->queue_array[workerid] = _starpu_create_deque();
 		ws->queue_array[workerid] = _starpu_create_deque();
+
+		/* Tell helgrid that we are fine with getting outdated values,
+		 * this is just an estimation */
+		STARPU_HG_DISABLE_CHECKING(ws->queue_array[workerid]->njobs);
+
 		/**
 		/**
 		 * The first WS_POP_TASK will increase NPROCESSED though no task was actually performed yet,
 		 * The first WS_POP_TASK will increase NPROCESSED though no task was actually performed yet,
 		 * we need to initialize it at -1.
 		 * we need to initialize it at -1.