nvmf: remove redundant trid obj copy

It is memory optimisation as transport id is 'heavy'. As a side effect
simpler handling of listen and stop_listen on transport specific layer.

Signed-off-by: Jacek Kalwas <jacek.kalwas@intel.com>
Change-Id: I4e9d0e0c5eee2d570ec4ac9079270c32d5afb8db
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/626
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Aleksey Marchuk <alexeymar@mellanox.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>

include/spdk/nvmf_transport.h

@@ -159,6 +159,13 @@ struct spdk_nvmf_poll_group {
 	struct spdk_nvmf_poll_group_stat		stat;
 };
 
+struct spdk_nvmf_listener {
+	struct spdk_nvme_transport_id	trid;
+	uint32_t			ref;
+
+	TAILQ_ENTRY(spdk_nvmf_listener)	link;
+};
+
 struct spdk_nvmf_transport {
 	struct spdk_nvmf_tgt			*tgt;
 	const struct spdk_nvmf_transport_ops	*ops;
@@ -167,6 +174,7 @@ struct spdk_nvmf_transport {
 	/* A mempool for transport related data transfers */
 	struct spdk_mempool			*data_buf_pool;
 
+	TAILQ_HEAD(, spdk_nvmf_listener)	listeners;
 	TAILQ_ENTRY(spdk_nvmf_transport)	link;
 };
 
@@ -208,8 +216,8 @@ struct spdk_nvmf_transport_ops {
 	/**
 	  * Stop accepting new connections at the given address.
 	  */
-	int (*stop_listen)(struct spdk_nvmf_transport *transport,
+	void (*stop_listen)(struct spdk_nvmf_transport *transport,
-			   const struct spdk_nvme_transport_id *trid);
+			    const struct spdk_nvme_transport_id *trid);
 
 	/**
 	 * Check for new connections on the transport.

lib/nvmf/ctrlr_discovery.c

@@ -106,7 +106,7 @@ nvmf_generate_discovery_log(struct spdk_nvmf_tgt *tgt, const char *hostnqn, size
 			entry->subtype = subsystem->subtype;
 			snprintf(entry->subnqn, sizeof(entry->subnqn), "%s", subsystem->subnqn);
 
-			spdk_nvmf_transport_listener_discover(listener->transport, &listener->trid, entry);
+			spdk_nvmf_transport_listener_discover(listener->transport, listener->trid, entry);
 
 			numrec++;
 		}

lib/nvmf/fc.c

@@ -1930,11 +1930,10 @@ nvmf_fc_listen(struct spdk_nvmf_transport *transport,
 	return 0;
 }
 
-static int
+static void
 nvmf_fc_stop_listen(struct spdk_nvmf_transport *transport,
 		    const struct spdk_nvme_transport_id *_trid)
 {
-	return 0;
 }
 
 static void

lib/nvmf/nvmf_internal.h

@@ -81,7 +81,7 @@ struct spdk_nvmf_host {
 };
 
 struct spdk_nvmf_subsystem_listener {
-	struct spdk_nvme_transport_id			trid;
+	struct spdk_nvme_transport_id			*trid;
 	struct spdk_nvmf_transport			*transport;
 	TAILQ_ENTRY(spdk_nvmf_subsystem_listener)	link;
 };
@@ -318,6 +318,9 @@ struct spdk_nvmf_ctrlr *spdk_nvmf_subsystem_get_ctrlr(struct spdk_nvmf_subsystem
 struct spdk_nvmf_subsystem_listener *spdk_nvmf_subsystem_find_listener(
 	struct spdk_nvmf_subsystem *subsystem,
 	const struct spdk_nvme_transport_id *trid);
+struct spdk_nvmf_listener *spdk_nvmf_transport_find_listener(
+	struct spdk_nvmf_transport *transport,
+	const struct spdk_nvme_transport_id *trid);
 
 int spdk_nvmf_ctrlr_async_event_ns_notice(struct spdk_nvmf_ctrlr *ctrlr);
 void spdk_nvmf_ctrlr_async_event_reservation_notification(struct spdk_nvmf_ctrlr *ctrlr);

lib/nvmf/rdma.c

@@ -489,10 +489,9 @@ struct spdk_nvmf_rdma_device {
 };
 
 struct spdk_nvmf_rdma_port {
-	struct spdk_nvme_transport_id		trid;
+	const struct spdk_nvme_transport_id	*trid;
 	struct rdma_cm_id			*id;
 	struct spdk_nvmf_rdma_device		*device;
-	uint32_t				ref;
 	TAILQ_ENTRY(spdk_nvmf_rdma_port)	link;
 };
 
@@ -2660,12 +2659,6 @@ spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
 	assert(rtransport->event_channel != NULL);
 
 	pthread_mutex_lock(&rtransport->lock);
-	TAILQ_FOREACH(port, &rtransport->ports, link) {
-		if (spdk_nvme_transport_id_compare(&port->trid, trid) == 0) {
-			goto success;
-		}
-	}
-
 	port = calloc(1, sizeof(*port));
 	if (!port) {
 		SPDK_ERRLOG("Port allocation failed\n");
@@ -2673,15 +2666,9 @@ spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
 		return -ENOMEM;
 	}
 
-	/* Selectively copy the trid. Things like NQN don't matter here - that
+	port->trid = trid;
-	 * mapping is enforced elsewhere.
-	 */
-	spdk_nvme_trid_populate_transport(&port->trid, SPDK_NVME_TRANSPORT_RDMA);
-	port->trid.adrfam = trid->adrfam;
-	snprintf(port->trid.traddr, sizeof(port->trid.traddr), "%s", trid->traddr);
-	snprintf(port->trid.trsvcid, sizeof(port->trid.trsvcid), "%s", trid->trsvcid);
 
-	switch (port->trid.adrfam) {
+	switch (trid->adrfam) {
 	case SPDK_NVMF_ADRFAM_IPV4:
 		family = AF_INET;
 		break;
@@ -2689,7 +2676,7 @@ spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
 		family = AF_INET6;
 		break;
 	default:
-		SPDK_ERRLOG("Unhandled ADRFAM %d\n", port->trid.adrfam);
+		SPDK_ERRLOG("Unhandled ADRFAM %d\n", trid->adrfam);
 		free(port);
 		pthread_mutex_unlock(&rtransport->lock);
 		return -EINVAL;
@@ -2701,7 +2688,7 @@ spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
 	hints.ai_socktype = SOCK_STREAM;
 	hints.ai_protocol = 0;
 
-	rc = getaddrinfo(port->trid.traddr, port->trid.trsvcid, &hints, &res);
+	rc = getaddrinfo(trid->traddr, trid->trsvcid, &hints, &res);
 	if (rc) {
 		SPDK_ERRLOG("getaddrinfo failed: %s (%d)\n", gai_strerror(rc), rc);
 		free(port);
@@ -2765,50 +2752,35 @@ spdk_nvmf_rdma_listen(struct spdk_nvmf_transport *transport,
 		       trid->traddr, trid->trsvcid);
 
 	TAILQ_INSERT_TAIL(&rtransport->ports, port, link);
-
-success:
-	port->ref++;
 	pthread_mutex_unlock(&rtransport->lock);
+
 	if (cb_fn != NULL) {
 		cb_fn(cb_arg, 0);
 	}
+
 	return 0;
 }
 
-static int
+static void
 spdk_nvmf_rdma_stop_listen(struct spdk_nvmf_transport *transport,
-			   const struct spdk_nvme_transport_id *_trid)
+			   const struct spdk_nvme_transport_id *trid)
 {
 	struct spdk_nvmf_rdma_transport *rtransport;
 	struct spdk_nvmf_rdma_port *port, *tmp;
-	struct spdk_nvme_transport_id trid = {};
 
 	rtransport = SPDK_CONTAINEROF(transport, struct spdk_nvmf_rdma_transport, transport);
 
-	/* Selectively copy the trid. Things like NQN don't matter here - that
-	 * mapping is enforced elsewhere.
-	 */
-	spdk_nvme_trid_populate_transport(&trid, SPDK_NVME_TRANSPORT_RDMA);
-	trid.adrfam = _trid->adrfam;
-	snprintf(trid.traddr, sizeof(port->trid.traddr), "%s", _trid->traddr);
-	snprintf(trid.trsvcid, sizeof(port->trid.trsvcid), "%s", _trid->trsvcid);
-
 	pthread_mutex_lock(&rtransport->lock);
 	TAILQ_FOREACH_SAFE(port, &rtransport->ports, link, tmp) {
-		if (spdk_nvme_transport_id_compare(&port->trid, &trid) == 0) {
+		if (spdk_nvme_transport_id_compare(port->trid, trid) == 0) {
-			assert(port->ref > 0);
+			TAILQ_REMOVE(&rtransport->ports, port, link);
-			port->ref--;
+			rdma_destroy_id(port->id);
-			if (port->ref == 0) {
+			free(port);
-				TAILQ_REMOVE(&rtransport->ports, port, link);
-				rdma_destroy_id(port->id);
-				free(port);
-			}
 			break;
 		}
 	}
 
 	pthread_mutex_unlock(&rtransport->lock);
-	return 0;
 }
 
 static void
@@ -3005,33 +2977,29 @@ static bool
 nvmf_rdma_handle_cm_event_addr_change(struct spdk_nvmf_transport *transport,
 				      struct rdma_cm_event *event)
 {
-	struct spdk_nvme_transport_id		trid;
+	const struct spdk_nvme_transport_id	*trid;
 	struct spdk_nvmf_rdma_port		*port;
 	struct spdk_nvmf_rdma_transport		*rtransport;
-	uint32_t				ref, i;
 	bool					event_acked = false;
 
 	rtransport = SPDK_CONTAINEROF(transport, struct spdk_nvmf_rdma_transport, transport);
 	TAILQ_FOREACH(port, &rtransport->ports, link) {
 		if (port->id == event->id) {
-			SPDK_ERRLOG("ADDR_CHANGE: IP %s:%s migrated\n", port->trid.traddr, port->trid.trsvcid);
+			SPDK_ERRLOG("ADDR_CHANGE: IP %s:%s migrated\n", port->trid->traddr, port->trid->trsvcid);
 			rdma_ack_cm_event(event);
 			event_acked = true;
 			trid = port->trid;
-			ref = port->ref;
 			break;
 		}
 	}
+
 	if (event_acked) {
 		nvmf_rdma_disconnect_qpairs_on_port(rtransport, port);
 
-		for (i = 0; i < ref; i++) {
+		spdk_nvmf_rdma_stop_listen(transport, trid);
-			spdk_nvmf_rdma_stop_listen(transport, &trid);
+		spdk_nvmf_rdma_listen(transport, trid, NULL, NULL);
-		}
-		for (i = 0; i < ref; i++) {
-			spdk_nvmf_rdma_listen(transport, &trid, NULL, NULL);
-		}
 	}
+
 	return event_acked;
 }
 
@@ -3041,20 +3009,18 @@ nvmf_rdma_handle_cm_event_port_removal(struct spdk_nvmf_transport *transport,
 {
 	struct spdk_nvmf_rdma_port		*port;
 	struct spdk_nvmf_rdma_transport		*rtransport;
-	uint32_t				ref, i;
 
 	port = event->id->context;
 	rtransport = SPDK_CONTAINEROF(transport, struct spdk_nvmf_rdma_transport, transport);
-	ref = port->ref;
 
-	SPDK_NOTICELOG("Port %s:%s is being removed\n", port->trid.traddr, port->trid.trsvcid);
+	SPDK_NOTICELOG("Port %s:%s is being removed\n", port->trid->traddr, port->trid->trsvcid);
 
 	nvmf_rdma_disconnect_qpairs_on_port(rtransport, port);
 
 	rdma_ack_cm_event(event);
 
-	for (i = 0; i < ref; i++) {
+	while (spdk_nvmf_transport_stop_listen(transport, port->trid) == 0) {
-		spdk_nvmf_rdma_stop_listen(transport, &port->trid);
+		;
 	}
 }
 

lib/nvmf/subsystem.c

@@ -320,9 +320,9 @@ _nvmf_subsystem_remove_listener(struct spdk_nvmf_subsystem *subsystem,
 	struct spdk_nvmf_transport *transport;
 
 	if (stop) {
-		transport = spdk_nvmf_tgt_get_transport(subsystem->tgt, listener->trid.trstring);
+		transport = spdk_nvmf_tgt_get_transport(subsystem->tgt, listener->trid->trstring);
 		if (transport != NULL) {
-			spdk_nvmf_transport_stop_listen(transport, &listener->trid);
+			spdk_nvmf_transport_stop_listen(transport, listener->trid);
 		}
 	}
 
@@ -739,7 +739,7 @@ spdk_nvmf_subsystem_find_listener(struct spdk_nvmf_subsystem *subsystem,
 	struct spdk_nvmf_subsystem_listener *listener;
 
 	TAILQ_FOREACH(listener, &subsystem->listeners, link) {
-		if (spdk_nvme_transport_id_compare(&listener->trid, trid) == 0) {
+		if (spdk_nvme_transport_id_compare(listener->trid, trid) == 0) {
 			return listener;
 		}
 	}
@@ -753,6 +753,7 @@ spdk_nvmf_subsystem_add_listener(struct spdk_nvmf_subsystem *subsystem,
 {
 	struct spdk_nvmf_transport *transport;
 	struct spdk_nvmf_subsystem_listener *listener;
+	struct spdk_nvmf_listener *tr_listener;
 
 	if (!(subsystem->state == SPDK_NVMF_SUBSYSTEM_INACTIVE ||
 	      subsystem->state == SPDK_NVMF_SUBSYSTEM_PAUSED)) {
@@ -770,12 +771,18 @@ spdk_nvmf_subsystem_add_listener(struct spdk_nvmf_subsystem *subsystem,
 		return -EINVAL;
 	}
 
+	tr_listener = spdk_nvmf_transport_find_listener(transport, trid);
+	if (!tr_listener) {
+		SPDK_ERRLOG("Cannot find transport listener for %s\n", trid->traddr);
+		return -EINVAL;
+	}
+
 	listener = calloc(1, sizeof(*listener));
 	if (!listener) {
 		return -ENOMEM;
 	}
 
-	listener->trid = *trid;
+	listener->trid = &tr_listener->trid;
 	listener->transport = transport;
 
 	TAILQ_INSERT_HEAD(&subsystem->listeners, listener, link);
@@ -827,7 +834,7 @@ spdk_nvmf_subsystem_listener_allowed(struct spdk_nvmf_subsystem *subsystem,
 	}
 
 	TAILQ_FOREACH(listener, &subsystem->listeners, link) {
-		if (spdk_nvme_transport_id_compare(&listener->trid, trid) == 0) {
+		if (spdk_nvme_transport_id_compare(listener->trid, trid) == 0) {
 			return true;
 		}
 	}
@@ -851,7 +858,7 @@ spdk_nvmf_subsystem_get_next_listener(struct spdk_nvmf_subsystem *subsystem,
 const struct spdk_nvme_transport_id *
 spdk_nvmf_subsystem_listener_get_trid(struct spdk_nvmf_subsystem_listener *listener)
 {
-	return &listener->trid;
+	return listener->trid;
 }
 
 void

lib/nvmf/tcp.c

@@ -267,9 +267,8 @@ struct spdk_nvmf_tcp_poll_group {
 };
 
 struct spdk_nvmf_tcp_port {
-	struct spdk_nvme_transport_id		trid;
+	const struct spdk_nvme_transport_id	*trid;
 	struct spdk_sock			*listen_sock;
-	uint32_t				ref;
 	TAILQ_ENTRY(spdk_nvmf_tcp_port)		link;
 };
 
@@ -588,7 +587,7 @@ _spdk_nvmf_tcp_find_port(struct spdk_nvmf_tcp_transport *ttransport,
 	}
 
 	TAILQ_FOREACH(port, &ttransport->ports, link) {
-		if (spdk_nvme_transport_id_compare(&canon_trid, &port->trid) == 0) {
+		if (spdk_nvme_transport_id_compare(&canon_trid, port->trid) == 0) {
 			return port;
 		}
 	}
@@ -616,11 +615,6 @@ spdk_nvmf_tcp_listen(struct spdk_nvmf_transport *transport,
 	}
 
 	pthread_mutex_lock(&ttransport->lock);
-	port = _spdk_nvmf_tcp_find_port(ttransport, trid);
-	if (port) {
-		goto success;
-	}
-
 	port = calloc(1, sizeof(*port));
 	if (!port) {
 		SPDK_ERRLOG("Port allocation failed\n");
@@ -628,14 +622,7 @@ spdk_nvmf_tcp_listen(struct spdk_nvmf_transport *transport,
 		return -ENOMEM;
 	}
 
-	if (_spdk_nvmf_tcp_canon_listen_trid(&port->trid, trid) != 0) {
+	port->trid = trid;
-		SPDK_ERRLOG("Invalid traddr %s / trsvcid %s\n",
-			    trid->traddr, trid->trsvcid);
-		free(port);
-		pthread_mutex_unlock(&ttransport->lock);
-		return -ENOMEM;
-	}
-
 	port->listen_sock = spdk_sock_listen(trid->traddr, trsvcid_int, NULL);
 	if (port->listen_sock == NULL) {
 		SPDK_ERRLOG("spdk_sock_listen(%s, %d) failed: %s (%d)\n",
@@ -667,21 +654,21 @@ spdk_nvmf_tcp_listen(struct spdk_nvmf_transport *transport,
 		       trid->traddr, trid->trsvcid);
 
 	TAILQ_INSERT_TAIL(&ttransport->ports, port, link);
-
-success:
-	port->ref++;
 	pthread_mutex_unlock(&ttransport->lock);
-	cb_fn(cb_arg, 0);
+
+	if (cb_fn != NULL) {
+		cb_fn(cb_arg, 0);
+	}
+
 	return 0;
 }
 
-static int
+static void
 spdk_nvmf_tcp_stop_listen(struct spdk_nvmf_transport *transport,
 			  const struct spdk_nvme_transport_id *trid)
 {
 	struct spdk_nvmf_tcp_transport *ttransport;
 	struct spdk_nvmf_tcp_port *port;
-	int rc;
 
 	ttransport = SPDK_CONTAINEROF(transport, struct spdk_nvmf_tcp_transport, transport);
 
@@ -691,21 +678,12 @@ spdk_nvmf_tcp_stop_listen(struct spdk_nvmf_transport *transport,
 	pthread_mutex_lock(&ttransport->lock);
 	port = _spdk_nvmf_tcp_find_port(ttransport, trid);
 	if (port) {
-		assert(port->ref > 0);
+		TAILQ_REMOVE(&ttransport->ports, port, link);
-		port->ref--;
+		spdk_sock_close(&port->listen_sock);
-		if (port->ref == 0) {
+		free(port);
-			TAILQ_REMOVE(&ttransport->ports, port, link);
-			spdk_sock_close(&port->listen_sock);
-			free(port);
-		}
-		rc = 0;
-	} else {
-		SPDK_DEBUGLOG(SPDK_LOG_NVMF_TCP, "Port not found\n");
-		rc = -ENOENT;
 	}
-	pthread_mutex_unlock(&ttransport->lock);
 
-	return rc;
+	pthread_mutex_unlock(&ttransport->lock);
 }
 
 static void spdk_nvmf_tcp_qpair_set_recv_state(struct spdk_nvmf_tcp_qpair *tqpair,
@@ -919,7 +897,7 @@ _spdk_nvmf_tcp_handle_connect(struct spdk_nvmf_transport *transport,
 	int rc;
 
 	SPDK_DEBUGLOG(SPDK_LOG_NVMF_TCP, "New connection accepted on %s port %s\n",
-		      port->trid.traddr, port->trid.trsvcid);
+		      port->trid->traddr, port->trid->trsvcid);
 
 	if (transport->opts.sock_priority) {
 		rc = spdk_sock_set_priority(sock, transport->opts.sock_priority);

lib/nvmf/transport.c

@@ -126,6 +126,8 @@ spdk_nvmf_transport_create(const char *transport_name, struct spdk_nvmf_transpor
 		return NULL;
 	}
 
+	TAILQ_INIT(&transport->listeners);
+
 	transport->ops = ops;
 	transport->opts = *opts;
 	chars_written = snprintf(spdk_mempool_name, MAX_MEMPOOL_NAME_LENGTH, "%s_%s_%s", "spdk_nvmf",
@@ -180,20 +182,62 @@ spdk_nvmf_transport_destroy(struct spdk_nvmf_transport *transport)
 	return transport->ops->destroy(transport);
 }
 
+struct spdk_nvmf_listener *
+spdk_nvmf_transport_find_listener(struct spdk_nvmf_transport *transport,
+				  const struct spdk_nvme_transport_id *trid)
+{
+	struct spdk_nvmf_listener *listener;
+
+	TAILQ_FOREACH(listener, &transport->listeners, link) {
+		if (spdk_nvme_transport_id_compare(&listener->trid, trid) == 0) {
+			return listener;
+		}
+	}
+
+	return NULL;
+}
+
 int
 spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport,
 			   const struct spdk_nvme_transport_id *trid,
 			   spdk_nvmf_tgt_listen_done_fn cb_fn,
 			   void *cb_arg)
 {
-	return transport->ops->listen(transport, trid, cb_fn, cb_arg);
+	struct spdk_nvmf_listener *listener = spdk_nvmf_transport_find_listener(transport, trid);
+	if (!listener) {
+		listener = calloc(1, sizeof(*listener));
+		if (!listener) {
+			return -ENOMEM;
+		}
+
+		listener->ref = 1;
+		listener->trid = *trid;
+		TAILQ_INSERT_TAIL(&transport->listeners, listener, link);
+		return transport->ops->listen(transport, &listener->trid, cb_fn, cb_arg);
+	}
+
+	++listener->ref;
+
+	cb_fn(cb_arg, 0);
+	return 0;
 }
 
 int
 spdk_nvmf_transport_stop_listen(struct spdk_nvmf_transport *transport,
 				const struct spdk_nvme_transport_id *trid)
 {
-	return transport->ops->stop_listen(transport, trid);
+	struct spdk_nvmf_listener *listener = spdk_nvmf_transport_find_listener(transport, trid);
+	if (!listener) {
+		return -ENOENT;
+	}
+
+	if (--listener->ref == 0) {
+		TAILQ_REMOVE(&transport->listeners, listener, link);
+		transport->ops->stop_listen(transport, trid);
+		free(listener);
+	}
+
+	return 0;
 }
 
 void

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

@@ -86,6 +86,15 @@ spdk_nvmf_transport_listen(struct spdk_nvmf_transport *transport,
 	return 0;
 }
 
+static struct spdk_nvmf_listener g_listener = {};
+
+struct spdk_nvmf_listener *
+spdk_nvmf_transport_find_listener(struct spdk_nvmf_transport *transport,
+				  const struct spdk_nvme_transport_id *trid)
+{
+	return &g_listener;
+}
+
 void
 spdk_nvmf_transport_listener_discover(struct spdk_nvmf_transport *transport,
 				      struct spdk_nvme_transport_id *trid,