[dpdk-dev,1/2] event/sw: fix queue memory leak and multi-link bug

Message ID 1512011314-19682-1-git-send-email-gage.eads@intel.com
State Accepted, archived
Delegated to: Jerin Jacob
Headers show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Eads, Gage Nov. 30, 2017, 3:08 a.m.
This commit reinitializes a queue before it is reconfigured, such that
reorder buffer memory is not leaked.

This bug masked a few other problems, which this commit corrects as well:
- sw_port_link() allowed a port to link to a queue twice, such that the
  port could then successfully unlink the queue twice. Now the link
  function checks whether a port is already linked to the queue, and if so
  returns success but doesn't assign the a port a second slot in the
  queue's cq map.
- test_eventdev.c's test_eventdev_unlink() was unlinking a queue twice
  from the same port, and expecting the second unlink to succeed. Now the
  test unlinks, links, then unlinks again.
- test_eventdev.c's test_eventdev_link_get() was linking a single queue but
  expecting the unlink function to return nb_queues (where nb_queues > 1).
  The test now checks for a return value of 1.

Fixes: 5ffb2f142d95 ("event/sw: support event queues")
Fixes: 371a688fc159 ("event/sw: support linking queues to ports")
Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")

Signed-off-by: Gage Eads <gage.eads@intel.com>
---
 drivers/event/sw/sw_evdev.c | 46 +++++++++++++++++++++++++++++----------------
 test/test/test_eventdev.c   |  7 +++++--
 2 files changed, 35 insertions(+), 18 deletions(-)

Comments

Van Haaren, Harry Dec. 7, 2017, 5:15 p.m. | #1
> From: Eads, Gage
> Sent: Thursday, November 30, 2017 3:09 AM
> To: dev@dpdk.org
> Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com; nipun.gupta@nxp.com;
> santosh.shukla@caviumnetworks.com; pbhagavatula@caviumnetworks.com
> Subject: [PATCH 1/2] event/sw: fix queue memory leak and multi-link bug
> 
> This commit reinitializes a queue before it is reconfigured, such that
> reorder buffer memory is not leaked.
> 
> This bug masked a few other problems, which this commit corrects as well:
> - sw_port_link() allowed a port to link to a queue twice, such that the
>   port could then successfully unlink the queue twice. Now the link
>   function checks whether a port is already linked to the queue, and if so
>   returns success but doesn't assign the a port a second slot in the
>   queue's cq map.
> - test_eventdev.c's test_eventdev_unlink() was unlinking a queue twice
>   from the same port, and expecting the second unlink to succeed. Now the
>   test unlinks, links, then unlinks again.
> - test_eventdev.c's test_eventdev_link_get() was linking a single queue but
>   expecting the unlink function to return nb_queues (where nb_queues > 1).
>   The test now checks for a return value of 1.
> 
> Fixes: 5ffb2f142d95 ("event/sw: support event queues")
> Fixes: 371a688fc159 ("event/sw: support linking queues to ports")
> Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>

Looks good to me,

Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
Jerin Jacob Dec. 9, 2017, 9:26 a.m. | #2
-----Original Message-----
> Date: Thu, 7 Dec 2017 17:15:16 +0000
> From: "Van Haaren, Harry" <harry.van.haaren@intel.com>
> To: "Eads, Gage" <gage.eads@intel.com>, "dev@dpdk.org" <dev@dpdk.org>
> CC: "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
>  "Richardson, Bruce" <bruce.richardson@intel.com>, "hemant.agrawal@nxp.com"
>  <hemant.agrawal@nxp.com>, "nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
>  "santosh.shukla@caviumnetworks.com" <santosh.shukla@caviumnetworks.com>,
>  "pbhagavatula@caviumnetworks.com" <pbhagavatula@caviumnetworks.com>
> Subject: RE: [PATCH 1/2] event/sw: fix queue memory leak and multi-link bug
> 
> > From: Eads, Gage
> > Sent: Thursday, November 30, 2017 3:09 AM
> > To: dev@dpdk.org
> > Cc: jerin.jacob@caviumnetworks.com; Van Haaren, Harry
> > <harry.van.haaren@intel.com>; Richardson, Bruce
> > <bruce.richardson@intel.com>; hemant.agrawal@nxp.com; nipun.gupta@nxp.com;
> > santosh.shukla@caviumnetworks.com; pbhagavatula@caviumnetworks.com
> > Subject: [PATCH 1/2] event/sw: fix queue memory leak and multi-link bug
> > 
> > This commit reinitializes a queue before it is reconfigured, such that
> > reorder buffer memory is not leaked.
> > 
> > This bug masked a few other problems, which this commit corrects as well:
> > - sw_port_link() allowed a port to link to a queue twice, such that the
> >   port could then successfully unlink the queue twice. Now the link
> >   function checks whether a port is already linked to the queue, and if so
> >   returns success but doesn't assign the a port a second slot in the
> >   queue's cq map.
> > - test_eventdev.c's test_eventdev_unlink() was unlinking a queue twice
> >   from the same port, and expecting the second unlink to succeed. Now the
> >   test unlinks, links, then unlinks again.
> > - test_eventdev.c's test_eventdev_link_get() was linking a single queue but
> >   expecting the unlink function to return nb_queues (where nb_queues > 1).
> >   The test now checks for a return value of 1.
> > 
> > Fixes: 5ffb2f142d95 ("event/sw: support event queues")
> > Fixes: 371a688fc159 ("event/sw: support linking queues to ports")
> > Fixes: f8f9d233ea0e ("test/eventdev: add unit tests")
> > 
> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> 
> Looks good to me,
> 
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>

Cc: stable@dpdk.org
Applied to dpdk-next-eventdev/master. Thanks.

Patch

diff --git a/drivers/event/sw/sw_evdev.c b/drivers/event/sw/sw_evdev.c
index aed8b72..5d03b7b 100644
--- a/drivers/event/sw/sw_evdev.c
+++ b/drivers/event/sw/sw_evdev.c
@@ -63,6 +63,7 @@  sw_port_link(struct rte_eventdev *dev, void *port, const uint8_t queues[],
 	RTE_SET_USED(priorities);
 	for (i = 0; i < num; i++) {
 		struct sw_qid *q = &sw->qids[queues[i]];
+		unsigned int j;
 
 		/* check for qid map overflow */
 		if (q->cq_num_mapped_cqs >= RTE_DIM(q->cq_map)) {
@@ -75,6 +76,15 @@  sw_port_link(struct rte_eventdev *dev, void *port, const uint8_t queues[],
 			break;
 		}
 
+		for (j = 0; j < q->cq_num_mapped_cqs; j++) {
+			if (q->cq_map[j] == p->id)
+				break;
+		}
+
+		/* check if port is already linked */
+		if (j < q->cq_num_mapped_cqs)
+			continue;
+
 		if (q->type == SW_SCHED_TYPE_DIRECT) {
 			/* check directed qids only map to one port */
 			if (p->num_qids_mapped > 0) {
@@ -339,6 +349,23 @@  qid_init(struct sw_evdev *sw, unsigned int idx, int type,
 	return -EINVAL;
 }
 
+static void
+sw_queue_release(struct rte_eventdev *dev, uint8_t id)
+{
+	struct sw_evdev *sw = sw_pmd_priv(dev);
+	struct sw_qid *qid = &sw->qids[id];
+	uint32_t i;
+
+	for (i = 0; i < SW_IQS_MAX; i++)
+		iq_ring_destroy(qid->iq[i]);
+
+	if (qid->type == RTE_SCHED_TYPE_ORDERED) {
+		rte_free(qid->reorder_buffer);
+		rte_ring_free(qid->reorder_buffer_freelist);
+	}
+	memset(qid, 0, sizeof(*qid));
+}
+
 static int
 sw_queue_setup(struct rte_eventdev *dev, uint8_t queue_id,
 		const struct rte_event_queue_conf *conf)
@@ -370,24 +397,11 @@  sw_queue_setup(struct rte_eventdev *dev, uint8_t queue_id,
 	}
 
 	struct sw_evdev *sw = sw_pmd_priv(dev);
-	return qid_init(sw, queue_id, type, conf);
-}
-
-static void
-sw_queue_release(struct rte_eventdev *dev, uint8_t id)
-{
-	struct sw_evdev *sw = sw_pmd_priv(dev);
-	struct sw_qid *qid = &sw->qids[id];
-	uint32_t i;
 
-	for (i = 0; i < SW_IQS_MAX; i++)
-		iq_ring_destroy(qid->iq[i]);
+	if (sw->qids[queue_id].initialized)
+		sw_queue_release(dev, queue_id);
 
-	if (qid->type == RTE_SCHED_TYPE_ORDERED) {
-		rte_free(qid->reorder_buffer);
-		rte_ring_free(qid->reorder_buffer_freelist);
-	}
-	memset(qid, 0, sizeof(*qid));
+	return qid_init(sw, queue_id, type, conf);
 }
 
 static void
diff --git a/test/test/test_eventdev.c b/test/test/test_eventdev.c
index d6ade78..2a55a7a 100644
--- a/test/test/test_eventdev.c
+++ b/test/test/test_eventdev.c
@@ -840,6 +840,9 @@  test_eventdev_unlink(void)
 	for (i = 0; i < nb_queues; i++)
 		queues[i] = i;
 
+	ret = rte_event_port_link(TEST_DEV_ID, 0, NULL, NULL, 0);
+	TEST_ASSERT(ret >= 0, "Failed to link with NULL device%d",
+				 TEST_DEV_ID);
 
 	ret = rte_event_port_unlink(TEST_DEV_ID, 0, queues, nb_queues);
 	TEST_ASSERT(ret == nb_queues, "Failed to unlink(device%d) ret=%d",
@@ -900,9 +903,9 @@  test_eventdev_link_get(void)
 	ret = rte_event_port_links_get(TEST_DEV_ID, 0, queues, priorities);
 	TEST_ASSERT(ret == 1, "(%d)Wrong link get ret=%d expected=%d",
 					TEST_DEV_ID, ret, 1);
-	/* unlink all*/
+	/* unlink the queue */
 	ret = rte_event_port_unlink(TEST_DEV_ID, 0, NULL, 0);
-	TEST_ASSERT(ret == nb_queues, "Failed to unlink(device%d) ret=%d",
+	TEST_ASSERT(ret == 1, "Failed to unlink(device%d) ret=%d",
 				 TEST_DEV_ID, ret);
 
 	/* 4links and 2 unlinks */