Commit 0aef7f2a authored by Jim Harris's avatar Jim Harris Committed by Konrad Sztyber
Browse files

nvmf: use RB-tree to track tgt subsystems 



Currently we use an array, which is computationally
expensive for lots of subsystems, because lookups
require O(n) string compares of the subsystem nqns.
It's not capacity expensive, since it's just a
pointer array.

So switch it from an array of pointers to an RB_HEAD,
so that we can reduce the lookups to O(log n) string
compares instead.

Note that we will still allocate arrays of
spdk_nvmf_subsystem_poll_groups for each transport
poll group, because we don't want to incur the extra
cost of RB_FINDs in the IO path.

Signed-off-by: default avatarJim Harris <james.r.harris@intel.com>
Change-Id: I896453154304bc41933757822f6078724d488908
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17966


Community-CI: Mellanox Build Bot
Reviewed-by: default avatarKonrad Sztyber <konrad.sztyber@intel.com>
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
parent 779059a2

lib/nvmf/nvmf.c

+4 −22
Original line number Diff line number Diff line
@@ -318,12 +318,7 @@ spdk_nvmf_tgt_create(struct spdk_nvmf_target_opts *opts) 
		return NULL;

	}



	tgt->subsystems = calloc(tgt->max_subsystems, sizeof(struct spdk_nvmf_subsystem *));

	if (!tgt->subsystems) {

		spdk_bit_array_free(&tgt->subsystem_ids);

		free(tgt);

		return NULL;

	}

	RB_INIT(&tgt->subsystems);



	pthread_mutex_init(&tgt->mutex, NULL);
@@ -370,11 +365,6 @@ nvmf_tgt_destroy_cb(void *io_device) 
	struct spdk_nvmf_subsystem *subsystem, *subsystem_next;

	int rc;



	if (tgt->subsystems == NULL) {

		_nvmf_tgt_destroy_next_transport(tgt);

		return;

	}



	/* We will be freeing subsystems in this loop, so we always need to get the next one

	 * ahead of time, since we can't call get_next() on a subsystem that's been freed.

	 */
@@ -396,7 +386,6 @@ nvmf_tgt_destroy_cb(void *io_device) 
			}

		}

	}

	free(tgt->subsystems);

	spdk_bit_array_free(&tgt->subsystem_ids);

	_nvmf_tgt_destroy_next_transport(tgt);

}
@@ -946,7 +935,7 @@ spdk_nvmf_tgt_resume_polling(struct spdk_nvmf_tgt *tgt, spdk_nvmf_tgt_resume_pol 
struct spdk_nvmf_subsystem *

spdk_nvmf_tgt_find_subsystem(struct spdk_nvmf_tgt *tgt, const char *subnqn)

{

	struct spdk_nvmf_subsystem *subsystem;

	struct spdk_nvmf_subsystem subsystem;



	if (!subnqn) {

		return NULL;
@@ -958,15 +947,8 @@ spdk_nvmf_tgt_find_subsystem(struct spdk_nvmf_tgt *tgt, const char *subnqn) 
		return NULL;

	}



	for (subsystem = spdk_nvmf_subsystem_get_first(tgt);

	     subsystem != NULL;

	     subsystem = spdk_nvmf_subsystem_get_next(subsystem)) {

		if (strcmp(subnqn, subsystem->subnqn) == 0) {

			return subsystem;

		}

	}



	return NULL;

	snprintf(subsystem.subnqn, sizeof(subsystem.subnqn), "%s", subnqn);

	return RB_FIND(subsystem_tree, &tgt->subsystems, &subsystem);

}



struct spdk_nvmf_transport *

lib/nvmf/nvmf_internal.h

+13 −2
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ 
#include "spdk/queue.h"

#include "spdk/util.h"

#include "spdk/thread.h"

#include "spdk/tree.h"



/* The spec reserves cntlid values in the range FFF0h to FFFFh. */

#define NVMF_MIN_CNTLID 1
@@ -43,6 +44,8 @@ enum spdk_nvmf_subsystem_state { 
	SPDK_NVMF_SUBSYSTEM_NUM_STATES,

};



RB_HEAD(subsystem_tree, spdk_nvmf_subsystem);



struct spdk_nvmf_tgt {

	char					name[NVMF_TGT_NAME_MAX_LENGTH];
@@ -58,8 +61,7 @@ struct spdk_nvmf_tgt { 


	struct spdk_bit_array			*subsystem_ids;



	/* Array of subsystem pointers of size max_subsystems indexed by sid */

	struct spdk_nvmf_subsystem		**subsystems;

	struct subsystem_tree			subsystems;



	TAILQ_HEAD(, spdk_nvmf_transport)	transports;

	TAILQ_HEAD(, spdk_nvmf_poll_group)	poll_groups;
@@ -282,6 +284,7 @@ struct spdk_nvmf_subsystem { 
	uint64_t					max_zone_append_size_kib;



	struct spdk_nvmf_tgt				*tgt;

	RB_ENTRY(spdk_nvmf_subsystem)			link;



	/* Array of pointers to namespaces of size max_nsid indexed by nsid - 1 */

	struct spdk_nvmf_ns				**ns;
@@ -317,6 +320,14 @@ struct spdk_nvmf_subsystem { 
	uint32_t					*ana_group;

};



static int

subsystem_cmp(struct spdk_nvmf_subsystem *subsystem1, struct spdk_nvmf_subsystem *subsystem2)

{

	return strncmp(subsystem1->subnqn, subsystem2->subnqn, sizeof(subsystem1->subnqn));

}



RB_GENERATE_STATIC(subsystem_tree, spdk_nvmf_subsystem, link, subsystem_cmp);



int nvmf_poll_group_update_subsystem(struct spdk_nvmf_poll_group *group,

				     struct spdk_nvmf_subsystem *subsystem);

int nvmf_poll_group_add_subsystem(struct spdk_nvmf_poll_group *group,

lib/nvmf/subsystem.c

+4 −26
Original line number Diff line number Diff line
@@ -300,7 +300,7 @@ spdk_nvmf_subsystem_create(struct spdk_nvmf_tgt *tgt, 
		 MODEL_NUMBER_DEFAULT);



	spdk_bit_array_set(tgt->subsystem_ids, sid);

	tgt->subsystems[sid] = subsystem;

	RB_INSERT(subsystem_tree, &tgt->subsystems, subsystem);



	SPDK_DTRACE_PROBE1(nvmf_subsystem_create, subsystem->subnqn);
@@ -382,7 +382,7 @@ _nvmf_subsystem_destroy(struct spdk_nvmf_subsystem *subsystem) 
	free(subsystem->ns);

	free(subsystem->ana_group);



	subsystem->tgt->subsystems[subsystem->id] = NULL;

	RB_REMOVE(subsystem_tree, &subsystem->tgt->subsystems, subsystem);

	assert(spdk_bit_array_get(subsystem->tgt->subsystem_ids, subsystem->id) == true);

	spdk_bit_array_clear(subsystem->tgt->subsystem_ids, subsystem->id);
@@ -760,39 +760,17 @@ spdk_nvmf_subsystem_resume(struct spdk_nvmf_subsystem *subsystem, 
struct spdk_nvmf_subsystem *

spdk_nvmf_subsystem_get_first(struct spdk_nvmf_tgt *tgt)

{

	struct spdk_nvmf_subsystem	*subsystem;

	uint32_t sid;



	for (sid = 0; sid < tgt->max_subsystems; sid++) {

		subsystem = tgt->subsystems[sid];

		if (subsystem) {

			return subsystem;

		}

	}



	return NULL;

	return RB_MIN(subsystem_tree, &tgt->subsystems);

}



struct spdk_nvmf_subsystem *

spdk_nvmf_subsystem_get_next(struct spdk_nvmf_subsystem *subsystem)

{

	uint32_t sid;

	struct spdk_nvmf_tgt *tgt;



	if (!subsystem) {

		return NULL;

	}



	tgt = subsystem->tgt;



	for (sid = subsystem->id + 1; sid < tgt->max_subsystems; sid++) {

		subsystem = tgt->subsystems[sid];

		if (subsystem) {

			return subsystem;

		}

	}



	return NULL;

	return RB_NEXT(subsystem_tree, &tgt->subsystems, subsystem);

}



/* Must hold subsystem->mutex while calling this function */

test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c

+2 −6
Original line number Diff line number Diff line
@@ -285,8 +285,7 @@ test_discovery_log(void) 


	tgt.max_subsystems = 1024;

	tgt.subsystem_ids = spdk_bit_array_create(tgt.max_subsystems);

	tgt.subsystems = calloc(tgt.max_subsystems, sizeof(struct spdk_nvmf_subsystem *));

	SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL);

	RB_INIT(&tgt.subsystems);



	/* Add one subsystem and verify that the discovery log contains it */

	subsystem = spdk_nvmf_subsystem_create(&tgt, "nqn.2016-06.io.spdk:subsystem1",
@@ -374,7 +373,6 @@ test_discovery_log(void) 
	CU_ASSERT(disc_log->genctr != 0);

	CU_ASSERT(disc_log->numrec == 0);



	free(tgt.subsystems);

	spdk_bit_array_free(&tgt.subsystem_ids);

}
@@ -420,8 +418,7 @@ test_discovery_log_with_filters(void) 


	tgt.max_subsystems = 4;

	tgt.subsystem_ids = spdk_bit_array_create(tgt.max_subsystems);

	tgt.subsystems = calloc(tgt.max_subsystems, sizeof(struct spdk_nvmf_subsystem *));

	SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL);

	RB_INIT(&tgt.subsystems);



	subsystem = spdk_nvmf_subsystem_create(&tgt, "nqn.2016-06.io.spdk:subsystem1",

					       SPDK_NVMF_SUBTYPE_NVME, 0);
@@ -641,7 +638,6 @@ test_discovery_log_with_filters(void) 


	subsystem->state = SPDK_NVMF_SUBSYSTEM_INACTIVE;

	spdk_nvmf_subsystem_destroy(subsystem, NULL, NULL);

	free(tgt.subsystems);

	spdk_bit_array_free(&tgt.subsystem_ids);

}

test/unit/lib/nvmf/nvmf.c/nvmf_ut.c

+6 −4
Original line number Diff line number Diff line
@@ -155,10 +155,13 @@ test_nvmf_tgt_create_poll_group(void) 
	MOCK_SET(spdk_bdev_get_io_channel, &ch);



	tgt.max_subsystems = 1;

	tgt.subsystems = calloc(tgt.max_subsystems, sizeof(struct spdk_nvmf_subsystem *));

	SPDK_CU_ASSERT_FATAL(tgt.subsystems != NULL);

	RB_INIT(&tgt.subsystems);



	tgt.subsystems[0] = &subsystem;

	/* Make sure subsystem has enough in subnqn so it can be

	 * inserted into RB-tree.

	 */

	snprintf(subsystem.subnqn, sizeof(subsystem.subnqn), "abc");

	RB_INSERT(subsystem_tree, &tgt.subsystems, &subsystem);

	subsystem.id = 0;

	subsystem.max_nsid = 1;

	subsystem.ns = calloc(1, sizeof(struct spdk_nvmf_ns *));
@@ -201,7 +204,6 @@ test_nvmf_tgt_create_poll_group(void) 
	CU_ASSERT(TAILQ_EMPTY(&tgt.poll_groups));

	CU_ASSERT(tgt.num_poll_groups == 0);

	free(subsystem.ns);

	free(tgt.subsystems);

	MOCK_CLEAR(spdk_nvmf_subsystem_get_first);



	spdk_thread_exit(thread);