nvmf: cntlid is now only unique within a subsystem

Previously we made cntlid globally unique as part of
a strategy for scaling connections that never panned
out. Now, we have a new strategy and don't need cntlid
to be globally unique, so relax the restrictions
and simplify the code.

Change-Id: I167772f5e7d37183715bf9967b0102529144bb2b
Signed-off-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-on: https://review.gerrithub.io/376250
Tested-by: SPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: Daniel Verkamp <daniel.verkamp@intel.com>

lib/nvmf/ctrlr.c

@@ -80,8 +80,8 @@ spdk_nvmf_ctrlr_create(struct spdk_nvmf_subsystem *subsystem,
 		return NULL;
 	}
 
-	ctrlr->cntlid = spdk_nvmf_tgt_gen_cntlid(tgt);
+	ctrlr->cntlid = spdk_nvmf_subsystem_gen_cntlid(subsystem);
-	if (ctrlr->cntlid == 0) {
+	if (ctrlr->cntlid == 0xFFFF) {
 		/* Unable to get a cntlid */
 		SPDK_ERRLOG("Reached max simultaneous ctrlrs\n");
 		spdk_nvmf_poll_group_destroy(ctrlr->group);

lib/nvmf/nvmf.c

@@ -164,48 +164,6 @@ spdk_nvmf_tgt_find_subsystem(struct spdk_nvmf_tgt *tgt, const char *subnqn)
 	return NULL;
 }
 
-uint16_t
-spdk_nvmf_tgt_gen_cntlid(struct spdk_nvmf_tgt *tgt)
-{
-	struct spdk_nvmf_subsystem *subsystem;
-	struct spdk_nvmf_ctrlr *ctrlr;
-	uint16_t count;
-
-	/*
-	 * In the worst case, we might have to try all CNTLID values between 1 and 0xFFF0 - 1
-	 * before we find one that is unused (or find that all values are in use).
-	 */
-	count = 0xFFF0 - 1;
-	do {
-		tgt->next_cntlid++;
-		if (tgt->next_cntlid >= 0xFFF0) {
-			/* The spec reserves cntlid values in the range FFF0h to FFFFh. */
-			tgt->next_cntlid = 1;
-		}
-
-		/* Check if a subsystem with this cntlid currently exists. This could
-		 * happen for a very long-lived ctrlr on a target with many short-lived
-		 * ctrlrs, where cntlid wraps around.
-		 */
-		TAILQ_FOREACH(subsystem, &tgt->subsystems, entries) {
-			TAILQ_FOREACH(ctrlr, &subsystem->ctrlrs, link) {
-				if (ctrlr->cntlid == tgt->next_cntlid) {
-					break;
-				}
-			}
-		}
-
-		count--;
-
-	} while (subsystem != NULL && count > 0);
-
-	if (count == 0) {
-		return 0;
-	}
-
-	return tgt->next_cntlid;
-}
-
 struct spdk_nvmf_transport *
 spdk_nvmf_tgt_get_transport(struct spdk_nvmf_tgt *tgt, enum spdk_nvme_transport_type type)
 {

lib/nvmf/nvmf_internal.h

@@ -50,7 +50,6 @@ struct spdk_nvmf_tgt {
 
 	struct spdk_thread			*master_thread;
 
-	uint16_t				next_cntlid;
 	uint64_t				discovery_genctr;
 	TAILQ_HEAD(, spdk_nvmf_subsystem)	subsystems;
 	struct spdk_nvmf_discovery_log_page	*discovery_log_page;
@@ -176,6 +175,7 @@ struct spdk_nvmf_subsystem {
 	uint32_t id;
 	char subnqn[SPDK_NVMF_NQN_MAX_LEN + 1];
 	enum spdk_nvmf_subtype subtype;
+	uint16_t next_cntlid;
 	bool allow_any_host;
 
 	struct spdk_nvmf_tgt			*tgt;
@@ -196,7 +196,6 @@ struct spdk_nvmf_subsystem {
 	TAILQ_ENTRY(spdk_nvmf_subsystem)	entries;
 };
 
-uint16_t spdk_nvmf_tgt_gen_cntlid(struct spdk_nvmf_tgt *tgt);
 struct spdk_nvmf_transport *spdk_nvmf_tgt_get_transport(struct spdk_nvmf_tgt *tgt,
 		enum spdk_nvme_transport_type);
 
@@ -241,6 +240,7 @@ int spdk_nvmf_bdev_ctrlr_identify_ns(struct spdk_bdev *bdev, struct spdk_nvme_ns
 
 int spdk_nvmf_subsystem_bdev_attach(struct spdk_nvmf_subsystem *subsystem);
 void spdk_nvmf_subsystem_bdev_detach(struct spdk_nvmf_subsystem *subsystem);
+uint16_t spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem);
 
 static inline struct spdk_nvmf_ns *
 _spdk_nvmf_subsystem_get_ns(struct spdk_nvmf_subsystem *subsystem, uint32_t nsid)

lib/nvmf/subsystem.c

@@ -117,6 +117,7 @@ spdk_nvmf_create_subsystem(struct spdk_nvmf_tgt *tgt,
 	subsystem->subtype = type;
 	subsystem->max_nsid = num_ns;
 	subsystem->num_allocated_nsid = 0;
+	subsystem->next_cntlid = 0;
 	snprintf(subsystem->subnqn, sizeof(subsystem->subnqn), "%s", nqn);
 	TAILQ_INIT(&subsystem->listeners);
 	TAILQ_INIT(&subsystem->hosts);
@@ -495,3 +496,42 @@ spdk_nvmf_subsystem_get_type(struct spdk_nvmf_subsystem *subsystem)
 {
 	return subsystem->subtype;
 }
+
+uint16_t
+spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem)
+{
+	struct spdk_nvmf_ctrlr *ctrlr;
+	uint16_t count;
+	bool in_use = true;
+
+	/*
+	 * In the worst case, we might have to try all CNTLID values between 1 and 0xFFF0 - 1
+	 * before we find one that is unused (or find that all values are in use).
+	 */
+	count = 0xFFF0 - 1;
+	do {
+		subsystem->next_cntlid++;
+		if (subsystem->next_cntlid >= 0xFFF0) {
+			/* The spec reserves cntlid values in the range FFF0h to FFFFh. */
+			subsystem->next_cntlid = 1;
+		}
+
+		/* Check if a controller with this cntlid currently exists. */
+		in_use = false;
+		TAILQ_FOREACH(ctrlr, &subsystem->ctrlrs, link) {
+			if (ctrlr->cntlid == subsystem->next_cntlid) {
+				in_use = true;
+				break;
+			}
+		}
+
+		count--;
+	} while (in_use && count > 0);
+
+	if (count == 0) {
+		/* All valid cntlid values are in use. */
+		return 0xFFFF;
+	}
+
+	return subsystem->next_cntlid;
+}

test/unit/lib/nvmf/ctrlr.c/ctrlr_ut.c

@@ -45,12 +45,6 @@ spdk_nvmf_tgt_find_subsystem(struct spdk_nvmf_tgt *tgt, const char *subnqn)
 	return NULL;
 }
 
-uint16_t
-spdk_nvmf_tgt_gen_cntlid(struct spdk_nvmf_tgt *tgt)
-{
-	return 0;
-}
-
 const struct spdk_nvme_ctrlr_data *
 spdk_nvme_ctrlr_get_data(struct spdk_nvme_ctrlr *ctrlr)
 {
@@ -121,6 +115,12 @@ spdk_nvmf_subsystem_get_next_ns(struct spdk_nvmf_subsystem *subsystem, struct sp
 	return NULL;
 }
 
+uint16_t
+spdk_nvmf_subsystem_gen_cntlid(struct spdk_nvmf_subsystem *subsystem)
+{
+	return 0;
+}
+
 bool
 spdk_nvmf_ctrlr_dsm_supported(struct spdk_nvmf_ctrlr *ctrlr)
 {