Commit df4600f4 authored by Richael Zhuang's avatar Richael Zhuang Committed by Konrad Sztyber
Browse files

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: default avatarRichael Zhuang <richael.zhuang@arm.com>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/17718


Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Reviewed-by: default avatarAleksey Marchuk <alexeymar@nvidia.com>
Reviewed-by: default avatarKrzysztof Karas <krzysztof.karas@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
parent cb97b860

lib/nvme/nvme_pcie_common.c

+7 −8
Original line number Diff line number Diff line
@@ -530,7 +530,6 @@ _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);
@@ -538,6 +537,7 @@ _nvme_pcie_ctrlr_create_io_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme 
		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) {

lib/nvme/nvme_tcp.c

+10 −4
Original line number Diff line number Diff line
@@ -2048,12 +2048,18 @@ nvme_tcp_ctrlr_connect_qpair(struct spdk_nvme_ctrlr *ctrlr, struct spdk_nvme_qpa 
		tqpair->stats = &tgroup->stats;

		tqpair->shared_stats = true;

	} else {

		/* 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;

			}

		}

	}



	tqpair->maxr2t = NVME_TCP_MAX_R2T_DEFAULT;

	/* Explicitly set the state and recv_state of tqpair */