DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] enic: fix free function to actually free memory
@ 2016-06-14 23:54 Nelson Escobar
  2016-06-23 11:36 ` Bruce Richardson
  0 siblings, 1 reply; 4+ messages in thread
From: Nelson Escobar @ 2016-06-14 23:54 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, johndale, Nelson Escobar

enic_alloc_consistent() allocated memory, but enic_free_consistent()
was an empty function, so allocated memory was never freed.

Fixes: fefed3d1e62c ("enic: new driver")

Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
 drivers/net/enic/base/vnic_dev.c | 14 +++++-----
 drivers/net/enic/base/vnic_dev.h |  2 +-
 drivers/net/enic/enic.h          | 12 +++++++++
 drivers/net/enic/enic_main.c     | 56 +++++++++++++++++++++++++++++++++-------
 4 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/drivers/net/enic/base/vnic_dev.c b/drivers/net/enic/base/vnic_dev.c
index e8a5028..fc2e4cc 100644
--- a/drivers/net/enic/base/vnic_dev.c
+++ b/drivers/net/enic/base/vnic_dev.c
@@ -83,7 +83,7 @@ struct vnic_dev {
 	struct vnic_intr_coal_timer_info intr_coal_timer_info;
 	void *(*alloc_consistent)(void *priv, size_t size,
 		dma_addr_t *dma_handle, u8 *name);
-	void (*free_consistent)(struct rte_pci_device *hwdev,
+	void (*free_consistent)(void *priv,
 		size_t size, void *vaddr,
 		dma_addr_t dma_handle);
 };
@@ -101,7 +101,7 @@ void *vnic_dev_priv(struct vnic_dev *vdev)
 void vnic_register_cbacks(struct vnic_dev *vdev,
 	void *(*alloc_consistent)(void *priv, size_t size,
 	    dma_addr_t *dma_handle, u8 *name),
-	void (*free_consistent)(struct rte_pci_device *hwdev,
+	void (*free_consistent)(void *priv,
 	    size_t size, void *vaddr,
 	    dma_addr_t dma_handle))
 {
@@ -807,7 +807,7 @@ int vnic_dev_notify_unsetcmd(struct vnic_dev *vdev)
 int vnic_dev_notify_unset(struct vnic_dev *vdev)
 {
 	if (vdev->notify && !vnic_dev_in_reset(vdev)) {
-		vdev->free_consistent(vdev->pdev,
+		vdev->free_consistent(vdev->priv,
 			sizeof(struct vnic_devcmd_notify),
 			vdev->notify,
 			vdev->notify_pa);
@@ -924,16 +924,16 @@ void vnic_dev_unregister(struct vnic_dev *vdev)
 {
 	if (vdev) {
 		if (vdev->notify)
-			vdev->free_consistent(vdev->pdev,
+			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_devcmd_notify),
 				vdev->notify,
 				vdev->notify_pa);
 		if (vdev->stats)
-			vdev->free_consistent(vdev->pdev,
+			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_stats),
 				vdev->stats, vdev->stats_pa);
 		if (vdev->fw_info)
-			vdev->free_consistent(vdev->pdev,
+			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_devcmd_fw_info),
 				vdev->fw_info, vdev->fw_info_pa);
 		kfree(vdev);
@@ -1041,7 +1041,7 @@ int vnic_dev_classifier(struct vnic_dev *vdev, u8 cmd, u16 *entry,
 
 		ret = vnic_dev_cmd(vdev, CMD_ADD_FILTER, &a0, &a1, wait);
 		*entry = (u16)a0;
-		vdev->free_consistent(vdev->pdev, tlv_size, tlv_va, tlv_pa);
+		vdev->free_consistent(vdev->priv, tlv_size, tlv_va, tlv_pa);
 	} else if (cmd == CLSF_DEL) {
 		a0 = *entry;
 		ret = vnic_dev_cmd(vdev, CMD_DEL_FILTER, &a0, &a1, wait);
diff --git a/drivers/net/enic/base/vnic_dev.h b/drivers/net/enic/base/vnic_dev.h
index 113d6ac..689442f 100644
--- a/drivers/net/enic/base/vnic_dev.h
+++ b/drivers/net/enic/base/vnic_dev.h
@@ -102,7 +102,7 @@ unsigned int vnic_dev_get_res_count(struct vnic_dev *vdev,
 void vnic_register_cbacks(struct vnic_dev *vdev,
 	void *(*alloc_consistent)(void *priv, size_t size,
 		dma_addr_t *dma_handle, u8 *name),
-	void (*free_consistent)(struct rte_pci_device *hwdev,
+	void (*free_consistent)(void *priv,
 		size_t size, void *vaddr,
 		dma_addr_t dma_handle));
 void __iomem *vnic_dev_get_res(struct vnic_dev *vdev, enum vnic_res_type type,
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 1e6914e..9f94afb 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -46,6 +46,8 @@
 #include "vnic_rss.h"
 #include "enic_res.h"
 #include "cq_enet_desc.h"
+#include <sys/queue.h>
+#include <rte_spinlock.h>
 
 #define DRV_NAME		"enic_pmd"
 #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Poll-mode Driver"
@@ -95,6 +97,11 @@ struct enic_soft_stats {
 	rte_atomic64_t rx_packet_errors;
 };
 
+struct enic_memzone_entry {
+	const struct rte_memzone *rz;
+	LIST_ENTRY(enic_memzone_entry) entries;
+};
+
 /* Per-instance private data structure */
 struct enic {
 	struct enic *next;
@@ -140,6 +147,11 @@ struct enic {
 
 	/* software counters */
 	struct enic_soft_stats soft_stats;
+
+	/* linked list storing memory allocations */
+	LIST_HEAD(enic_memzone_list, enic_memzone_entry) memzone_list;
+	rte_spinlock_t memzone_list_lock;
+
 };
 
 static inline unsigned int enic_cq_rq(__rte_unused struct enic *enic, unsigned int rq)
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 32ecdae..0576a6e 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -328,12 +328,14 @@ enic_alloc_rx_queue_mbufs(struct enic *enic, struct vnic_rq *rq)
 }
 
 static void *
-enic_alloc_consistent(__rte_unused void *priv, size_t size,
+enic_alloc_consistent(void *priv, size_t size,
 	dma_addr_t *dma_handle, u8 *name)
 {
 	void *vaddr;
 	const struct rte_memzone *rz;
 	*dma_handle = 0;
+	struct enic *enic = (struct enic *)priv;
+	struct enic_memzone_entry *mze;
 
 	rz = rte_memzone_reserve_aligned((const char *)name,
 					 size, SOCKET_ID_ANY, 0, ENIC_ALIGN);
@@ -346,16 +348,49 @@ enic_alloc_consistent(__rte_unused void *priv, size_t size,
 	vaddr = rz->addr;
 	*dma_handle = (dma_addr_t)rz->phys_addr;
 
+	mze = rte_malloc("enic memzone entry",
+			 sizeof(struct enic_memzone_entry), 0);
+
+	if (!mze) {
+		pr_err("%s : Failed to allocate memory for memzone list\n",
+		       __func__);
+		rte_memzone_free(rz);
+	}
+
+	mze->rz = rz;
+
+	rte_spinlock_lock(&enic->memzone_list_lock);
+	LIST_INSERT_HEAD(&enic->memzone_list, mze, entries);
+	rte_spinlock_unlock(&enic->memzone_list_lock);
+
 	return vaddr;
 }
 
 static void
-enic_free_consistent(__rte_unused struct rte_pci_device *hwdev,
-	__rte_unused size_t size,
-	__rte_unused void *vaddr,
-	__rte_unused dma_addr_t dma_handle)
-{
-	/* Nothing to be done */
+enic_free_consistent(void *priv,
+		     __rte_unused size_t size,
+		     void *vaddr,
+		     dma_addr_t dma_handle)
+{
+	struct enic_memzone_entry *mze;
+	struct enic *enic = (struct enic *)priv;
+
+	rte_spinlock_lock(&enic->memzone_list_lock);
+	LIST_FOREACH(mze, &enic->memzone_list, entries) {
+		if (mze->rz->addr == vaddr &&
+		    mze->rz->phys_addr == dma_handle)
+			break;
+	}
+	if (mze == NULL) {
+		rte_spinlock_unlock(&enic->memzone_list_lock);
+		dev_warning(enic,
+			    "Tried to free memory, but couldn't find it in the memzone list\n");
+		return;
+	}
+	LIST_REMOVE(mze, entries);
+	rte_spinlock_unlock(&enic->memzone_list_lock);
+	rte_memzone_free(mze->rz);
+	rte_free(mze);
 }
 
 static void
@@ -696,7 +731,7 @@ static int enic_set_rsskey(struct enic *enic)
 		rss_key_buf_pa,
 		sizeof(union vnic_rss_key));
 
-	enic_free_consistent(enic->pdev, sizeof(union vnic_rss_key),
+	enic_free_consistent(enic, sizeof(union vnic_rss_key),
 		rss_key_buf_va, rss_key_buf_pa);
 
 	return err;
@@ -723,7 +758,7 @@ static int enic_set_rsscpu(struct enic *enic, u8 rss_hash_bits)
 		rss_cpu_buf_pa,
 		sizeof(union vnic_rss_cpu));
 
-	enic_free_consistent(enic->pdev, sizeof(union vnic_rss_cpu),
+	enic_free_consistent(enic, sizeof(union vnic_rss_cpu),
 		rss_cpu_buf_va, rss_cpu_buf_pa);
 
 	return err;
@@ -905,6 +940,9 @@ int enic_probe(struct enic *enic)
 		goto err_out;
 	}
 
+	LIST_INIT(&enic->memzone_list);
+	rte_spinlock_init(&enic->memzone_list_lock);
+
 	vnic_register_cbacks(enic->vdev,
 		enic_alloc_consistent,
 		enic_free_consistent);
-- 
2.7.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] enic: fix free function to actually free memory
  2016-06-14 23:54 [dpdk-dev] [PATCH] enic: fix free function to actually free memory Nelson Escobar
@ 2016-06-23 11:36 ` Bruce Richardson
  2016-06-23 23:14   ` [dpdk-dev] [PATCH v2] " Nelson Escobar
  0 siblings, 1 reply; 4+ messages in thread
From: Bruce Richardson @ 2016-06-23 11:36 UTC (permalink / raw)
  To: Nelson Escobar; +Cc: dev, johndale

On Tue, Jun 14, 2016 at 04:54:54PM -0700, Nelson Escobar wrote:
> enic_alloc_consistent() allocated memory, but enic_free_consistent()
> was an empty function, so allocated memory was never freed.
> 

The additions to track memzones also needs to be described in the commit message,
since this patch does more than just provide an implementation for the
free function.

Regards,
/Bruce

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [dpdk-dev] [PATCH v2] enic: fix free function to actually free memory
  2016-06-23 11:36 ` Bruce Richardson
@ 2016-06-23 23:14   ` Nelson Escobar
  2016-06-28 11:09     ` Bruce Richardson
  0 siblings, 1 reply; 4+ messages in thread
From: Nelson Escobar @ 2016-06-23 23:14 UTC (permalink / raw)
  To: dev; +Cc: bruce.richardson, Nelson Escobar

enic_alloc_consistent() allocated memory, but enic_free_consistent()
was an empty function, so allocated memory was never freed.

This commit adds a list and lock to the enic structure to keep track
of the memzones allocated in enic_alloc_consistent(), and
enic_free_consistent() uses that information to properly free memory.

Fixes: fefed3d1e62c ("enic: new driver")

Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Reviewed-by: John Daley <johndale@cisco.com>
---
v2:
  - updated commit message to mention memzone tracking

 drivers/net/enic/base/vnic_dev.c | 14 +++++-----
 drivers/net/enic/base/vnic_dev.h |  2 +-
 drivers/net/enic/enic.h          | 12 +++++++++
 drivers/net/enic/enic_main.c     | 56 +++++++++++++++++++++++++++++++++-------
 4 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/drivers/net/enic/base/vnic_dev.c b/drivers/net/enic/base/vnic_dev.c
index e8a5028..fc2e4cc 100644
--- a/drivers/net/enic/base/vnic_dev.c
+++ b/drivers/net/enic/base/vnic_dev.c
@@ -83,7 +83,7 @@ struct vnic_dev {
 	struct vnic_intr_coal_timer_info intr_coal_timer_info;
 	void *(*alloc_consistent)(void *priv, size_t size,
 		dma_addr_t *dma_handle, u8 *name);
-	void (*free_consistent)(struct rte_pci_device *hwdev,
+	void (*free_consistent)(void *priv,
 		size_t size, void *vaddr,
 		dma_addr_t dma_handle);
 };
@@ -101,7 +101,7 @@ void *vnic_dev_priv(struct vnic_dev *vdev)
 void vnic_register_cbacks(struct vnic_dev *vdev,
 	void *(*alloc_consistent)(void *priv, size_t size,
 	    dma_addr_t *dma_handle, u8 *name),
-	void (*free_consistent)(struct rte_pci_device *hwdev,
+	void (*free_consistent)(void *priv,
 	    size_t size, void *vaddr,
 	    dma_addr_t dma_handle))
 {
@@ -807,7 +807,7 @@ int vnic_dev_notify_unsetcmd(struct vnic_dev *vdev)
 int vnic_dev_notify_unset(struct vnic_dev *vdev)
 {
 	if (vdev->notify && !vnic_dev_in_reset(vdev)) {
-		vdev->free_consistent(vdev->pdev,
+		vdev->free_consistent(vdev->priv,
 			sizeof(struct vnic_devcmd_notify),
 			vdev->notify,
 			vdev->notify_pa);
@@ -924,16 +924,16 @@ void vnic_dev_unregister(struct vnic_dev *vdev)
 {
 	if (vdev) {
 		if (vdev->notify)
-			vdev->free_consistent(vdev->pdev,
+			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_devcmd_notify),
 				vdev->notify,
 				vdev->notify_pa);
 		if (vdev->stats)
-			vdev->free_consistent(vdev->pdev,
+			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_stats),
 				vdev->stats, vdev->stats_pa);
 		if (vdev->fw_info)
-			vdev->free_consistent(vdev->pdev,
+			vdev->free_consistent(vdev->priv,
 				sizeof(struct vnic_devcmd_fw_info),
 				vdev->fw_info, vdev->fw_info_pa);
 		kfree(vdev);
@@ -1041,7 +1041,7 @@ int vnic_dev_classifier(struct vnic_dev *vdev, u8 cmd, u16 *entry,
 
 		ret = vnic_dev_cmd(vdev, CMD_ADD_FILTER, &a0, &a1, wait);
 		*entry = (u16)a0;
-		vdev->free_consistent(vdev->pdev, tlv_size, tlv_va, tlv_pa);
+		vdev->free_consistent(vdev->priv, tlv_size, tlv_va, tlv_pa);
 	} else if (cmd == CLSF_DEL) {
 		a0 = *entry;
 		ret = vnic_dev_cmd(vdev, CMD_DEL_FILTER, &a0, &a1, wait);
diff --git a/drivers/net/enic/base/vnic_dev.h b/drivers/net/enic/base/vnic_dev.h
index 113d6ac..689442f 100644
--- a/drivers/net/enic/base/vnic_dev.h
+++ b/drivers/net/enic/base/vnic_dev.h
@@ -102,7 +102,7 @@ unsigned int vnic_dev_get_res_count(struct vnic_dev *vdev,
 void vnic_register_cbacks(struct vnic_dev *vdev,
 	void *(*alloc_consistent)(void *priv, size_t size,
 		dma_addr_t *dma_handle, u8 *name),
-	void (*free_consistent)(struct rte_pci_device *hwdev,
+	void (*free_consistent)(void *priv,
 		size_t size, void *vaddr,
 		dma_addr_t dma_handle));
 void __iomem *vnic_dev_get_res(struct vnic_dev *vdev, enum vnic_res_type type,
diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index 1e6914e..9f94afb 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -46,6 +46,8 @@
 #include "vnic_rss.h"
 #include "enic_res.h"
 #include "cq_enet_desc.h"
+#include <sys/queue.h>
+#include <rte_spinlock.h>
 
 #define DRV_NAME		"enic_pmd"
 #define DRV_DESCRIPTION		"Cisco VIC Ethernet NIC Poll-mode Driver"
@@ -95,6 +97,11 @@ struct enic_soft_stats {
 	rte_atomic64_t rx_packet_errors;
 };
 
+struct enic_memzone_entry {
+	const struct rte_memzone *rz;
+	LIST_ENTRY(enic_memzone_entry) entries;
+};
+
 /* Per-instance private data structure */
 struct enic {
 	struct enic *next;
@@ -140,6 +147,11 @@ struct enic {
 
 	/* software counters */
 	struct enic_soft_stats soft_stats;
+
+	/* linked list storing memory allocations */
+	LIST_HEAD(enic_memzone_list, enic_memzone_entry) memzone_list;
+	rte_spinlock_t memzone_list_lock;
+
 };
 
 static inline unsigned int enic_cq_rq(__rte_unused struct enic *enic, unsigned int rq)
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index e583b90..9b6fe36 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -328,12 +328,14 @@ enic_alloc_rx_queue_mbufs(struct enic *enic, struct vnic_rq *rq)
 }
 
 static void *
-enic_alloc_consistent(__rte_unused void *priv, size_t size,
+enic_alloc_consistent(void *priv, size_t size,
 	dma_addr_t *dma_handle, u8 *name)
 {
 	void *vaddr;
 	const struct rte_memzone *rz;
 	*dma_handle = 0;
+	struct enic *enic = (struct enic *)priv;
+	struct enic_memzone_entry *mze;
 
 	rz = rte_memzone_reserve_aligned((const char *)name,
 					 size, SOCKET_ID_ANY, 0, ENIC_ALIGN);
@@ -346,16 +348,49 @@ enic_alloc_consistent(__rte_unused void *priv, size_t size,
 	vaddr = rz->addr;
 	*dma_handle = (dma_addr_t)rz->phys_addr;
 
+	mze = rte_malloc("enic memzone entry",
+			 sizeof(struct enic_memzone_entry), 0);
+
+	if (!mze) {
+		pr_err("%s : Failed to allocate memory for memzone list\n",
+		       __func__);
+		rte_memzone_free(rz);
+	}
+
+	mze->rz = rz;
+
+	rte_spinlock_lock(&enic->memzone_list_lock);
+	LIST_INSERT_HEAD(&enic->memzone_list, mze, entries);
+	rte_spinlock_unlock(&enic->memzone_list_lock);
+
 	return vaddr;
 }
 
 static void
-enic_free_consistent(__rte_unused struct rte_pci_device *hwdev,
-	__rte_unused size_t size,
-	__rte_unused void *vaddr,
-	__rte_unused dma_addr_t dma_handle)
-{
-	/* Nothing to be done */
+enic_free_consistent(void *priv,
+		     __rte_unused size_t size,
+		     void *vaddr,
+		     dma_addr_t dma_handle)
+{
+	struct enic_memzone_entry *mze;
+	struct enic *enic = (struct enic *)priv;
+
+	rte_spinlock_lock(&enic->memzone_list_lock);
+	LIST_FOREACH(mze, &enic->memzone_list, entries) {
+		if (mze->rz->addr == vaddr &&
+		    mze->rz->phys_addr == dma_handle)
+			break;
+	}
+	if (mze == NULL) {
+		rte_spinlock_unlock(&enic->memzone_list_lock);
+		dev_warning(enic,
+			    "Tried to free memory, but couldn't find it in the memzone list\n");
+		return;
+	}
+	LIST_REMOVE(mze, entries);
+	rte_spinlock_unlock(&enic->memzone_list_lock);
+	rte_memzone_free(mze->rz);
+	rte_free(mze);
 }
 
 static void
@@ -696,7 +731,7 @@ static int enic_set_rsskey(struct enic *enic)
 		rss_key_buf_pa,
 		sizeof(union vnic_rss_key));
 
-	enic_free_consistent(enic->pdev, sizeof(union vnic_rss_key),
+	enic_free_consistent(enic, sizeof(union vnic_rss_key),
 		rss_key_buf_va, rss_key_buf_pa);
 
 	return err;
@@ -723,7 +758,7 @@ static int enic_set_rsscpu(struct enic *enic, u8 rss_hash_bits)
 		rss_cpu_buf_pa,
 		sizeof(union vnic_rss_cpu));
 
-	enic_free_consistent(enic->pdev, sizeof(union vnic_rss_cpu),
+	enic_free_consistent(enic, sizeof(union vnic_rss_cpu),
 		rss_cpu_buf_va, rss_cpu_buf_pa);
 
 	return err;
@@ -905,6 +940,9 @@ int enic_probe(struct enic *enic)
 		goto err_out;
 	}
 
+	LIST_INIT(&enic->memzone_list);
+	rte_spinlock_init(&enic->memzone_list_lock);
+
 	vnic_register_cbacks(enic->vdev,
 		enic_alloc_consistent,
 		enic_free_consistent);
-- 
2.7.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH v2] enic: fix free function to actually free memory
  2016-06-23 23:14   ` [dpdk-dev] [PATCH v2] " Nelson Escobar
@ 2016-06-28 11:09     ` Bruce Richardson
  0 siblings, 0 replies; 4+ messages in thread
From: Bruce Richardson @ 2016-06-28 11:09 UTC (permalink / raw)
  To: Nelson Escobar; +Cc: dev

On Thu, Jun 23, 2016 at 04:14:58PM -0700, Nelson Escobar wrote:
> enic_alloc_consistent() allocated memory, but enic_free_consistent()
> was an empty function, so allocated memory was never freed.
> 
> This commit adds a list and lock to the enic structure to keep track
> of the memzones allocated in enic_alloc_consistent(), and
> enic_free_consistent() uses that information to properly free memory.
> 
> Fixes: fefed3d1e62c ("enic: new driver")
> 
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Reviewed-by: John Daley <johndale@cisco.com>

Applied to dpdk-next-net/rel_16_07

/Bruce

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-06-28 11:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-14 23:54 [dpdk-dev] [PATCH] enic: fix free function to actually free memory Nelson Escobar
2016-06-23 11:36 ` Bruce Richardson
2016-06-23 23:14   ` [dpdk-dev] [PATCH v2] " Nelson Escobar
2016-06-28 11:09     ` Bruce Richardson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).