From 648d6cd5ddc3c4069cc28e1f42b1522acc124acf Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Mon, 10 Feb 2020 19:35:38 -0500 Subject: [PATCH] lib/thread: Fail spdk_thread_exit() if thread has any registered poller This is a preparation to support voluntary thread termination by calling spdk_thread_exit(). Change spdk_thread_exit() to return -EBUSY if the thread has any registered poller. We enforce all pollers including paused poller are unresitered before the thread is marked as exited. By this change, a bug was found in reactor_perf test tool. Fix it by adding spdk_poller_unregister() and add the g_ prefix to avoid future potential errors. Signed-off-by: Shuhei Matsumoto Change-Id: If7f40357c9a6f4101b3998ea0da3cc46cc435031 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/487 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Aleksey Marchuk --- include/spdk/thread.h | 3 ++- lib/thread/thread.c | 24 +++++++++++++++++ test/event/reactor_perf/reactor_perf.c | 9 ++++--- test/unit/lib/thread/thread.c/thread_ut.c | 32 +++++++++++++++++++++-- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/include/spdk/thread.h b/include/spdk/thread.h index c78ae7547..5909739de 100644 --- a/include/spdk/thread.h +++ b/include/spdk/thread.h @@ -225,7 +225,8 @@ void spdk_set_thread(struct spdk_thread *thread); * only be called within an spdk poller or message. * * All I/O channel references associated with the thread must be released using - * spdk_put_io_channel() prior to calling this function. + * spdk_put_io_channel(), and all active pollers associated with the thread must + * be unregistered using spdk_poller_unregister(), prior to calling this function. * * \param thread The thread to destroy. * diff --git a/lib/thread/thread.c b/lib/thread/thread.c index 69d96844c..0f20a78a4 100644 --- a/lib/thread/thread.c +++ b/lib/thread/thread.c @@ -348,6 +348,8 @@ spdk_set_thread(struct spdk_thread *thread) int spdk_thread_exit(struct spdk_thread *thread) { + struct spdk_poller *poller; + SPDK_DEBUGLOG(SPDK_LOG_THREAD, "Exit thread %s\n", thread->name); assert(tls_thread == thread); @@ -358,6 +360,28 @@ spdk_thread_exit(struct spdk_thread *thread) return -EINVAL; } + TAILQ_FOREACH(poller, &thread->active_pollers, tailq) { + if (poller->state != SPDK_POLLER_STATE_UNREGISTERED) { + SPDK_ERRLOG("thread %s still has active poller %p\n", + thread->name, poller); + return -EBUSY; + } + } + + TAILQ_FOREACH(poller, &thread->timed_pollers, tailq) { + if (poller->state != SPDK_POLLER_STATE_UNREGISTERED) { + SPDK_ERRLOG("thread %s still has active timed poller %p\n", + thread->name, poller); + return -EBUSY; + } + } + + TAILQ_FOREACH(poller, &thread->paused_pollers, tailq) { + SPDK_ERRLOG("thread %s still has paused poller %p\n", + thread->name, poller); + return -EBUSY; + } + thread->exit = true; return 0; } diff --git a/test/event/reactor_perf/reactor_perf.c b/test/event/reactor_perf/reactor_perf.c index e4287f3ed..e8f8ff7dd 100644 --- a/test/event/reactor_perf/reactor_perf.c +++ b/test/event/reactor_perf/reactor_perf.c @@ -40,13 +40,14 @@ static int g_time_in_sec; static int g_queue_depth; -static struct spdk_poller *test_end_poller; +static struct spdk_poller *g_test_end_poller; static uint64_t g_call_count = 0; static int __test_end(void *arg) { printf("test_end\n"); + spdk_poller_unregister(&g_test_end_poller); spdk_app_stop(0); return -1; } @@ -71,8 +72,8 @@ test_start(void *arg1) printf("test_start\n"); /* Register a poller that will stop the test after the time has elapsed. */ - test_end_poller = spdk_poller_register(__test_end, NULL, - g_time_in_sec * 1000000ULL); + g_test_end_poller = spdk_poller_register(__test_end, NULL, + g_time_in_sec * 1000000ULL); for (i = 0; i < g_queue_depth; i++) { __submit_next(NULL, NULL); @@ -84,7 +85,7 @@ test_cleanup(void) { printf("test_abort\n"); - spdk_poller_unregister(&test_end_poller); + spdk_poller_unregister(&g_test_end_poller); spdk_app_stop(0); } diff --git a/test/unit/lib/thread/thread.c/thread_ut.c b/test/unit/lib/thread/thread.c/thread_ut.c index 681ec4230..ba46149bd 100644 --- a/test/unit/lib/thread/thread.c/thread_ut.c +++ b/test/unit/lib/thread/thread.c/thread_ut.c @@ -750,11 +750,12 @@ thread_exit(void) { struct spdk_thread *thread; struct spdk_io_channel *ch; + struct spdk_poller *poller; void *ctx; - bool done1 = false, done2 = false; + bool done1 = false, done2 = false, poller_run = false; int rc __attribute__((unused)); - allocate_threads(4); + allocate_threads(5); /* Test all pending messages are reaped for the thread marked as exited. */ set_thread(0); @@ -819,6 +820,33 @@ thread_exit(void) CU_ASSERT(spdk_thread_exit(thread) == 0); CU_ASSERT(spdk_thread_exit(thread) == -EINVAL); + /* Test if spdk_thread_exit() fails when there is any not-unregistered poller, + * and if no poller is executed after the thread is marked as exited. + */ + set_thread(4); + thread = spdk_get_thread(); + + poller = spdk_poller_register(poller_run_done, &poller_run, 0); + CU_ASSERT(poller != NULL); + + CU_ASSERT(spdk_thread_exit(thread) == -EBUSY); + + spdk_poller_pause(poller); + + CU_ASSERT(spdk_thread_exit(thread) == -EBUSY); + + poll_threads(); + + CU_ASSERT(spdk_thread_exit(thread) == -EBUSY); + + spdk_poller_unregister(&poller); + + CU_ASSERT(spdk_thread_exit(thread) == 0); + + poll_threads(); + + CU_ASSERT(poller_run == false); + free_threads(); }