From 836356f2d586043c965b573f6cfba0c872b3ffdd Mon Sep 17 00:00:00 2001 From: Ben Walker Date: Tue, 15 Jan 2019 14:35:46 -0700 Subject: [PATCH] thread: Run all pollers on each spdk_thread_poll call This prevents issues where spdk_thread_poll may report that it did not useful work (for the one poller it ran), causing the system thread to go to sleep. Change-Id: I7a4842d5e399758c19268aee343a001ccfc88a3a Signed-off-by: Ben Walker Reviewed-on: https://review.gerrithub.io/c/440598 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Shuhei Matsumoto Reviewed-by: Darek Stojaczyk --- lib/thread/thread.c | 114 +++++++++++++++++++++++++++----------------- 1 file changed, 71 insertions(+), 43 deletions(-) diff --git a/lib/thread/thread.c b/lib/thread/thread.c index 502cffdc6..192fd1f33 100644 --- a/lib/thread/thread.c +++ b/lib/thread/thread.c @@ -121,12 +121,12 @@ struct spdk_thread { * of the ring, executes it, then puts it back at the tail of * the ring. */ - TAILQ_HEAD(, spdk_poller) active_pollers; + TAILQ_HEAD(active_pollers_head, spdk_poller) active_pollers; /** * Contains pollers running on this thread with a periodic timer. */ - TAILQ_HEAD(timer_pollers_head, spdk_poller) timer_pollers; + TAILQ_HEAD(timer_pollers_head, spdk_poller) timer_pollers; struct spdk_ring *messages; @@ -264,6 +264,7 @@ spdk_thread_exit(struct spdk_thread *thread) { struct spdk_io_channel *ch; struct spdk_msg *msg; + struct spdk_poller *poller, *ptmp; SPDK_DEBUGLOG(SPDK_LOG_THREAD, "Freeing thread %s\n", thread->name); @@ -276,6 +277,27 @@ spdk_thread_exit(struct spdk_thread *thread) thread->name, ch->dev->name); } + TAILQ_FOREACH_SAFE(poller, &thread->active_pollers, tailq, ptmp) { + if (poller->state == SPDK_POLLER_STATE_WAITING) { + SPDK_WARNLOG("poller %p still registered at thread exit\n", + poller); + } + + TAILQ_REMOVE(&thread->active_pollers, poller, tailq); + free(poller); + } + + + TAILQ_FOREACH_SAFE(poller, &thread->timer_pollers, tailq, ptmp) { + if (poller->state == SPDK_POLLER_STATE_WAITING) { + SPDK_WARNLOG("poller %p still registered at thread exit\n", + poller); + } + + TAILQ_REMOVE(&thread->timer_pollers, poller, tailq); + free(poller); + } + pthread_mutex_lock(&g_devlist_mutex); assert(g_thread_count > 0); g_thread_count--; @@ -376,7 +398,7 @@ spdk_thread_poll(struct spdk_thread *thread, uint32_t max_msgs, uint64_t now) { uint32_t msg_count; struct spdk_thread *orig_thread; - struct spdk_poller *poller; + struct spdk_poller *poller, *tmp; int rc = 0; orig_thread = _get_thread(); @@ -391,20 +413,27 @@ spdk_thread_poll(struct spdk_thread *thread, uint32_t max_msgs, uint64_t now) rc = 1; } - poller = TAILQ_FIRST(&thread->active_pollers); - if (poller) { + TAILQ_FOREACH_REVERSE_SAFE(poller, &thread->active_pollers, + active_pollers_head, tailq, tmp) { int poller_rc; - TAILQ_REMOVE(&thread->active_pollers, poller, tailq); + if (poller->state == SPDK_POLLER_STATE_UNREGISTERED) { + TAILQ_REMOVE(&thread->active_pollers, poller, tailq); + free(poller); + continue; + } + poller->state = SPDK_POLLER_STATE_RUNNING; poller_rc = poller->fn(poller->arg); + if (poller->state == SPDK_POLLER_STATE_UNREGISTERED) { + TAILQ_REMOVE(&thread->active_pollers, poller, tailq); free(poller); - } else { - poller->state = SPDK_POLLER_STATE_WAITING; - TAILQ_INSERT_TAIL(&thread->active_pollers, poller, tailq); + continue; } + poller->state = SPDK_POLLER_STATE_WAITING; + #ifdef DEBUG if (poller_rc == -1) { SPDK_DEBUGLOG(SPDK_LOG_THREAD, "Poller %p returned -1\n", poller); @@ -414,33 +443,44 @@ spdk_thread_poll(struct spdk_thread *thread, uint32_t max_msgs, uint64_t now) if (poller_rc > rc) { rc = poller_rc; } + } - poller = TAILQ_FIRST(&thread->timer_pollers); - if (poller) { - if (now >= poller->next_run_tick) { - int timer_rc = 0; + TAILQ_FOREACH_SAFE(poller, &thread->timer_pollers, tailq, tmp) { + int timer_rc = 0; + if (poller->state == SPDK_POLLER_STATE_UNREGISTERED) { TAILQ_REMOVE(&thread->timer_pollers, poller, tailq); - poller->state = SPDK_POLLER_STATE_RUNNING; - timer_rc = poller->fn(poller->arg); - if (poller->state == SPDK_POLLER_STATE_UNREGISTERED) { - free(poller); - } else { - poller->state = SPDK_POLLER_STATE_WAITING; - _spdk_poller_insert_timer(thread, poller, now); - } + free(poller); + continue; + } + + if (now < poller->next_run_tick) { + break; + } + + poller->state = SPDK_POLLER_STATE_RUNNING; + timer_rc = poller->fn(poller->arg); + + if (poller->state == SPDK_POLLER_STATE_UNREGISTERED) { + TAILQ_REMOVE(&thread->timer_pollers, poller, tailq); + free(poller); + continue; + } + + poller->state = SPDK_POLLER_STATE_WAITING; + TAILQ_REMOVE(&thread->timer_pollers, poller, tailq); + _spdk_poller_insert_timer(thread, poller, now); #ifdef DEBUG - if (timer_rc == -1) { - SPDK_DEBUGLOG(SPDK_LOG_THREAD, "Timed poller %p returned -1\n", poller); - } + if (timer_rc == -1) { + SPDK_DEBUGLOG(SPDK_LOG_THREAD, "Timed poller %p returned -1\n", poller); + } #endif - if (timer_rc > rc) { - rc = timer_rc; + if (timer_rc > rc) { + rc = timer_rc; - } } } @@ -636,22 +676,10 @@ spdk_poller_unregister(struct spdk_poller **ppoller) return; } - if (poller->state == SPDK_POLLER_STATE_RUNNING) { - /* - * We are being called from the poller_fn, so set the state to unregistered - * and let the thread poll loop free the poller. - */ - poller->state = SPDK_POLLER_STATE_UNREGISTERED; - } else { - /* Poller is not running currently, so just free it. */ - if (poller->period_ticks) { - TAILQ_REMOVE(&thread->timer_pollers, poller, tailq); - } else { - TAILQ_REMOVE(&thread->active_pollers, poller, tailq); - } - - free(poller); - } + /* Simply set the state to unregistered. The poller will get cleaned up + * in a subsequent call to spdk_thread_poll(). + */ + poller->state = SPDK_POLLER_STATE_UNREGISTERED; } struct call_thread {