From c754043946592ba5ecb6a171bc37d35afaeb31f8 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Mon, 7 Jun 2021 14:50:40 +0000 Subject: [PATCH] Revert "thread: speed up io_device lookup by using rbtree" This reverts commit 2246a9371809c30333b1844afbf9772c4b06db79. We are seeing a lot of failure on io_device lookup in the test pool. These only showed up after this patch was merged and sees the most likely culprit. Reverting this patch for now while we continue debug. Signed-off-by: Jim Harris Change-Id: I2ab098319dfae3a5356eb4fe0dbf9f4af2d2eea5 Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8199 Community-CI: Mellanox Build Bot Reviewed-by: Tomasz Zawadzki Reviewed-by: Aleksey Marchuk Reviewed-by: Shuhei Matsumoto Reviewed-by: Paul Luse Tested-by: SPDK CI Jenkins --- lib/thread/thread.c | 54 ++++++++++------------- test/unit/lib/thread/thread.c/thread_ut.c | 42 ++++++++++++------ 2 files changed, 52 insertions(+), 44 deletions(-) diff --git a/lib/thread/thread.c b/lib/thread/thread.c index 39a9839db..3cee7de00 100644 --- a/lib/thread/thread.c +++ b/lib/thread/thread.c @@ -177,22 +177,14 @@ struct io_device { struct spdk_thread *unregister_thread; uint32_t ctx_size; uint32_t for_each_count; - RB_ENTRY(io_device) node; + TAILQ_ENTRY(io_device) tailq; uint32_t refcnt; bool unregistered; }; -static RB_HEAD(io_device_tree, io_device) g_io_devices = RB_INITIALIZER(g_io_devices); - -static int -io_device_cmp(struct io_device *dev1, struct io_device *dev2) -{ - return dev1->io_device - dev2->io_device; -} - -RB_GENERATE_STATIC(io_device_tree, io_device, node, io_device_cmp); +static TAILQ_HEAD(, io_device) g_io_devices = TAILQ_HEAD_INITIALIZER(g_io_devices); struct spdk_msg { spdk_msg_fn fn; @@ -304,7 +296,7 @@ spdk_thread_lib_fini(void) { struct io_device *dev; - RB_FOREACH(dev, io_device_tree, &g_io_devices) { + TAILQ_FOREACH(dev, &g_io_devices, tailq) { SPDK_ERRLOG("io_device %s not unregistered\n", dev->name); } @@ -1850,15 +1842,6 @@ spdk_thread_set_interrupt_mode(bool enable_interrupt) return; } -static struct io_device * -io_device_get(void *io_device) -{ - struct io_device find = {}; - - find.io_device = io_device; - return RB_FIND(io_device_tree, &g_io_devices, &find); -} - void spdk_io_device_register(void *io_device, spdk_io_channel_create_cb create_cb, spdk_io_channel_destroy_cb destroy_cb, uint32_t ctx_size, @@ -1902,14 +1885,16 @@ spdk_io_device_register(void *io_device, spdk_io_channel_create_cb create_cb, dev->name, dev->io_device, thread->name); pthread_mutex_lock(&g_devlist_mutex); - tmp = RB_INSERT(io_device_tree, &g_io_devices, dev); - if (tmp != NULL) { - SPDK_ERRLOG("io_device %p already registered (old:%s new:%s)\n", - io_device, tmp->name, dev->name); - free(dev); - pthread_mutex_unlock(&g_devlist_mutex); - return; + TAILQ_FOREACH(tmp, &g_io_devices, tailq) { + if (tmp->io_device == io_device) { + SPDK_ERRLOG("io_device %p already registered (old:%s new:%s)\n", + io_device, tmp->name, dev->name); + free(dev); + pthread_mutex_unlock(&g_devlist_mutex); + return; + } } + TAILQ_INSERT_TAIL(&g_io_devices, dev, tailq); pthread_mutex_unlock(&g_devlist_mutex); } @@ -1963,7 +1948,12 @@ spdk_io_device_unregister(void *io_device, spdk_io_device_unregister_cb unregist } pthread_mutex_lock(&g_devlist_mutex); - dev = io_device_get(io_device); + TAILQ_FOREACH(dev, &g_io_devices, tailq) { + if (dev->io_device == io_device) { + break; + } + } + if (!dev) { SPDK_ERRLOG("io_device %p not found\n", io_device); assert(false); @@ -1980,7 +1970,7 @@ spdk_io_device_unregister(void *io_device, spdk_io_device_unregister_cb unregist dev->unregister_cb = unregister_cb; dev->unregistered = true; - RB_REMOVE(io_device_tree, &g_io_devices, dev); + TAILQ_REMOVE(&g_io_devices, dev, tailq); refcnt = dev->refcnt; dev->unregister_thread = thread; pthread_mutex_unlock(&g_devlist_mutex); @@ -2015,7 +2005,11 @@ spdk_get_io_channel(void *io_device) int rc; pthread_mutex_lock(&g_devlist_mutex); - dev = io_device_get(io_device); + TAILQ_FOREACH(dev, &g_io_devices, tailq) { + if (dev->io_device == io_device) { + break; + } + } if (dev == NULL) { SPDK_ERRLOG("could not find io_device %p\n", io_device); pthread_mutex_unlock(&g_devlist_mutex); diff --git a/test/unit/lib/thread/thread.c/thread_ut.c b/test/unit/lib/thread/thread.c/thread_ut.c index a3d57c2fb..150aaed84 100644 --- a/test/unit/lib/thread/thread.c/thread_ut.c +++ b/test/unit/lib/thread/thread.c/thread_ut.c @@ -644,12 +644,12 @@ for_each_channel_unreg(void) allocate_threads(1); set_thread(0); - CU_ASSERT(RB_EMPTY(&g_io_devices)); + CU_ASSERT(TAILQ_EMPTY(&g_io_devices)); spdk_io_device_register(&io_target, channel_create, channel_destroy, sizeof(int), NULL); - CU_ASSERT(!RB_EMPTY(&g_io_devices)); - dev = RB_MIN(io_device_tree, &g_io_devices); + CU_ASSERT(!TAILQ_EMPTY(&g_io_devices)); + dev = TAILQ_FIRST(&g_io_devices); SPDK_CU_ASSERT_FATAL(dev != NULL); - CU_ASSERT(RB_NEXT(io_device_tree, &g_io_devices, dev) == NULL); + CU_ASSERT(TAILQ_NEXT(dev, tailq) == NULL); ch0 = spdk_get_io_channel(&io_target); spdk_for_each_channel(&io_target, unreg_ch_done, &ctx, unreg_foreach_done); @@ -658,14 +658,14 @@ for_each_channel_unreg(void) * There is an outstanding foreach call on the io_device, so the unregister should not * have removed the device. */ - CU_ASSERT(dev == RB_MIN(io_device_tree, &g_io_devices)); + CU_ASSERT(dev == TAILQ_FIRST(&g_io_devices)); spdk_io_device_register(&io_target, channel_create, channel_destroy, sizeof(int), NULL); /* * There is already a device registered at &io_target, so a new io_device should not * have been added to g_io_devices. */ - CU_ASSERT(dev == RB_MIN(io_device_tree, &g_io_devices)); - CU_ASSERT(RB_NEXT(io_device_tree, &g_io_devices, dev) == NULL); + CU_ASSERT(dev == TAILQ_FIRST(&g_io_devices)); + CU_ASSERT(TAILQ_NEXT(dev, tailq) == NULL); poll_thread(0); CU_ASSERT(ctx.ch_done == true); @@ -675,7 +675,7 @@ for_each_channel_unreg(void) * even though a channel still exists for the device. */ spdk_io_device_unregister(&io_target, NULL); - CU_ASSERT(RB_EMPTY(&g_io_devices)); + CU_ASSERT(TAILQ_EMPTY(&g_io_devices)); set_thread(0); spdk_put_io_channel(ch0); @@ -824,7 +824,7 @@ channel(void) poll_threads(); spdk_io_device_unregister(&g_device2, NULL); poll_threads(); - CU_ASSERT(RB_EMPTY(&g_io_devices)); + CU_ASSERT(TAILQ_EMPTY(&g_io_devices)); free_threads(); CU_ASSERT(TAILQ_EMPTY(&g_threads)); } @@ -879,7 +879,7 @@ channel_destroy_races(void) spdk_io_device_unregister(&device, NULL); poll_threads(); - CU_ASSERT(RB_EMPTY(&g_io_devices)); + CU_ASSERT(TAILQ_EMPTY(&g_io_devices)); free_threads(); CU_ASSERT(TAILQ_EMPTY(&g_threads)); } @@ -1135,6 +1135,20 @@ struct ut_nested_dev { struct ut_nested_dev *child; }; +static struct io_device * +ut_get_io_device(void *dev) +{ + struct io_device *tmp; + + TAILQ_FOREACH(tmp, &g_io_devices, tailq) { + if (tmp->io_device == dev) { + return tmp; + } + } + + return NULL; +} + static int ut_null_poll(void *ctx) { @@ -1232,11 +1246,11 @@ nested_channel(void) spdk_io_device_register(&_dev3, ut_nested_ch_create_cb, ut_nested_ch_destroy_cb, sizeof(struct ut_nested_ch), "dev3"); - dev1 = io_device_get(&_dev1); + dev1 = ut_get_io_device(&_dev1); SPDK_CU_ASSERT_FATAL(dev1 != NULL); - dev2 = io_device_get(&_dev2); + dev2 = ut_get_io_device(&_dev2); SPDK_CU_ASSERT_FATAL(dev2 != NULL); - dev3 = io_device_get(&_dev3); + dev3 = ut_get_io_device(&_dev3); SPDK_CU_ASSERT_FATAL(dev3 != NULL); /* A single call spdk_get_io_channel() to dev1 will also create channels @@ -1301,7 +1315,7 @@ nested_channel(void) spdk_io_device_unregister(&_dev1, NULL); spdk_io_device_unregister(&_dev2, NULL); spdk_io_device_unregister(&_dev3, NULL); - CU_ASSERT(RB_EMPTY(&g_io_devices)); + CU_ASSERT(TAILQ_EMPTY(&g_io_devices)); free_threads(); CU_ASSERT(TAILQ_EMPTY(&g_threads));