From 6d8f1fc648c96b2a23835141505e0a03aac42f38 Mon Sep 17 00:00:00 2001 From: Jacek Kalwas Date: Sat, 15 Feb 2020 07:19:24 +0100 Subject: [PATCH] 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 Change-Id: I4e9d0e0c5eee2d570ec4ac9079270c32d5afb8db Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/626 Tested-by: SPDK CI Jenkins Reviewed-by: Aleksey Marchuk Reviewed-by: Shuhei Matsumoto --- include/spdk/nvmf_transport.h | 12 ++- lib/nvmf/ctrlr_discovery.c | 2 +- lib/nvmf/fc.c | 3 +- lib/nvmf/nvmf_internal.h | 5 +- lib/nvmf/rdma.c | 78 ++++++------------- lib/nvmf/subsystem.c | 19 +++-- lib/nvmf/tcp.c | 50 ++++-------- lib/nvmf/transport.c | 48 +++++++++++- .../ctrlr_discovery.c/ctrlr_discovery_ut.c | 9 +++ 9 files changed, 120 insertions(+), 106 deletions(-) diff --git a/include/spdk/nvmf_transport.h b/include/spdk/nvmf_transport.h index dc43b1d2c..a6ca0407b 100644 --- a/include/spdk/nvmf_transport.h +++ b/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, - const struct spdk_nvme_transport_id *trid); + void (*stop_listen)(struct spdk_nvmf_transport *transport, + const struct spdk_nvme_transport_id *trid); /** * Check for new connections on the transport. diff --git a/lib/nvmf/ctrlr_discovery.c b/lib/nvmf/ctrlr_discovery.c index f5aa252b7..abd546619 100644 --- a/lib/nvmf/ctrlr_discovery.c +++ b/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++; } diff --git a/lib/nvmf/fc.c b/lib/nvmf/fc.c index 26f88ba00..ca9093756 100644 --- a/lib/nvmf/fc.c +++ b/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 diff --git a/lib/nvmf/nvmf_internal.h b/lib/nvmf/nvmf_internal.h index 8d6156a28..282db49ec 100644 --- a/lib/nvmf/nvmf_internal.h +++ b/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); diff --git a/lib/nvmf/rdma.c b/lib/nvmf/rdma.c index b080aa0c1..a31c9bafd 100644 --- a/lib/nvmf/rdma.c +++ b/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 - * 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); + port->trid = trid; - 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) { - assert(port->ref > 0); - port->ref--; - if (port->ref == 0) { - TAILQ_REMOVE(&rtransport->ports, port, link); - rdma_destroy_id(port->id); - free(port); - } + if (spdk_nvme_transport_id_compare(port->trid, trid) == 0) { + 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); - } - for (i = 0; i < ref; i++) { - spdk_nvmf_rdma_listen(transport, &trid, NULL, NULL); - } + spdk_nvmf_rdma_stop_listen(transport, trid); + 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++) { - spdk_nvmf_rdma_stop_listen(transport, &port->trid); + while (spdk_nvmf_transport_stop_listen(transport, port->trid) == 0) { + ; } } diff --git a/lib/nvmf/subsystem.c b/lib/nvmf/subsystem.c index 9dc50995c..512d63873 100644 --- a/lib/nvmf/subsystem.c +++ b/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 diff --git a/lib/nvmf/tcp.c b/lib/nvmf/tcp.c index 5cba3f5db..3765b5198 100644 --- a/lib/nvmf/tcp.c +++ b/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) { - SPDK_ERRLOG("Invalid traddr %s / trsvcid %s\n", - trid->traddr, trid->trsvcid); - free(port); - pthread_mutex_unlock(&ttransport->lock); - return -ENOMEM; - } - + port->trid = trid; 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); - port->ref--; - if (port->ref == 0) { - 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; + TAILQ_REMOVE(&ttransport->ports, port, link); + spdk_sock_close(&port->listen_sock); + free(port); } - 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); diff --git a/lib/nvmf/transport.c b/lib/nvmf/transport.c index 64c4606e0..ed23c1e87 100644 --- a/lib/nvmf/transport.c +++ b/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 diff --git a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c b/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c index 5683a372a..57c137d75 100644 --- a/test/unit/lib/nvmf/ctrlr_discovery.c/ctrlr_discovery_ut.c +++ b/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,