thread: Set poller to PAUSING even when it is in RUNNING

This change is a preparation to first dequeue the closest timed poller
always when it is expired. Previously the poller_remove_timer() calls
were not consistent and difficult to follow.

spdk_poller_pause() sets poller to PAUSING even when it in RUNNING
and move it to PAUSED after returning from its context.

If spdk_poller_pause() and spdk_poller_resume() are called while poller
runs, it is moved to WAITING. Hence thread_execute_poller() and
thread_execute_timed_poller() ignore such cases.

Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Change-Id: I29340613a2ec0c3529d0886f4d81c0a0fdf8745d
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/7908
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Community-CI: Mellanox Build Bot
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>

lib/thread/thread.c

@@ -757,7 +757,13 @@ thread_execute_poller(struct spdk_thread *thread, struct spdk_poller *poller)
 		TAILQ_REMOVE(&thread->active_pollers, poller, tailq);
 		free(poller);
 		break;
+	case SPDK_POLLER_STATE_PAUSING:
+		TAILQ_REMOVE(&thread->active_pollers, poller, tailq);
+		TAILQ_INSERT_TAIL(&thread->paused_pollers, poller, tailq);
+		poller->state = SPDK_POLLER_STATE_PAUSED;
+		break;
 	case SPDK_POLLER_STATE_PAUSED:
+	case SPDK_POLLER_STATE_WAITING:
 		break;
 	case SPDK_POLLER_STATE_RUNNING:
 		poller->state = SPDK_POLLER_STATE_WAITING;
@@ -812,10 +818,17 @@ thread_execute_timed_poller(struct spdk_thread *thread, struct spdk_poller *poll
 		poller_remove_timer(thread, poller);
 		free(poller);
 		break;
+	case SPDK_POLLER_STATE_PAUSING:
+		poller_remove_timer(thread, poller);
+		TAILQ_INSERT_TAIL(&thread->paused_pollers, poller, tailq);
+		poller->state = SPDK_POLLER_STATE_PAUSED;
+		break;
 	case SPDK_POLLER_STATE_PAUSED:
 		break;
 	case SPDK_POLLER_STATE_RUNNING:
 		poller->state = SPDK_POLLER_STATE_WAITING;
+	/* fallthrough */
+	case SPDK_POLLER_STATE_WAITING:
 		poller_remove_timer(thread, poller);
 		poller_insert_timer(thread, poller, now);
 		break;
@@ -1528,26 +1541,17 @@ spdk_poller_pause(struct spdk_poller *poller)
 		return;
 	}
 
-	/* If a poller is paused from within itself, we can immediately move it
+	/* We just set its state to SPDK_POLLER_STATE_PAUSING and let
-	 * on the paused_pollers list.  Otherwise we just set its state to
+	 * spdk_thread_poll() move it. It allows a poller to be paused from
-	 * SPDK_POLLER_STATE_PAUSING and let spdk_thread_poll() move it.  It
+	 * another one's context without breaking the TAILQ_FOREACH_REVERSE_SAFE
-	 * allows a poller to be paused from another one's context without
+	 * iteration, or from within itself without breaking the logic to always
-	 * breaking the TAILQ_FOREACH_REVERSE_SAFE iteration.
+	 * remove the closest timed poller in the TAILQ_FOREACH_SAFE iteration.
 	 */
 	switch (poller->state) {
 	case SPDK_POLLER_STATE_PAUSED:
 	case SPDK_POLLER_STATE_PAUSING:
 		break;
 	case SPDK_POLLER_STATE_RUNNING:
-		if (poller->period_ticks > 0) {
-			poller_remove_timer(thread, poller);
-		} else {
-			TAILQ_REMOVE(&thread->active_pollers, poller, tailq);
-		}
-
-		TAILQ_INSERT_TAIL(&thread->paused_pollers, poller, tailq);
-		poller->state = SPDK_POLLER_STATE_PAUSED;
-		break;
 	case SPDK_POLLER_STATE_WAITING:
 		poller->state = SPDK_POLLER_STATE_PAUSING;
 		break;

test/unit/lib/thread/thread.c/thread_ut.c

@@ -244,6 +244,23 @@ poller_run_pause(void *ctx)
 	return 0;
 }
 
+/* Verify the same poller can be switched multiple times between
+ * pause and resume while it runs.
+ */
+static int
+poller_run_pause_resume_pause(void *ctx)
+{
+	struct poller_ctx *poller_ctx = ctx;
+
+	poller_ctx->run = true;
+
+	spdk_poller_pause(poller_ctx->poller);
+	spdk_poller_resume(poller_ctx->poller);
+	spdk_poller_pause(poller_ctx->poller);
+
+	return 0;
+}
+
 static void
 poller_msg_pause_cb(void *ctx)
 {
@@ -285,6 +302,21 @@ poller_pause(void)
 	spdk_poller_unregister(&poller_ctx.poller);
 	CU_ASSERT_PTR_NULL(poller_ctx.poller);
 
+	/* Register a poller that switches between pause and resume itself */
+	poller_ctx.poller = spdk_poller_register(poller_run_pause_resume_pause, &poller_ctx, 0);
+	CU_ASSERT_PTR_NOT_NULL(poller_ctx.poller);
+
+	poller_ctx.run = false;
+	poll_threads();
+	CU_ASSERT_EQUAL(poller_ctx.run, true);
+
+	poller_ctx.run = false;
+	poll_threads();
+	CU_ASSERT_EQUAL(poller_ctx.run, false);
+
+	spdk_poller_unregister(&poller_ctx.poller);
+	CU_ASSERT_PTR_NULL(poller_ctx.poller);
+
 	/* Verify that resuming an unpaused poller doesn't do anything */
 	poller_ctx.poller = spdk_poller_register(poller_run_done, &poller_ctx.run, 0);
 	CU_ASSERT_PTR_NOT_NULL(poller_ctx.poller);
@@ -386,6 +418,55 @@ poller_pause(void)
 
 		spdk_poller_unregister(&poller_ctx.poller);
 		CU_ASSERT_PTR_NULL(poller_ctx.poller);
+
+		/* Register a timed poller that pauses itself */
+		poller_ctx.poller = spdk_poller_register(poller_run_pause, &poller_ctx, delay[i]);
+		CU_ASSERT_PTR_NOT_NULL(poller_ctx.poller);
+
+		spdk_delay_us(delay[i]);
+		poller_ctx.run = false;
+		poll_threads();
+		CU_ASSERT_EQUAL(poller_ctx.run, true);
+
+		poller_ctx.run = false;
+		spdk_delay_us(delay[i]);
+		poll_threads();
+		CU_ASSERT_EQUAL(poller_ctx.run, false);
+
+		spdk_poller_resume(poller_ctx.poller);
+
+		CU_ASSERT_EQUAL(poller_ctx.run, false);
+		spdk_delay_us(delay[i]);
+		poll_threads();
+		CU_ASSERT_EQUAL(poller_ctx.run, true);
+
+		spdk_poller_unregister(&poller_ctx.poller);
+		CU_ASSERT_PTR_NULL(poller_ctx.poller);
+
+		/* Register a timed poller that switches between pause and resume itself */
+		poller_ctx.poller = spdk_poller_register(poller_run_pause_resume_pause,
+				    &poller_ctx, delay[i]);
+		CU_ASSERT_PTR_NOT_NULL(poller_ctx.poller);
+
+		spdk_delay_us(delay[i]);
+		poller_ctx.run = false;
+		poll_threads();
+		CU_ASSERT_EQUAL(poller_ctx.run, true);
+
+		poller_ctx.run = false;
+		spdk_delay_us(delay[i]);
+		poll_threads();
+		CU_ASSERT_EQUAL(poller_ctx.run, false);
+
+		spdk_poller_resume(poller_ctx.poller);
+
+		CU_ASSERT_EQUAL(poller_ctx.run, false);
+		spdk_delay_us(delay[i]);
+		poll_threads();
+		CU_ASSERT_EQUAL(poller_ctx.run, true);
+
+		spdk_poller_unregister(&poller_ctx.poller);
+		CU_ASSERT_PTR_NULL(poller_ctx.poller);
 	}
 
 	free_threads();