From 46113de13b64abe8b49c69801793ca4db242277b Mon Sep 17 00:00:00 2001 From: Richael Zhuang Date: Sun, 23 Apr 2023 10:42:53 +0800 Subject: [PATCH] nvme_tcp: fix memory leak when resetting controllor In failover test, it reports memory leak about tqpair->stats when detaching a tcp controller and it failover to the other controller. Because during resetting the controller, we disconnect the controller at first and then reconnect. when disconnecting, the adminq is not freed which means the corresponding tqpair and tqpair->stats are not freed. But when reconnecting, nvme_tcp_ctrlr_connect_qpair will allocate memory for tqpair->stats again which causes memory leak. So this patch fix the bug by not reallocating memory for tqpair->stats if it's not NULL. We keep the old stats because from user perspective, the spdk_nvme_qpair is the same one. Besides, when destroying a qpair, the qpair->poll_group is set as NULL which means if qpair->poll_group is not NULL, it should be a new qpair. So there's no need to check if stats is NULL or not if qpair->poll_group is not NULL. So adjusting the if...else... in _nvme_pcie_ctrlr_create_io_qpair. Change-Id: I4108a980aeffe4797e5bca5b1a8ea89f7457162b Signed-off-by: Richael Zhuang Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17718 Tested-by: SPDK CI Jenkins Reviewed-by: Shuhei Matsumoto Reviewed-by: Aleksey Marchuk Reviewed-by: Krzysztof Karas Reviewed-by: Jim Harris --- lib/nvme/nvme_pcie_common.c | 15 +++++++-------- lib/nvme/nvme_tcp.c | 14 ++++++++++---- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/lib/nvme/nvme_pcie_common.c b/lib/nvme/nvme_pcie_common.c index a64a3a278..90f8ce01d 100644 --- a/lib/nvme/nvme_pcie_common.c +++ b/lib/nvme/nvme_pcie_common.c @@ -530,14 +530,14 @@ _nvme_pcie_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme int rc; /* Statistics may already be allocated in the case of controller reset */ - if (!pqpair->stat) { - if (qpair->poll_group) { - struct nvme_pcie_poll_group *group = SPDK_CONTAINEROF(qpair->poll_group, - struct nvme_pcie_poll_group, group); + if (qpair->poll_group) { + struct nvme_pcie_poll_group *group = SPDK_CONTAINEROF(qpair->poll_group, + struct nvme_pcie_poll_group, group); - pqpair->stat = &group->stats; - pqpair->shared_stats = true; - } else { + pqpair->stat = &group->stats; + pqpair->shared_stats = true; + } else { + if (pqpair->stat == NULL) { pqpair->stat = calloc(1, sizeof(*pqpair->stat)); if (!pqpair->stat) { SPDK_ERRLOG("Failed to allocate qpair statistics\n"); @@ -547,7 +547,6 @@ _nvme_pcie_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme } } - rc = nvme_pcie_ctrlr_cmd_create_io_cq(ctrlr, qpair, nvme_completion_create_cq_cb, qpair); if (rc != 0) { diff --git a/lib/nvme/nvme_tcp.c b/lib/nvme/nvme_tcp.c index 2b474bc83..52fdf6f96 100644 --- a/lib/nvme/nvme_tcp.c +++ b/lib/nvme/nvme_tcp.c @@ -2048,10 +2048,16 @@ nvme_tcp_ctrlr_connect_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpa tqpair->stats = &tgroup->stats; tqpair->shared_stats = true; } else { - tqpair->stats = calloc(1, sizeof(*tqpair->stats)); - if (!tqpair->stats) { - SPDK_ERRLOG("tcp stats memory allocation failed\n"); - return -ENOMEM; + /* When resetting a controller, we disconnect adminq and then reconnect. The stats + * is not freed when disconnecting. So when reconnecting, don't allocate memory + * again. + */ + if (tqpair->stats == NULL) { + tqpair->stats = calloc(1, sizeof(*tqpair->stats)); + if (!tqpair->stats) { + SPDK_ERRLOG("tcp stats memory allocation failed\n"); + return -ENOMEM; + } } }