Commit c7540439 authored by Jim Harris's avatar Jim Harris
Browse files

Revert "thread: speed up io_device lookup by using rbtree" 



This reverts commit 2246a937.

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: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: I2ab098319dfae3a5356eb4fe0dbf9f4af2d2eea5
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/8199


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarTomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: default avatarShuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: default avatarPaul Luse <paul.e.luse@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 9d8e1ec0

lib/thread/thread.c

+24 −30
Original line number Diff line number Diff line
@@ -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) {

	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);

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

+28 −14
Original line number Diff line number Diff line
@@ -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));