[dpdk-dev] [PATCH] i40e: fix the issue of not freeing memzone

[dpdk-dev] [PATCH] i40e: fix the issue of not freeing memzone

[Helin Zhang]

This fixes the issue of not freeing memzone in a call to free the
memory for adminq DMA.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 doc/guides/rel_notes/release_2_2.rst |  5 +++++
 drivers/net/i40e/base/i40e_osdep.h   |  2 +-
 drivers/net/i40e/i40e_ethdev.c       | 12 +++++++-----
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
index 59dda59..eaa906c 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -150,6 +150,11 @@ Drivers
 
   Added discarding packets on VSI to the stats and rectify the old statistics.
 
+* **i40e: Fixed issue of not freeing memzone.**
+
+  Fixed the issue of not freeing memzone in the call to free the memory for
+  adminq DMA.
+
 * **vhost: Fixed Qemu shutdown.**
 
   Fixed issue with libvirt ``virsh destroy`` not killing the VM.
diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
index 70d2721..71077f0 100644
--- a/drivers/net/i40e/base/i40e_osdep.h
+++ b/drivers/net/i40e/base/i40e_osdep.h
@@ -146,7 +146,7 @@ struct i40e_dma_mem {
 	void *va;
 	u64 pa;
 	u32 size;
-	u64 id;
+	const void *zone;
 } __attribute__((packed));
 
 #define i40e_allocate_dma_mem(h, m, unused, s, a) \
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index ddf3d38..8d6c0fa 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2910,15 +2910,13 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 			u64 size,
 			u32 alignment)
 {
-	static uint64_t id = 0;
 	const struct rte_memzone *mz = NULL;
 	char z_name[RTE_MEMZONE_NAMESIZE];
 
 	if (!mem)
 		return I40E_ERR_PARAM;
 
-	id++;
-	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, id);
+	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, rte_rand());
 #ifdef RTE_LIBRTE_XEN_DOM0
 	mz = rte_memzone_reserve_bounded(z_name, size, SOCKET_ID_ANY, 0,
 					 alignment, RTE_PGSIZE_2M);
@@ -2929,7 +2927,6 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 	if (!mz)
 		return I40E_ERR_NO_MEMORY;
 
-	mem->id = id;
 	mem->size = size;
 	mem->va = mz->addr;
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -2937,6 +2934,8 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 #else
 	mem->pa = mz->phys_addr;
 #endif
+	mem->zone = (const void *)mz;
+	PMD_DRV_LOG(DEBUG, "memzone allocated: %p", mem->zone);
 
 	return I40E_SUCCESS;
 }
@@ -2950,9 +2949,12 @@ enum i40e_status_code
 i40e_free_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 		    struct i40e_dma_mem *mem)
 {
-	if (!mem || !mem->va)
+	if (!mem)
 		return I40E_ERR_PARAM;
 
+	PMD_DRV_LOG(DEBUG, "memzone to be freed: %p", mem->zone);
+	rte_memzone_free((const struct rte_memzone *)mem->zone);
+	mem->zone = NULL;
 	mem->va = NULL;
 	mem->pa = (u64)0;
 
-- 
1.9.3

Re: [dpdk-dev] [PATCH] i40e: fix the issue of not freeing memzone

[Wu, Jingjing]

> -	static uint64_t id = 0;
>  	const struct rte_memzone *mz = NULL;
>  	char z_name[RTE_MEMZONE_NAMESIZE];
> 
>  	if (!mem)
>  		return I40E_ERR_PARAM;
> 
> -	id++;
> -	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, id);
> +	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, rte_rand());

Why change id++ to rte_rand() ?

>  #ifdef RTE_LIBRTE_XEN_DOM0
>  	mz = rte_memzone_reserve_bounded(z_name, size,
> SOCKET_ID_ANY, 0,
>  					 alignment, RTE_PGSIZE_2M);
> @@ -2929,7 +2927,6 @@
> i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
>  	if (!mz)
>  		return I40E_ERR_NO_MEMORY;
> 
> -	mem->id = id;
>  	mem->size = size;
>  	mem->va = mz->addr;
>  #ifdef RTE_LIBRTE_XEN_DOM0
> @@ -2937,6 +2934,8 @@
> i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
> #else
>  	mem->pa = mz->phys_addr;
>  #endif
> +	mem->zone = (const void *)mz;
> +	PMD_DRV_LOG(DEBUG, "memzone allocated: %p", mem->zone);
> 
Why not print the name of mem_zone instead of pointer?


Thanks
Jingjing

Re: [dpdk-dev] [PATCH] i40e: fix the issue of not freeing memzone

[Zhang, Helin]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Friday, November 6, 2015 1:40 PM
> To: Zhang, Helin; dev@dpdk.org
> Subject: RE: [PATCH] i40e: fix the issue of not freeing memzone
> 
> > -	static uint64_t id = 0;
> >  	const struct rte_memzone *mz = NULL;
> >  	char z_name[RTE_MEMZONE_NAMESIZE];
> >
> >  	if (!mem)
> >  		return I40E_ERR_PARAM;
> >
> > -	id++;
> > -	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, id);
> > +	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, rte_rand());
> 
> Why change id++ to rte_rand() ?
Don't need to maintain the ID, which may have race condition issue.
Get a random data is good enough. Some other PMDs are using tsc count for the similar purpose.

> 
> >  #ifdef RTE_LIBRTE_XEN_DOM0
> >  	mz = rte_memzone_reserve_bounded(z_name, size, SOCKET_ID_ANY, 0,
> >  					 alignment, RTE_PGSIZE_2M);
> > @@ -2929,7 +2927,6 @@
> > i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
> >  	if (!mz)
> >  		return I40E_ERR_NO_MEMORY;
> >
> > -	mem->id = id;
> >  	mem->size = size;
> >  	mem->va = mz->addr;
> >  #ifdef RTE_LIBRTE_XEN_DOM0
> > @@ -2937,6 +2934,8 @@
> > i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
> > #else
> >  	mem->pa = mz->phys_addr;
> >  #endif
> > +	mem->zone = (const void *)mz;
> > +	PMD_DRV_LOG(DEBUG, "memzone allocated: %p", mem->zone);
> >
> Why not print the name of mem_zone instead of pointer?
Good idea to print the name instead, and possible physical address, virtual address, etc.

Regards,
Helin

> 
> 
> Thanks
> Jingjing

[dpdk-dev] [PATCH v2] i40e: fix the issue of not freeing memzone

[Helin Zhang]

This fixes the issue of not freeing memzone in a call to free the
memory for adminq DMA.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 doc/guides/rel_notes/release_2_2.rst |  5 +++++
 drivers/net/i40e/base/i40e_osdep.h   |  2 +-
 drivers/net/i40e/i40e_ethdev.c       | 14 +++++++++-----
 3 files changed, 15 insertions(+), 6 deletions(-)

v2 changes:
Reworked debug messages.

diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
index 59dda59..eaa906c 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -150,6 +150,11 @@ Drivers
 
   Added discarding packets on VSI to the stats and rectify the old statistics.
 
+* **i40e: Fixed issue of not freeing memzone.**
+
+  Fixed the issue of not freeing memzone in the call to free the memory for
+  adminq DMA.
+
 * **vhost: Fixed Qemu shutdown.**
 
   Fixed issue with libvirt ``virsh destroy`` not killing the VM.
diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
index 70d2721..71077f0 100644
--- a/drivers/net/i40e/base/i40e_osdep.h
+++ b/drivers/net/i40e/base/i40e_osdep.h
@@ -146,7 +146,7 @@ struct i40e_dma_mem {
 	void *va;
 	u64 pa;
 	u32 size;
-	u64 id;
+	const void *zone;
 } __attribute__((packed));
 
 #define i40e_allocate_dma_mem(h, m, unused, s, a) \
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index ddf3d38..9f06ec2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2910,15 +2910,13 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 			u64 size,
 			u32 alignment)
 {
-	static uint64_t id = 0;
 	const struct rte_memzone *mz = NULL;
 	char z_name[RTE_MEMZONE_NAMESIZE];
 
 	if (!mem)
 		return I40E_ERR_PARAM;
 
-	id++;
-	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, id);
+	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, rte_rand());
 #ifdef RTE_LIBRTE_XEN_DOM0
 	mz = rte_memzone_reserve_bounded(z_name, size, SOCKET_ID_ANY, 0,
 					 alignment, RTE_PGSIZE_2M);
@@ -2929,7 +2927,6 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 	if (!mz)
 		return I40E_ERR_NO_MEMORY;
 
-	mem->id = id;
 	mem->size = size;
 	mem->va = mz->addr;
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -2937,6 +2934,9 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 #else
 	mem->pa = mz->phys_addr;
 #endif
+	mem->zone = (const void *)mz;
+	PMD_DRV_LOG(DEBUG, "memzone %s allocated with physical address: %p",
+		    mz->name, mem->pa);
 
 	return I40E_SUCCESS;
 }
@@ -2950,9 +2950,13 @@ enum i40e_status_code
 i40e_free_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 		    struct i40e_dma_mem *mem)
 {
-	if (!mem || !mem->va)
+	if (!mem)
 		return I40E_ERR_PARAM;
 
+	PMD_DRV_LOG(DEBUG, "memzone %s to be freed with physical address: %p",
+		    ((const struct rte_memzone *)mem->zone)->name, mem->pa);
+	rte_memzone_free((const struct rte_memzone *)mem->zone);
+	mem->zone = NULL;
 	mem->va = NULL;
 	mem->pa = (u64)0;
 
-- 
1.9.3

Re: [dpdk-dev] [PATCH v2] i40e: fix the issue of not freeing memzone

[Wu, Jingjing]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Friday, November 06, 2015 3:57 PM
> To: dev@dpdk.org
> Cc: Wu, Jingjing; Zhang, Helin
> Subject: [PATCH v2] i40e: fix the issue of not freeing memzone
> 
> This fixes the issue of not freeing memzone in a call to free the memory for
> adminq DMA.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Re: [dpdk-dev] [PATCH v2] i40e: fix the issue of not freeing memzone

[Thomas Monjalon]

2015-11-06 15:57, Helin Zhang:
> This fixes the issue of not freeing memzone in a call to free the
> memory for adminq DMA.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>

Please could you add a "Fixes:" line?
Thanks

Re: [dpdk-dev] [PATCH v2] i40e: fix the issue of not freeing memzone

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, November 6, 2015 6:20 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] i40e: fix the issue of not freeing memzone
> 
> 2015-11-06 15:57, Helin Zhang:
> > This fixes the issue of not freeing memzone in a call to free the
> > memory for adminq DMA.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> 
> Please could you add a "Fixes:" line?
> Thanks

Sure, will send a new patch for that. Thanks!

Helin

[dpdk-dev] [PATCH v3] i40e: fix the issue of not freeing memzone

[Helin Zhang]

This fixes the issue of not freeing memzone in a call to free the
memory for adminq DMA.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 doc/guides/rel_notes/release_2_2.rst |  5 +++++
 drivers/net/i40e/base/i40e_osdep.h   |  2 +-
 drivers/net/i40e/i40e_ethdev.c       | 14 +++++++++-----
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
index 59dda59..eaa906c 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -150,6 +150,11 @@ Drivers
 
   Added discarding packets on VSI to the stats and rectify the old statistics.
 
+* **i40e: Fixed issue of not freeing memzone.**
+
+  Fixed the issue of not freeing memzone in the call to free the memory for
+  adminq DMA.
+
 * **vhost: Fixed Qemu shutdown.**
 
   Fixed issue with libvirt ``virsh destroy`` not killing the VM.
diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
index 70d2721..71077f0 100644
--- a/drivers/net/i40e/base/i40e_osdep.h
+++ b/drivers/net/i40e/base/i40e_osdep.h
@@ -146,7 +146,7 @@ struct i40e_dma_mem {
 	void *va;
 	u64 pa;
 	u32 size;
-	u64 id;
+	const void *zone;
 } __attribute__((packed));
 
 #define i40e_allocate_dma_mem(h, m, unused, s, a) \
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index ddf3d38..9f06ec2 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2910,15 +2910,13 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 			u64 size,
 			u32 alignment)
 {
-	static uint64_t id = 0;
 	const struct rte_memzone *mz = NULL;
 	char z_name[RTE_MEMZONE_NAMESIZE];
 
 	if (!mem)
 		return I40E_ERR_PARAM;
 
-	id++;
-	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, id);
+	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, rte_rand());
 #ifdef RTE_LIBRTE_XEN_DOM0
 	mz = rte_memzone_reserve_bounded(z_name, size, SOCKET_ID_ANY, 0,
 					 alignment, RTE_PGSIZE_2M);
@@ -2929,7 +2927,6 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 	if (!mz)
 		return I40E_ERR_NO_MEMORY;
 
-	mem->id = id;
 	mem->size = size;
 	mem->va = mz->addr;
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -2937,6 +2934,9 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 #else
 	mem->pa = mz->phys_addr;
 #endif
+	mem->zone = (const void *)mz;
+	PMD_DRV_LOG(DEBUG, "memzone %s allocated with physical address: %p",
+		    mz->name, mem->pa);
 
 	return I40E_SUCCESS;
 }
@@ -2950,9 +2950,13 @@ enum i40e_status_code
 i40e_free_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 		    struct i40e_dma_mem *mem)
 {
-	if (!mem || !mem->va)
+	if (!mem)
 		return I40E_ERR_PARAM;
 
+	PMD_DRV_LOG(DEBUG, "memzone %s to be freed with physical address: %p",
+		    ((const struct rte_memzone *)mem->zone)->name, mem->pa);
+	rte_memzone_free((const struct rte_memzone *)mem->zone);
+	mem->zone = NULL;
 	mem->va = NULL;
 	mem->pa = (u64)0;
 
-- 
1.9.3

Re: [dpdk-dev] [PATCH v3] i40e: fix the issue of not freeing memzone

[Thomas Monjalon]

Helin,
There is a compilation error.

2015-11-09 09:20, Helin Zhang:
> +	mem->zone = (const void *)mz;
> +	PMD_DRV_LOG(DEBUG, "memzone %s allocated with physical address: %p",
> +		    mz->name, mem->pa);

error: format ‘%p’ expects argument of type ‘void *’, but argument 6 has type ‘u64 {aka long unsigned int}’

[dpdk-dev] [PATCH v4] i40e: fix the issue of not freeing memzone

[Helin Zhang]

This fixes the issue of not freeing memzone in a call to free the
memory for adminq DMA.

Fixes: 4861cde46116 ("i40e: new poll mode driver")

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 doc/guides/rel_notes/release_2_2.rst |  5 +++++
 drivers/net/i40e/base/i40e_osdep.h   |  2 +-
 drivers/net/i40e/i40e_ethdev.c       | 15 ++++++++++-----
 3 files changed, 16 insertions(+), 6 deletions(-)

v2 changes:
Reworked the debug logs.

v3 changes:
Added more info into the commit logs.

v4 changes:
Fixed the compile issues in debug mode.

diff --git a/doc/guides/rel_notes/release_2_2.rst b/doc/guides/rel_notes/release_2_2.rst
index 59dda59..eaa906c 100644
--- a/doc/guides/rel_notes/release_2_2.rst
+++ b/doc/guides/rel_notes/release_2_2.rst
@@ -150,6 +150,11 @@ Drivers
 
   Added discarding packets on VSI to the stats and rectify the old statistics.
 
+* **i40e: Fixed issue of not freeing memzone.**
+
+  Fixed the issue of not freeing memzone in the call to free the memory for
+  adminq DMA.
+
 * **vhost: Fixed Qemu shutdown.**
 
   Fixed issue with libvirt ``virsh destroy`` not killing the VM.
diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
index 70d2721..71077f0 100644
--- a/drivers/net/i40e/base/i40e_osdep.h
+++ b/drivers/net/i40e/base/i40e_osdep.h
@@ -146,7 +146,7 @@ struct i40e_dma_mem {
 	void *va;
 	u64 pa;
 	u32 size;
-	u64 id;
+	const void *zone;
 } __attribute__((packed));
 
 #define i40e_allocate_dma_mem(h, m, unused, s, a) \
diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index ddf3d38..c156665 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -2910,15 +2910,13 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 			u64 size,
 			u32 alignment)
 {
-	static uint64_t id = 0;
 	const struct rte_memzone *mz = NULL;
 	char z_name[RTE_MEMZONE_NAMESIZE];
 
 	if (!mem)
 		return I40E_ERR_PARAM;
 
-	id++;
-	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, id);
+	snprintf(z_name, sizeof(z_name), "i40e_dma_%"PRIu64, rte_rand());
 #ifdef RTE_LIBRTE_XEN_DOM0
 	mz = rte_memzone_reserve_bounded(z_name, size, SOCKET_ID_ANY, 0,
 					 alignment, RTE_PGSIZE_2M);
@@ -2929,7 +2927,6 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 	if (!mz)
 		return I40E_ERR_NO_MEMORY;
 
-	mem->id = id;
 	mem->size = size;
 	mem->va = mz->addr;
 #ifdef RTE_LIBRTE_XEN_DOM0
@@ -2937,6 +2934,9 @@ i40e_allocate_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 #else
 	mem->pa = mz->phys_addr;
 #endif
+	mem->zone = (const void *)mz;
+	PMD_DRV_LOG(DEBUG, "memzone %s allocated with physical address: "
+		    "%"PRIu64, mz->name, mem->pa);
 
 	return I40E_SUCCESS;
 }
@@ -2950,9 +2950,14 @@ enum i40e_status_code
 i40e_free_dma_mem_d(__attribute__((unused)) struct i40e_hw *hw,
 		    struct i40e_dma_mem *mem)
 {
-	if (!mem || !mem->va)
+	if (!mem)
 		return I40E_ERR_PARAM;
 
+	PMD_DRV_LOG(DEBUG, "memzone %s to be freed with physical address: "
+		    "%"PRIu64, ((const struct rte_memzone *)mem->zone)->name,
+		    mem->pa);
+	rte_memzone_free((const struct rte_memzone *)mem->zone);
+	mem->zone = NULL;
 	mem->va = NULL;
 	mem->pa = (u64)0;
 
-- 
1.9.3

Re: [dpdk-dev] [PATCH v4] i40e: fix the issue of not freeing memzone

[Thomas Monjalon]

2015-11-11 13:28, Helin Zhang:
> This fixes the issue of not freeing memzone in a call to free the
> memory for adminq DMA.
> 
> Fixes: 4861cde46116 ("i40e: new poll mode driver")
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>

Applied, thanks