api: timer: only inactive timers can be freed

Remove possibility to free a timer that is running. When timer
free call returns, the timer handle cannot be referenced any
more. Expiration and event delivery of an already destroyed
timer is an error prone corner case.

Signed-off-by: Petri Savolainen <petri.savolainen@nokia.com>
Reviewed-by: Jere Leppänen <jere.leppanen@nokia.com>
Acked-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

9 files changed, 109 insertions, 80 deletions

diff --git a/example/debug/odp_debug.c b/example/debug/odp_debug.c
index 06d10f34d..cca6fa939 100644
--- a/example/debug/odp_debug.c
+++ b/example/debug/odp_debug.c
@@ -384,6 +384,7 @@ static int timer_debug(void)
 	uint64_t tick;
 	uint64_t max_tmo = ODP_TIME_SEC_IN_NS;
 	uint64_t res     = 100 * ODP_TIME_MSEC_IN_NS;
+	int started = 0;
 
 	odp_pool_param_init(&pool_param);
 	pool_param.type = ODP_POOL_TIMEOUT;
@@ -462,16 +463,17 @@ static int timer_debug(void)
 	start_param.tick = tick;
 	start_param.tmo_ev = event;
 
-	if (odp_timer_start(timer, &start_param) != ODP_TIMER_SUCCESS)
+	if (odp_timer_start(timer, &start_param) == ODP_TIMER_SUCCESS)
+		started = 1;
+	else
 		ODPH_ERR("Timer start failed.\n");
 
 	printf("\n");
 	odp_timer_print(timer);
 
-	event = odp_timer_free(timer);
-
-	if (event == ODP_EVENT_INVALID) {
-		ODPH_ERR("Timer free failed.\n");
+	if (started && odp_timer_cancel(timer, &event) != ODP_TIMER_SUCCESS) {
+		ODPH_ERR("Timer cancel failed\n");
+		return -1;
 	} else {
 		timeout = odp_timeout_from_event(event);
 
@@ -481,6 +483,11 @@ static int timer_debug(void)
 		odp_timeout_free(timeout);
 	}
 
+	if (odp_timer_free(timer)) {
+		ODPH_ERR("Timer free failed\n");
+		return -1;
+	}
+
 	odp_timer_pool_destroy(timer_pool);
 
 	if (odp_queue_destroy(queue)) {
diff --git a/example/timer/odp_timer_accuracy.c b/example/timer/odp_timer_accuracy.c
index 05e4e9181..83ca371d9 100644
--- a/example/timer/odp_timer_accuracy.c
+++ b/example/timer/odp_timer_accuracy.c
@@ -806,7 +806,6 @@ static int destroy_timers(test_global_t *test_global)
 {
 	uint64_t i, alloc_timers;
 	odp_timer_t timer;
-	odp_event_t ev;
 	int ret = 0;
 
 	alloc_timers = test_global->opt.alloc_timers;
@@ -817,10 +816,10 @@ static int destroy_timers(test_global_t *test_global)
 		if (timer == ODP_TIMER_INVALID)
 			break;
 
-		ev = odp_timer_free(timer);
-
-		if (ev != ODP_EVENT_INVALID)
-			odp_event_free(ev);
+		if (odp_timer_free(timer)) {
+			printf("Timer free failed: %" PRIu64 "\n", i);
+			ret = -1;
+		}
 	}
 
 	if (test_global->timer_pool != ODP_TIMER_POOL_INVALID)
diff --git a/example/timer/odp_timer_simple.c b/example/timer/odp_timer_simple.c
index fdf38c9d3..b1c818c76 100644
--- a/example/timer/odp_timer_simple.c
+++ b/example/timer/odp_timer_simple.c
@@ -159,18 +159,24 @@ int main(int argc ODP_UNUSED, char *argv[] ODP_UNUSED)
 	}
 
 	/* Destroy created resources */
-	rc += odp_timer_cancel(tim, &ev);
-	rc += -(odp_timer_free(tim) == ODP_EVENT_INVALID);
 	odp_event_free(ev);
 
-	ret += odp_queue_destroy(queue);
+	if (odp_timer_free(tim))
+		ret++;
+
+	if (odp_queue_destroy(queue))
+		ret++;
 err:
 	odp_timer_pool_destroy(timer_pool);
 err_tp:
-	ret += odp_pool_destroy(timeout_pool);
-	ret += odp_term_local();
+	if (odp_pool_destroy(timeout_pool))
+		ret++;
+
+	if (odp_term_local())
+		ret++;
 err_local:
-	ret += odp_term_global(instance);
+	if (odp_term_global(instance))
+		ret++;
 err_global:
 	return ret;
 }
diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h
index ad27b5ded..e8625236e 100644
--- a/include/odp/api/spec/timer.h
+++ b/include/odp/api/spec/timer.h
@@ -246,19 +246,21 @@ odp_timer_t odp_timer_alloc(odp_timer_pool_t timer_pool, odp_queue_t queue, cons
 /**
  * Free a timer
  *
- * Free (destroy) a timer, reclaiming associated resources.
- * The timeout event for an active timer will be returned.
- * The timeout event for an expired timer will not be returned. It is the
- * responsibility of the application to handle this timeout when it is received.
+ * Frees a previously allocated timer. The timer must be inactive when calling this function.
+ * In other words, the application must cancel an active single shot timer (odp_timer_cancel())
+ * successfully or wait it to expire before freeing it. Similarly for an active periodic timer, the
+ * application must cancel it (odp_timer_periodic_cancel()) and receive the last event from
+ * the timer (odp_timer_periodic_ack()) before freeing it.
  *
- * A periodic timer must be cancelled successfully before freeing it.
+ * The call returns failure only on non-recoverable errors. Application must not use the timer
+ * handle anymore after the call, regardless of the return value.
  *
- * @param timer      Timer
+ * @param timer       Timer
  *
- * @return Event handle of timeout event
- * @retval ODP_EVENT_INVALID on failure
+ * @retval 0 on success
+ * @retval <0 on failure
  */
-odp_event_t odp_timer_free(odp_timer_t timer);
+int odp_timer_free(odp_timer_t timer);
 
 /**
  * Start a timer
diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c
index ccc716652..9fc247862 100644
--- a/platform/linux-generic/odp_timer.c
+++ b/platform/linux-generic/odp_timer.c
@@ -579,14 +579,20 @@ static odp_event_t timer_set_unused(timer_pool_t *tp, uint32_t idx)
 	return old_event;
 }
 
-static inline odp_event_t timer_free(timer_pool_t *tp, uint32_t idx)
+int odp_timer_free(odp_timer_t hdl)
 {
+	timer_pool_t *tp = handle_to_tp(hdl);
+	uint32_t idx = handle_to_idx(hdl, tp);
 	_odp_timer_t *tim = &tp->timers[idx];
 	tick_buf_t *tb = &tp->tick_buf[idx];
 
 	/* Free the timer by setting timer state to unused and
 	 * grab any timeout event */
 	odp_event_t old_event = timer_set_unused(tp, idx);
+	if (old_event != ODP_EVENT_INVALID) {
+		_ODP_ERR("Timer is active\n");
+		return -1;
+	}
 
 	/* Remove timer from queue */
 	_odp_queue_fn->timer_rem(tim->queue);
@@ -605,7 +611,7 @@ static inline odp_event_t timer_free(timer_pool_t *tp, uint32_t idx)
 	tp->num_alloc--;
 	odp_spinlock_unlock(&tp->lock);
 
-	return old_event;
+	return 0;
 }
 
 static odp_event_t timer_cancel(timer_pool_t *tp, uint32_t idx)
@@ -1528,14 +1534,6 @@ odp_timer_t odp_timer_alloc(odp_timer_pool_t tpid, odp_queue_t queue, const void
 	return timer_alloc(tp, queue, user_ptr);
 }
 
-odp_event_t odp_timer_free(odp_timer_t hdl)
-{
-	timer_pool_t *tp = handle_to_tp(hdl);
-	uint32_t idx = handle_to_idx(hdl, tp);
-
-	return timer_free(tp, idx);
-}
-
 int odp_timer_start(odp_timer_t timer, const odp_timer_start_t *start_param)
 {
 	uint64_t abs_tick, rel_tick;
diff --git a/test/performance/odp_sched_pktio.c b/test/performance/odp_sched_pktio.c
index 927d35cbd..ceaef0898 100644
--- a/test/performance/odp_sched_pktio.c
+++ b/test/performance/odp_sched_pktio.c
@@ -1081,10 +1081,10 @@ static int stop_pktios(test_global_t *test_global)
 	return ret;
 }
 
-static void empty_queues(void)
+static void empty_queues(uint64_t wait_ns)
 {
 	odp_event_t ev;
-	uint64_t wait_time = odp_schedule_wait_time(ODP_TIME_SEC_IN_NS / 2);
+	uint64_t wait_time = odp_schedule_wait_time(wait_ns);
 
 	/* Drop all events from all queues */
 	while (1) {
@@ -1365,7 +1365,6 @@ static int start_timers(test_global_t *test_global)
 static void destroy_timers(test_global_t *test_global)
 {
 	int i, j;
-	odp_event_t event;
 	odp_timer_t timer;
 	int num_pktio = test_global->opt.num_pktio;
 	int num_queue = test_global->opt.num_pktio_queue;
@@ -1375,6 +1374,9 @@ static void destroy_timers(test_global_t *test_global)
 	if (timer_pool == ODP_TIMER_POOL_INVALID)
 		return;
 
+	/* Wait any remaining timers to expire */
+	empty_queues(2000 * test_global->opt.timeout_us);
+
 	for (i = 0; i < num_pktio; i++) {
 		for (j = 0; j < num_queue; j++) {
 			timer = test_global->timer.timer[i][j];
@@ -1382,10 +1384,8 @@ static void destroy_timers(test_global_t *test_global)
 			if (timer == ODP_TIMER_INVALID)
 				break;
 
-			event = odp_timer_free(timer);
-
-			if (event != ODP_EVENT_INVALID)
-				odp_event_free(event);
+			if (odp_timer_free(timer))
+				printf("Timer free failed: %i, %i\n", i, j);
 		}
 	}
 
@@ -1552,7 +1552,7 @@ int main(int argc, char *argv[])
 
 quit:
 	stop_pktios(test_global);
-	empty_queues();
+	empty_queues(ODP_TIME_SEC_IN_NS / 2);
 	close_pktios(test_global);
 	destroy_pipeline_queues(test_global);
 	destroy_timers(test_global);
diff --git a/test/performance/odp_stress.c b/test/performance/odp_stress.c
index d5e3142f6..f18d85c89 100644
--- a/test/performance/odp_stress.c
+++ b/test/performance/odp_stress.c
@@ -61,6 +61,7 @@ typedef struct test_global_t {
 	test_stat_t stat[ODP_THREAD_COUNT_MAX];
 	thread_arg_t thread_arg[ODP_THREAD_COUNT_MAX];
 	test_stat_sum_t stat_sum;
+	odp_atomic_u64_t tot_rounds;
 
 } test_global_t;
 
@@ -219,19 +220,21 @@ static int worker_thread(void *arg)
 	test_global_t *global = thread_arg->global;
 	test_options_t *test_options = &global->test_options;
 	int mode = test_options->mode;
+	int group_mode = test_options->group_mode;
 	uint64_t mem_size = test_options->mem_size;
 	uint64_t copy_size = mem_size / 2;
 	uint64_t rounds = 0;
 	int ret = 0;
 	uint32_t done = 0;
 	uint64_t wait = ODP_SCHED_WAIT;
+	uint64_t tot_rounds = test_options->rounds * test_options->num_cpu;
 
 	thr = odp_thread_id();
 	max_nsec = 2 * test_options->rounds * test_options->period_ns;
 	max_time = odp_time_local_from_ns(max_nsec);
 	printf("Thread %i starting on CPU %i\n", thr, odp_cpu_id());
 
-	if (test_options->group_mode == 0) {
+	if (group_mode == 0) {
 		/* Timeout events are load balanced. Using this
 		 * period to poll exit status. */
 		wait = odp_schedule_wait_time(100 * ODP_TIME_MSEC_IN_NS);
@@ -280,7 +283,15 @@ static int worker_thread(void *arg)
 
 		rounds++;
 
-		if (rounds < test_options->rounds) {
+		if (group_mode) {
+			if (rounds >= test_options->rounds)
+				done = 1;
+		} else {
+			if (odp_atomic_fetch_inc_u64(&global->tot_rounds) >= (tot_rounds - 1))
+				done = 1;
+		}
+
+		if (done == 0) {
 			tmo = odp_timeout_from_event(ev);
 			timer = odp_timeout_timer(tmo);
 			start_param.tmo_ev = ev;
@@ -291,8 +302,6 @@ static int worker_thread(void *arg)
 				ODPH_ERR("Timer start failed (%" PRIu64 ")\n", rounds);
 				done = 1;
 			}
-		} else {
-			done = 1;
 		}
 
 		/* Do work */
@@ -530,7 +539,6 @@ static int start_timers(test_global_t *global)
 static void destroy_timers(test_global_t *global)
 {
 	uint32_t i;
-	odp_event_t ev;
 	test_options_t *test_options = &global->test_options;
 	uint32_t num_cpu = test_options->num_cpu;
 
@@ -540,9 +548,8 @@ static void destroy_timers(test_global_t *global)
 		if (timer == ODP_TIMER_INVALID)
 			continue;
 
-		ev = odp_timer_free(timer);
-		if (ev != ODP_EVENT_INVALID)
-			odp_event_free(ev);
+		if (odp_timer_free(timer))
+			ODPH_ERR("Timer free failed (%u)\n", i);
 	}
 
 	if (global->timer_pool != ODP_TIMER_POOL_INVALID)
@@ -744,6 +751,7 @@ int main(int argc, char **argv)
 
 	memset(global, 0, sizeof(test_global_t));
 	odp_atomic_init_u32(&global->exit_test, 0);
+	odp_atomic_init_u64(&global->tot_rounds, 0);
 
 	global->timer_pool = ODP_TIMER_POOL_INVALID;
 	global->tmo_pool   = ODP_POOL_INVALID;
diff --git a/test/performance/odp_timer_perf.c b/test/performance/odp_timer_perf.c
index 5abee1988..d5b2a6e03 100644
--- a/test/performance/odp_timer_perf.c
+++ b/test/performance/odp_timer_perf.c
@@ -514,7 +514,6 @@ static int destroy_timer_pool(test_global_t *global)
 	odp_pool_t pool;
 	odp_queue_t queue;
 	odp_timer_t timer;
-	odp_event_t ev;
 	uint32_t i, j;
 	test_options_t *test_options = &global->test_options;
 	uint32_t num_timer = test_options->num_timer;
@@ -527,13 +526,8 @@ static int destroy_timer_pool(test_global_t *global)
 			if (timer == ODP_TIMER_INVALID)
 				break;
 
-			ev = odp_timer_free(timer);
-
-			if (ev != ODP_EVENT_INVALID) {
-				if (test_options->mode == MODE_SCHED_OVERH)
-					printf("Event from timer free %i/%i\n", i, j);
-				odp_event_free(ev);
-			}
+			if (odp_timer_free(timer))
+				printf("Timer free failed: %i/%i\n", i, j);
 		}
 
 		queue = global->queue[i];
@@ -649,15 +643,17 @@ static int sched_mode_worker(void *arg)
 	return ret;
 }
 
-static void cancel_timers(test_global_t *global, uint32_t worker_idx)
+static int cancel_timers(test_global_t *global, uint32_t worker_idx)
 {
 	uint32_t i, j;
+	int r;
 	odp_timer_t timer;
 	odp_event_t ev;
 	test_options_t *test_options = &global->test_options;
 	uint32_t num_tp = test_options->num_tp;
 	uint32_t num_timer = test_options->num_timer;
 	uint32_t num_worker = test_options->num_cpu;
+	int ret = 0;
 
 	for (i = 0; i < num_tp; i++) {
 		for (j = 0; j < num_timer; j++) {
@@ -668,10 +664,20 @@ static void cancel_timers(test_global_t *global, uint32_t worker_idx)
 			if (timer == ODP_TIMER_INVALID)
 				continue;
 
-			if (odp_timer_cancel(timer, &ev) == ODP_TIMER_SUCCESS)
+			r = odp_timer_cancel(timer, &ev);
+
+			if (r == ODP_TIMER_SUCCESS) {
 				odp_event_free(ev);
+			} else if (r == ODP_TIMER_TOO_NEAR) {
+				ret = 1;
+			} else {
+				ret = -1;
+				break;
+			}
 		}
 	}
+
+	return ret;
 }
 
 static int set_cancel_mode_worker(void *arg)
@@ -815,7 +821,8 @@ static int set_cancel_mode_worker(void *arg)
 	diff = odp_cpu_cycles_diff(c2, c1);
 
 	/* Cancel all timers that belong to this thread */
-	cancel_timers(global, worker_idx);
+	if (cancel_timers(global, worker_idx))
+		ODPH_ERR("Timer cancel failed\n");
 
 	/* Update stats */
 	global->stat[thr].events = num_tmo;
@@ -835,7 +842,7 @@ static int set_expire_mode_worker(void *arg)
 	uint32_t i, j, exit_test;
 	odp_event_t ev;
 	odp_timeout_t tmo;
-	uint64_t c2, c3, c4, diff, nsec, time_ns, target_ns, period_tick;
+	uint64_t c2, c3, c4, diff, nsec, time_ns, target_ns, period_tick, wait;
 	odp_timer_t timer;
 	odp_timer_start_t start_param;
 	odp_time_t t1, t2;
@@ -937,11 +944,15 @@ static int set_expire_mode_worker(void *arg)
 	nsec = odp_time_diff_ns(t2, t1);
 
 	/* Cancel all timers that belong to this thread */
-	cancel_timers(global, thread_arg->worker_idx);
+	status = cancel_timers(global, thread_arg->worker_idx);
+
+	wait = ODP_SCHED_NO_WAIT;
+	if (status > 0)
+		wait = odp_schedule_wait_time(opt->period_ns);
 
-	/* Free already scheduled events */
+	/* Wait and free remaining events */
 	while (1) {
-		ev = odp_schedule(NULL, ODP_SCHED_NO_WAIT);
+		ev = odp_schedule(NULL, wait);
 		if (ev == ODP_EVENT_INVALID)
 			break;
 		odp_event_free(ev);
diff --git a/test/validation/api/timer/timer.c b/test/validation/api/timer/timer.c
index 5436b9b42..5fd5948b5 100644
--- a/test/validation/api/timer/timer.c
+++ b/test/validation/api/timer/timer.c
@@ -707,7 +707,7 @@ static void timer_pool_create_destroy(void)
 
 	tim = odp_timer_alloc(tp[0], queue, USER_PTR);
 	CU_ASSERT(tim != ODP_TIMER_INVALID);
-	CU_ASSERT(odp_timer_free(tim) == ODP_EVENT_INVALID);
+	CU_ASSERT(odp_timer_free(tim) == 0);
 
 	odp_timer_pool_destroy(tp[0]);
 
@@ -731,7 +731,7 @@ static void timer_pool_create_destroy(void)
 
 	tim = odp_timer_alloc(tp[1], queue, USER_PTR);
 	CU_ASSERT(tim != ODP_TIMER_INVALID);
-	CU_ASSERT(odp_timer_free(tim) == ODP_EVENT_INVALID);
+	CU_ASSERT(odp_timer_free(tim) == 0);
 
 	odp_timer_pool_destroy(tp[1]);
 
@@ -741,7 +741,7 @@ static void timer_pool_create_destroy(void)
 
 	tim = odp_timer_alloc(tp[0], queue, USER_PTR);
 	CU_ASSERT(tim != ODP_TIMER_INVALID);
-	CU_ASSERT(odp_timer_free(tim) == ODP_EVENT_INVALID);
+	CU_ASSERT(odp_timer_free(tim) == 0);
 
 	odp_timer_pool_destroy(tp[0]);
 
@@ -816,7 +816,7 @@ static void timer_pool_create_max(void)
 	}
 
 	for (i = 0; i < num; i++)
-		CU_ASSERT(odp_timer_free(timer[i]) == ODP_EVENT_INVALID);
+		CU_ASSERT(odp_timer_free(timer[i]) == 0);
 
 	for (i = 0; i < num; i++)
 		odp_timer_pool_destroy(tp[i]);
@@ -912,7 +912,7 @@ static void timer_pool_max_res(void)
 			odp_event_free(ev);
 		}
 
-		CU_ASSERT(odp_timer_free(timer) == ODP_EVENT_INVALID);
+		CU_ASSERT(odp_timer_free(timer) == 0);
 		odp_timer_pool_destroy(tp);
 	}
 
@@ -1122,7 +1122,7 @@ static void timer_single_shot(odp_queue_type_t queue_type, odp_timer_tick_type_t
 
 	free_schedule_context(queue_type);
 
-	CU_ASSERT(odp_timer_free(timer) == ODP_EVENT_INVALID);
+	CU_ASSERT(odp_timer_free(timer) == 0);
 	odp_timer_pool_destroy(tp);
 
 	CU_ASSERT(odp_queue_destroy(queue) == 0);
@@ -1585,7 +1585,7 @@ static void timer_test_event_type(odp_queue_type_t queue_type,
 	}
 
 	for (i = 0; i < num; i++)
-		CU_ASSERT(odp_timer_free(timer[i]) == ODP_EVENT_INVALID);
+		CU_ASSERT(odp_timer_free(timer[i]) == 0);
 
 	odp_timer_pool_destroy(timer_pool);
 	CU_ASSERT(odp_queue_destroy(queue) == 0);
@@ -1791,7 +1791,7 @@ static void timer_test_queue_type(odp_queue_type_t queue_type, int priv, int exp
 				 tick, nsec, target, (int64_t)(nsec - target));
 
 			odp_timeout_free(tmo);
-			CU_ASSERT(odp_timer_free(tim) == ODP_EVENT_INVALID);
+			CU_ASSERT(odp_timer_free(tim) == 0);
 
 			num_tmo++;
 		}
@@ -1947,9 +1947,7 @@ static void timer_test_cancel(void)
 
 	odp_timeout_free(tmo);
 
-	ev = odp_timer_free(tim);
-	if (ev != ODP_EVENT_INVALID)
-		CU_FAIL_FATAL("Free returned event");
+	CU_ASSERT_FATAL(odp_timer_free(tim) == 0);
 
 	odp_timer_pool_destroy(tp);
 
@@ -2135,7 +2133,7 @@ static void timer_test_tmo_limit(odp_queue_type_t queue_type,
 		CU_ASSERT(num_tmo == num);
 
 	for (i = 0; i < num; i++)
-		CU_ASSERT(odp_timer_free(timer[i]) == ODP_EVENT_INVALID);
+		CU_ASSERT(odp_timer_free(timer[i]) == 0);
 
 	odp_timer_pool_destroy(timer_pool);
 	CU_ASSERT(odp_queue_destroy(queue) == 0);
@@ -2507,7 +2505,7 @@ sleep:
 	}
 
 	for (i = 0; i < allocated; i++) {
-		if (odp_timer_free(tt[i].tim) != ODP_EVENT_INVALID)
+		if (odp_timer_free(tt[i].tim))
 			CU_FAIL("odp_timer_free");
 	}
 
@@ -3108,7 +3106,7 @@ static void timer_test_periodic(odp_queue_type_t queue_type, int use_first, int
 		CU_ASSERT(ret == 2);
 	}
 
-	CU_ASSERT(odp_timer_free(timer) == ODP_EVENT_INVALID);
+	CU_ASSERT(odp_timer_free(timer) == 0);
 	odp_timer_pool_destroy(timer_pool);
 	CU_ASSERT(odp_queue_destroy(queue) == 0);
 	CU_ASSERT(odp_pool_destroy(pool) == 0);